diff mbox series

[1/5] ALSA: pcm: Revert "ALSA: pcm: rewrite snd_pcm_playback_silence()"

Message ID 20230505073813.1219175-2-perex@perex.cz (mailing list archive)
State New, archived
Headers show
Series rewrite snd_pcm_playback_silence() again | expand

Commit Message

Jaroslav Kysela May 5, 2023, 7:38 a.m. UTC 
This reverts commit 9f656705c5faa18afb26d922cfc64f9fd103c38d.

There is a regression (the top-up mode). Unfortunately, the patch
provided from the author of this commit is not easy for the review.

Keep the update and new comments in headers.

Signed-off-by: Jaroslav Kysela <perex@perex.cz>
---
 sound/core/pcm_lib.c    | 86 ++++++++++++++++++++++++-----------------
 sound/core/pcm_local.h  |  3 +-
 sound/core/pcm_native.c |  6 +--
 3 files changed, 55 insertions(+), 40 deletions(-)

Comments

Takashi Iwai May 5, 2023, 9:31 a.m. UTC | #1 
On Fri, 05 May 2023 09:38:09 +0200,
Jaroslav Kysela wrote:
> 
> This reverts commit 9f656705c5faa18afb26d922cfc64f9fd103c38d.
> 
> There is a regression (the top-up mode). Unfortunately, the patch
> provided from the author of this commit is not easy for the review.
> 
> Keep the update and new comments in headers.
> 
> Signed-off-by: Jaroslav Kysela <perex@perex.cz>

Better to add Reported-by and the reference for the original thread
reporting the regression, as well as the Fixes tag.

I can cook up it by myself, too.


Takashi
Oswald Buddenhagen May 5, 2023, 9:38 a.m. UTC | #2 
On Fri, May 05, 2023 at 11:31:16AM +0200, Takashi Iwai wrote:
>On Fri, 05 May 2023 09:38:09 +0200,
>Jaroslav Kysela wrote:
>> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
>
>Better to add Reported-by and the reference for the original thread
>reporting the regression,
>
i'll post a slight rework of the series shortly, where i'll include 
this.

> as well as the Fixes tag.
>
that seems pointless for a revert, as all the info is already included 
anyway, no?

regards
Takashi Iwai May 5, 2023, 10:44 a.m. UTC | #3 
On Fri, 05 May 2023 11:38:45 +0200,
Oswald Buddenhagen wrote:
> 
> On Fri, May 05, 2023 at 11:31:16AM +0200, Takashi Iwai wrote:
> > On Fri, 05 May 2023 09:38:09 +0200,
> > Jaroslav Kysela wrote:
> >> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
> > 
> > Better to add Reported-by and the reference for the original thread
> > reporting the regression,
> > 
> i'll post a slight rework of the series shortly, where i'll include
> this.
> 
> > as well as the Fixes tag.
> > 
> that seems pointless for a revert, as all the info is already included
> anyway, no?

People tend to look at Fixes tag to judge whether the commit can fix
something real or not.  For example, we (at SUSE) regularly check
Fixes tag to pick up the missing fixing.
In the case of a revert, it doesn't mean always a fix.  So it's not
mandatory, but would be still helpful.


thanks,

Takashi
diff mbox series

Patch

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index d21c73944efd..af1eb136feb0 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -42,56 +42,70 @@  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_uframes_t noise_dist, ofs, transfer;
+	snd_pcm_uframes_t frames, 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) {
-		frames = runtime->silence_threshold - noise_dist;
-		if (frames <= 0)
+		snd_pcm_sframes_t noise_dist, n;
+		snd_pcm_uframes_t appl_ptr = READ_ONCE(runtime->control->appl_ptr);
+		if (runtime->silence_start != appl_ptr) {
+			n = appl_ptr - runtime->silence_start;
+			if (n < 0)
+				n += runtime->boundary;
+			if ((snd_pcm_uframes_t)n < runtime->silence_filled)
+				runtime->silence_filled -= n;
+			else
+				runtime->silence_filled = 0;
+			runtime->silence_start = appl_ptr;
+		}
+		if (runtime->silence_filled >= runtime->buffer_size)
+			return;
+		noise_dist = snd_pcm_playback_hw_avail(runtime) + runtime->silence_filled;
+		if (noise_dist >= (snd_pcm_sframes_t) runtime->silence_threshold)
 			return;
+		frames = runtime->silence_threshold - noise_dist;
 		if (frames > runtime->silence_size)
 			frames = runtime->silence_size;
 	} else {
-		frames = runtime->buffer_size - noise_dist;
-		if (frames <= 0)
-			return;
+		if (new_hw_ptr == ULONG_MAX) {	/* initialization */
+			snd_pcm_sframes_t avail = snd_pcm_playback_hw_avail(runtime);
+			if (avail > runtime->buffer_size)
+				avail = runtime->buffer_size;
+			runtime->silence_filled = avail > 0 ? avail : 0;
+			runtime->silence_start = (runtime->status->hw_ptr +
+						  runtime->silence_filled) %
+						 runtime->boundary;
+		} else {
+			ofs = runtime->status->hw_ptr;
+			frames = new_hw_ptr - ofs;
+			if ((snd_pcm_sframes_t)frames < 0)
+				frames += runtime->boundary;
+			runtime->silence_filled -= frames;
+			if ((snd_pcm_sframes_t)runtime->silence_filled < 0) {
+				runtime->silence_filled = 0;
+				runtime->silence_start = new_hw_ptr;
+			} else {
+				runtime->silence_start = ofs;
+			}
+		}
+		frames = runtime->buffer_size - runtime->silence_filled;
 	}
-
 	if (snd_BUG_ON(frames > runtime->buffer_size))
 		return;
-	ofs = (runtime->silence_start + runtime->silence_filled) % runtime->buffer_size;
-	do {
+	if (frames == 0)
+		return;
+	ofs = runtime->silence_start % runtime->buffer_size;
+	while (frames > 0) {
 		transfer = ofs + frames > runtime->buffer_size ? runtime->buffer_size - ofs : frames;
 		err = fill_silence_frames(substream, ofs, transfer);
 		snd_BUG_ON(err < 0);
 		runtime->silence_filled += transfer;
 		frames -= transfer;
 		ofs = 0;
-	} while (frames > 0);
+	}
 	snd_pcm_dma_buffer_sync(substream, SNDRV_DMA_SYNC_DEVICE);
 }
 
@@ -425,6 +439,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 +460,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 91c87cdb786e..39a65d1415ab 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);
 }