[PATCH 1/7] ALSA: pcm: Revert "ALSA: pcm: rewrite snd_pcm_playback_silence()"
From: Jaroslav Kysela perex@perex.cz
This reverts commit 9f656705c5faa18afb26d922cfc64f9fd103c38d.
There was a regression (in the top-up mode). Unfortunately, the patch provided from the author of this commit is not easy to review.
Keep the updated and new comments in headers. Also add a new comment that documents the missed API constraint which led to the regression.
Reported-by: Jeff Chua jeff.chua.linux@gmail.com Link: https://lore.kernel.org/r/CAAJw_ZsbTVd3Es373x_wTNDF7RknGhCD0r+NKUSwAO7HpLAkY... Signed-off-by: Jaroslav Kysela perex@perex.cz Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- v2: - add docu comment - commit message improvements --- sound/core/pcm_lib.c | 90 ++++++++++++++++++++++++----------------- sound/core/pcm_local.h | 3 +- sound/core/pcm_native.c | 6 +-- 3 files changed, 59 insertions(+), 40 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index d21c73944efd..3357ffae635f 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -42,56 +42,74 @@ 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; + /* + * This filling mode aims at free-running mode (used for example by dmix), + * which doesn't update the application pointer. + */ + 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 +443,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 +464,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); }
From: Jaroslav Kysela perex@perex.cz
The snd_pcm_playback_hw_avail() function uses runtime->status->hw_ptr. Unfortunately, in case when we call this function from snd_pcm_update_hw_ptr0(), this variable contains the previous hardware pointer. Use the new_hw_ptr argument to calculate hw_avail (filled samples by the user space) to correct the threshold comparison.
The new_hw_ptr argument may also be set to ULONG_MAX which means the initialization phase. In this case, use runtime->status->hw_ptr.
Suggested-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de Signed-off-by: Jaroslav Kysela perex@perex.cz Reviewed-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/core/pcm_lib.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 3357ffae635f..6ad67e7e740c 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -63,7 +63,15 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram } if (runtime->silence_filled >= runtime->buffer_size) return; - noise_dist = snd_pcm_playback_hw_avail(runtime) + runtime->silence_filled; + /* initialization outside pointer updates */ + if (new_hw_ptr == ULONG_MAX) + new_hw_ptr = runtime->status->hw_ptr; + /* get hw_avail with the boundary crossing */ + noise_dist = appl_ptr - new_hw_ptr; + if (noise_dist < 0) + noise_dist += runtime->boundary; + /* total noise distance */ + noise_dist += runtime->silence_filled; if (noise_dist >= (snd_pcm_sframes_t) runtime->silence_threshold) return; frames = runtime->silence_threshold - noise_dist;
On Fri, 05 May 2023 12:31:35 +0200, Oswald Buddenhagen wrote:
From: Jaroslav Kysela perex@perex.cz
The snd_pcm_playback_hw_avail() function uses runtime->status->hw_ptr. Unfortunately, in case when we call this function from snd_pcm_update_hw_ptr0(), this variable contains the previous hardware pointer. Use the new_hw_ptr argument to calculate hw_avail (filled samples by the user space) to correct the threshold comparison.
The new_hw_ptr argument may also be set to ULONG_MAX which means the initialization phase. In this case, use runtime->status->hw_ptr.
Suggested-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de Signed-off-by: Jaroslav Kysela perex@perex.cz Reviewed-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de
Here misses your Signed-off-by tag. Ditto for the patch 3. It's a legal requirement, and I can't apply patches without it.
Could you resubmit the series quickly with your SOB?
thanks,
Takashi
On Fri, May 05, 2023 at 05:22:57PM +0200, Takashi Iwai wrote:
On Fri, 05 May 2023 12:31:35 +0200, Oswald Buddenhagen wrote:
Suggested-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de Signed-off-by: Jaroslav Kysela perex@perex.cz Reviewed-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de
Here misses your Signed-off-by tag. Ditto for the patch 3. It's a legal requirement, and I can't apply patches without it.
i assumed reviewed-by would be a superset, but apparently it's not. however, there is no need to add SOB to patches which i only reviewed or suggested. i'll adjust the patches to which i made substantial modifications, though.
regards
On Fri, 05 May 2023 17:52:11 +0200, Oswald Buddenhagen wrote:
On Fri, May 05, 2023 at 05:22:57PM +0200, Takashi Iwai wrote:
On Fri, 05 May 2023 12:31:35 +0200, Oswald Buddenhagen wrote:
Suggested-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de Signed-off-by: Jaroslav Kysela perex@perex.cz Reviewed-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de
Here misses your Signed-off-by tag. Ditto for the patch 3. It's a legal requirement, and I can't apply patches without it.
i assumed reviewed-by would be a superset, but apparently it's not. however, there is no need to add SOB to patches which i only reviewed or suggested.
SOB is needed by a person who submitted, no matter whether any other tags are present. So, it's still mandatory in this case.
i'll adjust the patches to which i made substantial modifications, though.
Great, thanks!
Takashi
On Fri, May 05, 2023 at 06:22:15PM +0200, Takashi Iwai wrote:
On Fri, 05 May 2023 17:52:11 +0200, Oswald Buddenhagen wrote:
however, there is no need to add SOB to patches which i only reviewed or suggested.
SOB is needed by a person who submitted, no matter whether any other tags are present. So, it's still mandatory in this case.
pedantically, yes. you can link to perex' patch instead, as it's literally the same one, sans the reviewed-by.
(i don't think any of this really matters for re-posts of patches that have been publicly posted to the same list a few hours prior, as any legal challenge could be resolved within minutes anyway.)
regards
From: Jaroslav Kysela perex@perex.cz
Commit 9a826ddba6e ("[ALSA] pcm core: fix silence_start calculations") came with exactly the right commit message, but the patch just made things broken in a different way: We'd fill at a too low address if the area was already partially zeroed, so we'd under-fill. This affected both thresholded mode (where it was somewhat less likely) and top-up mode (where it would be the case consistently).
Suggested-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de Signed-off-by: Jaroslav Kysela perex@perex.cz Reviewed-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- v2: - rewrote commit message - postponed the addition of comments to a subsequent patch where it makes more sense - dropped the part which the subsequent refactoring obsoletes. this reduces the patch to its essentials, making it easier to understand, and reducing churn. --- sound/core/pcm_lib.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 6ad67e7e740c..5ddb74a12030 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -87,29 +87,25 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram 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; + runtime->silence_start = runtime->status->hw_ptr; } 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; } + runtime->silence_start = new_hw_ptr; } frames = runtime->buffer_size - runtime->silence_filled; } if (snd_BUG_ON(frames > runtime->buffer_size)) return; if (frames == 0) return; - ofs = runtime->silence_start % runtime->buffer_size; + ofs = (runtime->silence_start + runtime->silence_filled) % runtime->buffer_size; while (frames > 0) { transfer = ofs + frames > runtime->buffer_size ? runtime->buffer_size - ofs : frames; err = fill_silence_frames(substream, ofs, transfer);
From: Jaroslav Kysela perex@perex.cz
The removed condition handles de facto only one situation where runtime->silence_filled variable is equal to runtime->buffer_size, because this variable cannot go over the buffer size. This case is implicitly caught by the required comparison of the noise distance with the threshold.
Suggested-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de Signed-off-by: Jaroslav Kysela perex@perex.cz Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- v2: - improved language of commit message --- sound/core/pcm_lib.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 5ddb74a12030..a1838130c830 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -61,8 +61,6 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram runtime->silence_filled = 0; runtime->silence_start = appl_ptr; } - if (runtime->silence_filled >= runtime->buffer_size) - return; /* initialization outside pointer updates */ if (new_hw_ptr == ULONG_MAX) new_hw_ptr = runtime->status->hw_ptr;
From: Jaroslav Kysela perex@perex.cz
The code tracking the added samples in thresholded mode and the code tracking the just played samples in top-up mode are semantically identical, so factor it out to a common function to enhance readability.
Signed-off-by: Jaroslav Kysela perex@perex.cz Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- v2: - nicer function name and variable name - improved language of commit message --- sound/core/pcm_lib.c | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index a1838130c830..670572c9a8cc 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -33,6 +33,25 @@ static int fill_silence_frames(struct snd_pcm_substream *substream, snd_pcm_uframes_t off, snd_pcm_uframes_t frames);
+ +static inline void update_silence_vars(struct snd_pcm_runtime *runtime, + snd_pcm_uframes_t ptr, + snd_pcm_uframes_t new_ptr) +{ + snd_pcm_sframes_t delta; + + delta = new_ptr - ptr; + if (delta == 0) + return; + if (delta < 0) + delta += runtime->boundary; + if ((snd_pcm_uframes_t)delta < runtime->silence_filled) + runtime->silence_filled -= delta; + else + runtime->silence_filled = 0; + runtime->silence_start = new_ptr; +} + /* * fill ring buffer with silence * runtime->silence_start: starting pointer to silence area @@ -49,18 +68,9 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram int err;
if (runtime->silence_size < runtime->boundary) { - snd_pcm_sframes_t noise_dist, n; + snd_pcm_sframes_t noise_dist; 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; - } + update_silence_vars(runtime, runtime->silence_start, appl_ptr); /* initialization outside pointer updates */ if (new_hw_ptr == ULONG_MAX) new_hw_ptr = runtime->status->hw_ptr; @@ -87,15 +97,7 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram runtime->silence_filled = avail > 0 ? avail : 0; runtime->silence_start = runtime->status->hw_ptr; } 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; + update_silence_vars(runtime, runtime->status->hw_ptr, new_hw_ptr); } frames = runtime->buffer_size - runtime->silence_filled; }
Inline the remaining call of snd_pcm_playback_hw_avail(). This makes the top-up branch more congruent with the thresholded one, and allows simplifying the handling of the corner cases.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/core/pcm_lib.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 670572c9a8cc..17fc80a654be 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -90,15 +90,32 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram * This filling mode aims at free-running mode (used for example by dmix), * which doesn't update the application pointer. */ - 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; + snd_pcm_uframes_t hw_ptr = runtime->status->hw_ptr; + if (new_hw_ptr == ULONG_MAX) { + /* + * Initialization, fill the whole unused buffer with silence. + * + * Usually, this is entered while stopped, before data is queued, + * so both pointers are expected to be zero. + */ + snd_pcm_sframes_t avail = runtime->control->appl_ptr - hw_ptr; + if (avail < 0) + 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 = avail > runtime->buffer_size ? 0 : avail; + runtime->silence_start = hw_ptr; } else { - update_silence_vars(runtime, runtime->status->hw_ptr, new_hw_ptr); + /* Silence the just played area immediately */ + update_silence_vars(runtime, hw_ptr, new_hw_ptr); } + /* + * In this mode, silence_filled actually includes the valid + * sample data from the user. + */ frames = runtime->buffer_size - runtime->silence_filled; } if (snd_BUG_ON(frames > runtime->buffer_size))
We already know that `frames` is greater than zero, because we just checked it. So we don't need to check the loop condition on the first iteration.
Signed-off-by: Oswald Buddenhagen oswald.buddenhagen@gmx.de --- sound/core/pcm_lib.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 17fc80a654be..9c121a921b04 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -123,14 +123,14 @@ void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_ufram if (frames == 0) return; ofs = (runtime->silence_start + runtime->silence_filled) % runtime->buffer_size; - while (frames > 0) { + do { 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); }
participants (2)
-
Oswald Buddenhagen -
Takashi Iwai