diff mbox series

ALSA: pcm: fix snd_pcm_playback_silence() with free-running mode

Message ID 20230504130007.2208916-1-oswald.buddenhagen@gmx.de
State New
Headers show
Series ALSA: pcm: fix snd_pcm_playback_silence() with free-running mode | expand

Commit Message

Oswald Buddenhagen May 4, 2023, 1 p.m. UTC 
Turns out that we cannot rely on the application pointer being updated
in top-up mode, as its primary purpose is to remain operational in
free-running mode as used by dmix.

So we logically revert some bits from commit 9f656705c5faa ("ALSA: pcm:
rewrite snd_pcm_playback_silence()"), but we retain the bug fixes and
try to make the code paths congruent.

Reported-by: Jeff Chua <jeff.chua.linux@gmail.com>
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sound/core/pcm_lib.c    | 82 +++++++++++++++++++++++++++--------------
 sound/core/pcm_local.h  |  3 +-
 sound/core/pcm_native.c |  6 +--
 3 files changed, 60 insertions(+), 31 deletions(-)

Comments

Jeff Chua May 4, 2023, 1:25 p.m. UTC | #1 
On Thu, May 4, 2023 at 9:08 PM Jaroslav Kysela <perex@perex.cz> wrote:
>
> On 04. 05. 23 15:00, Oswald Buddenhagen wrote:
> > Turns out that we cannot rely on the application pointer being updated
> > in top-up mode, as its primary purpose is to remain operational in
> > free-running mode as used by dmix.
> >
> > So we logically revert some bits from commit 9f656705c5faa ("ALSA: pcm:
> > rewrite snd_pcm_playback_silence()"), but we retain the bug fixes and
> > try to make the code paths congruent.
>
> It would be better to revert the broken patch and make changes on top of the
> original code. This is really difficult to review.

Just to confirm that the patch works. Thanks!

Jeff
diff mbox series

Patch

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index d21c73944efd..cd5f2ef14ab4 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -42,41 +42,69 @@  static int fill_silence_frames(struct snd_pcm_substream *substream,
  *
  * when runtime->silence_size >= runtime->boundary - fill processed area with silence immediately
  */
-void snd_pcm_playback_silence(struct snd_pcm_substream *substream)
+void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_uframes_t new_hw_ptr)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	snd_pcm_uframes_t appl_ptr = READ_ONCE(runtime->control->appl_ptr);
-	snd_pcm_sframes_t added, hw_avail, frames;
+	snd_pcm_sframes_t hw_avail, frames;
 	snd_pcm_uframes_t noise_dist, ofs, transfer;
 	int err;
 
-	added = appl_ptr - runtime->silence_start;
-	if (added) {
-		if (added < 0)
-			added += runtime->boundary;
-		if (added < runtime->silence_filled)
-			runtime->silence_filled -= added;
-		else
-			runtime->silence_filled = 0;
-		runtime->silence_start = appl_ptr;
-	}
-
-	// This will "legitimately" turn negative on underrun, and will be mangled
-	// into a huge number by the boundary crossing handling. The initial state
-	// might also be not quite sane. The code below MUST account for these cases.
-	hw_avail = appl_ptr - runtime->status->hw_ptr;
-	if (hw_avail < 0)
-		hw_avail += runtime->boundary;
-
-	noise_dist = hw_avail + runtime->silence_filled;
 	if (runtime->silence_size < runtime->boundary) {
+		snd_pcm_uframes_t appl_ptr = READ_ONCE(runtime->control->appl_ptr);
+		snd_pcm_sframes_t added = appl_ptr - runtime->silence_start;
+		if (added) {
+			if (added < 0)
+				added += runtime->boundary;
+			if (added < runtime->silence_filled)
+				runtime->silence_filled -= added;
+			else
+				runtime->silence_filled = 0;
+			runtime->silence_start = appl_ptr;
+		}
+
+		if (new_hw_ptr == ULONG_MAX)  // initialization
+			new_hw_ptr = runtime->status->hw_ptr;
+		// This will "legitimately" turn negative on underrun, and will be mangled
+		// into a huge number by the boundary crossing handling. The initial state
+		// might also be not quite sane. The code below MUST account for these cases.
+		hw_avail = appl_ptr - new_hw_ptr;
+		if (hw_avail < 0)
+			hw_avail += runtime->boundary;
+
+		noise_dist = hw_avail + runtime->silence_filled;
 		frames = runtime->silence_threshold - noise_dist;
 		if (frames <= 0)
 			return;
 		if (frames > runtime->silence_size)
 			frames = runtime->silence_size;
 	} else {
-		frames = runtime->buffer_size - noise_dist;
+		// This filling mode aims at free-running mode (used for example by dmix),
+		// which doesn't update the application pointer.
+		snd_pcm_uframes_t hw_ptr = runtime->status->hw_ptr;
+		if (new_hw_ptr == ULONG_MAX) {  // initialization
+			// Usually, this is entered while stopped, before data is queued,
+			// so both pointers are expected to be zero.
+			hw_avail = runtime->control->appl_ptr - hw_ptr;
+			if (hw_avail < 0)
+				hw_avail += runtime->boundary;
+			// In free-running mode, appl_ptr will be zero even while running,
+			// so we end up with a huge number. There is no useful way to
+			// handle this, so we just clear the whole buffer.
+			runtime->silence_filled = hw_avail > runtime->buffer_size ? 0 : hw_avail;
+			runtime->silence_start = hw_ptr;
+		} else {
+			snd_pcm_sframes_t played = new_hw_ptr - hw_ptr;
+			if (played) {
+				if (played < 0)
+					played += runtime->boundary;
+				if (played < runtime->silence_filled)
+					runtime->silence_filled -= played;
+				else  // This may happen due to a reset.
+					runtime->silence_filled = 0;
+				runtime->silence_start = new_hw_ptr;
+			}
+		}
+		frames = runtime->buffer_size - runtime->silence_filled;
 		if (frames <= 0)
 			return;
 	}
@@ -425,6 +453,10 @@  static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 		return 0;
 	}
 
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
+	    runtime->silence_size > 0)
+		snd_pcm_playback_silence(substream, new_hw_ptr);
+
 	if (in_interrupt) {
 		delta = new_hw_ptr - runtime->hw_ptr_interrupt;
 		if (delta < 0)
@@ -442,10 +474,6 @@  static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 		runtime->hw_ptr_wrap += runtime->boundary;
 	}
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
-	    runtime->silence_size > 0)
-		snd_pcm_playback_silence(substream);
-
 	update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp);
 
 	return snd_pcm_update_state(substream, runtime);
diff --git a/sound/core/pcm_local.h b/sound/core/pcm_local.h
index 42fe3a4e9154..ecb21697ae3a 100644
--- a/sound/core/pcm_local.h
+++ b/sound/core/pcm_local.h
@@ -29,7 +29,8 @@  int snd_pcm_update_state(struct snd_pcm_substream *substream,
 			 struct snd_pcm_runtime *runtime);
 int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream);
 
-void snd_pcm_playback_silence(struct snd_pcm_substream *substream);
+void snd_pcm_playback_silence(struct snd_pcm_substream *substream,
+			      snd_pcm_uframes_t new_hw_ptr);
 
 static inline snd_pcm_uframes_t
 snd_pcm_avail(struct snd_pcm_substream *substream)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 3d0c4a5b701b..94185267a7b9 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -958,7 +958,7 @@  static int snd_pcm_sw_params(struct snd_pcm_substream *substream,
 	if (snd_pcm_running(substream)) {
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
 		    runtime->silence_size > 0)
-			snd_pcm_playback_silence(substream);
+			snd_pcm_playback_silence(substream, ULONG_MAX);
 		err = snd_pcm_update_state(substream, runtime);
 	}
 	snd_pcm_stream_unlock_irq(substream);
@@ -1455,7 +1455,7 @@  static void snd_pcm_post_start(struct snd_pcm_substream *substream,
 	__snd_pcm_set_state(runtime, state);
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
 	    runtime->silence_size > 0)
-		snd_pcm_playback_silence(substream);
+		snd_pcm_playback_silence(substream, ULONG_MAX);
 	snd_pcm_timer_notify(substream, SNDRV_TIMER_EVENT_MSTART);
 }
 
@@ -1916,7 +1916,7 @@  static void snd_pcm_post_reset(struct snd_pcm_substream *substream,
 	runtime->control->appl_ptr = runtime->status->hw_ptr;
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
 	    runtime->silence_size > 0)
-		snd_pcm_playback_silence(substream);
+		snd_pcm_playback_silence(substream, ULONG_MAX);
 	snd_pcm_stream_unlock_irq(substream);
 }