* [PATCH 1/7] ALSA: pcm: Revert "ALSA: pcm: rewrite snd_pcm_playback_silence()"
@ 2023-05-05 10:31 Oswald Buddenhagen
2023-05-05 10:31 ` [PATCH 2/7] ALSA: pcm: fix playback silence - use the actual new_hw_ptr for the threshold mode Oswald Buddenhagen
` (5 more replies)
0 siblings, 6 replies; 11+ messages in thread
From: Oswald Buddenhagen @ 2023-05-05 10:31 UTC (permalink / raw)
To: alsa-devel; +Cc: Takashi Iwai, Jeff Chua
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+NKUSwAO7HpLAkYA@mail.gmail.com
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);
}
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/7] ALSA: pcm: fix playback silence - use the actual new_hw_ptr for the threshold mode
2023-05-05 10:31 [PATCH 1/7] ALSA: pcm: Revert "ALSA: pcm: rewrite snd_pcm_playback_silence()" Oswald Buddenhagen
@ 2023-05-05 10:31 ` Oswald Buddenhagen
2023-05-05 15:22 ` Takashi Iwai
2023-05-05 10:31 ` [PATCH 3/7] ALSA: pcm: fix playback silence - correct incremental silencing Oswald Buddenhagen
` (4 subsequent siblings)
5 siblings, 1 reply; 11+ messages in thread
From: Oswald Buddenhagen @ 2023-05-05 10:31 UTC (permalink / raw)
To: alsa-devel; +Cc: Takashi Iwai
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;
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/7] ALSA: pcm: fix playback silence - correct incremental silencing
2023-05-05 10:31 [PATCH 1/7] ALSA: pcm: Revert "ALSA: pcm: rewrite snd_pcm_playback_silence()" Oswald Buddenhagen
2023-05-05 10:31 ` [PATCH 2/7] ALSA: pcm: fix playback silence - use the actual new_hw_ptr for the threshold mode Oswald Buddenhagen
@ 2023-05-05 10:31 ` Oswald Buddenhagen
2023-05-05 10:31 ` [PATCH 4/7] ALSA: pcm: playback silence - remove extra code Oswald Buddenhagen
` (3 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Oswald Buddenhagen @ 2023-05-05 10:31 UTC (permalink / raw)
To: alsa-devel; +Cc: Takashi Iwai
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);
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 4/7] ALSA: pcm: playback silence - remove extra code
2023-05-05 10:31 [PATCH 1/7] ALSA: pcm: Revert "ALSA: pcm: rewrite snd_pcm_playback_silence()" Oswald Buddenhagen
2023-05-05 10:31 ` [PATCH 2/7] ALSA: pcm: fix playback silence - use the actual new_hw_ptr for the threshold mode Oswald Buddenhagen
2023-05-05 10:31 ` [PATCH 3/7] ALSA: pcm: fix playback silence - correct incremental silencing Oswald Buddenhagen
@ 2023-05-05 10:31 ` Oswald Buddenhagen
2023-05-05 10:31 ` [PATCH 5/7] ALSA: pcm: playback silence - move silence variable updates to separate function Oswald Buddenhagen
` (2 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Oswald Buddenhagen @ 2023-05-05 10:31 UTC (permalink / raw)
To: alsa-devel; +Cc: Takashi Iwai
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;
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 5/7] ALSA: pcm: playback silence - move silence variable updates to separate function
2023-05-05 10:31 [PATCH 1/7] ALSA: pcm: Revert "ALSA: pcm: rewrite snd_pcm_playback_silence()" Oswald Buddenhagen
` (2 preceding siblings ...)
2023-05-05 10:31 ` [PATCH 4/7] ALSA: pcm: playback silence - remove extra code Oswald Buddenhagen
@ 2023-05-05 10:31 ` Oswald Buddenhagen
2023-05-05 10:31 ` [PATCH 6/7] ALSA: pcm: simplify top-up mode init in snd_pcm_playback_silence() Oswald Buddenhagen
2023-05-05 10:31 ` [PATCH 7/7] ALSA: pcm: use exit controlled loop " Oswald Buddenhagen
5 siblings, 0 replies; 11+ messages in thread
From: Oswald Buddenhagen @ 2023-05-05 10:31 UTC (permalink / raw)
To: alsa-devel; +Cc: Takashi Iwai
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;
}
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 6/7] ALSA: pcm: simplify top-up mode init in snd_pcm_playback_silence()
2023-05-05 10:31 [PATCH 1/7] ALSA: pcm: Revert "ALSA: pcm: rewrite snd_pcm_playback_silence()" Oswald Buddenhagen
` (3 preceding siblings ...)
2023-05-05 10:31 ` [PATCH 5/7] ALSA: pcm: playback silence - move silence variable updates to separate function Oswald Buddenhagen
@ 2023-05-05 10:31 ` Oswald Buddenhagen
2023-05-05 10:31 ` [PATCH 7/7] ALSA: pcm: use exit controlled loop " Oswald Buddenhagen
5 siblings, 0 replies; 11+ messages in thread
From: Oswald Buddenhagen @ 2023-05-05 10:31 UTC (permalink / raw)
To: alsa-devel; +Cc: Takashi Iwai
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))
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 7/7] ALSA: pcm: use exit controlled loop in snd_pcm_playback_silence()
2023-05-05 10:31 [PATCH 1/7] ALSA: pcm: Revert "ALSA: pcm: rewrite snd_pcm_playback_silence()" Oswald Buddenhagen
` (4 preceding siblings ...)
2023-05-05 10:31 ` [PATCH 6/7] ALSA: pcm: simplify top-up mode init in snd_pcm_playback_silence() Oswald Buddenhagen
@ 2023-05-05 10:31 ` Oswald Buddenhagen
5 siblings, 0 replies; 11+ messages in thread
From: Oswald Buddenhagen @ 2023-05-05 10:31 UTC (permalink / raw)
To: alsa-devel; +Cc: Takashi Iwai
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);
}
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/7] ALSA: pcm: fix playback silence - use the actual new_hw_ptr for the threshold mode
2023-05-05 10:31 ` [PATCH 2/7] ALSA: pcm: fix playback silence - use the actual new_hw_ptr for the threshold mode Oswald Buddenhagen
@ 2023-05-05 15:22 ` Takashi Iwai
2023-05-05 15:52 ` Oswald Buddenhagen
0 siblings, 1 reply; 11+ messages in thread
From: Takashi Iwai @ 2023-05-05 15:22 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: alsa-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/7] ALSA: pcm: fix playback silence - use the actual new_hw_ptr for the threshold mode
2023-05-05 15:22 ` Takashi Iwai
@ 2023-05-05 15:52 ` Oswald Buddenhagen
2023-05-05 16:22 ` Takashi Iwai
0 siblings, 1 reply; 11+ messages in thread
From: Oswald Buddenhagen @ 2023-05-05 15:52 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/7] ALSA: pcm: fix playback silence - use the actual new_hw_ptr for the threshold mode
2023-05-05 15:52 ` Oswald Buddenhagen
@ 2023-05-05 16:22 ` Takashi Iwai
2023-05-05 16:32 ` Oswald Buddenhagen
0 siblings, 1 reply; 11+ messages in thread
From: Takashi Iwai @ 2023-05-05 16:22 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: alsa-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/7] ALSA: pcm: fix playback silence - use the actual new_hw_ptr for the threshold mode
2023-05-05 16:22 ` Takashi Iwai
@ 2023-05-05 16:32 ` Oswald Buddenhagen
0 siblings, 0 replies; 11+ messages in thread
From: Oswald Buddenhagen @ 2023-05-05 16:32 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-05-05 16:34 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-05 10:31 [PATCH 1/7] ALSA: pcm: Revert "ALSA: pcm: rewrite snd_pcm_playback_silence()" Oswald Buddenhagen
2023-05-05 10:31 ` [PATCH 2/7] ALSA: pcm: fix playback silence - use the actual new_hw_ptr for the threshold mode Oswald Buddenhagen
2023-05-05 15:22 ` Takashi Iwai
2023-05-05 15:52 ` Oswald Buddenhagen
2023-05-05 16:22 ` Takashi Iwai
2023-05-05 16:32 ` Oswald Buddenhagen
2023-05-05 10:31 ` [PATCH 3/7] ALSA: pcm: fix playback silence - correct incremental silencing Oswald Buddenhagen
2023-05-05 10:31 ` [PATCH 4/7] ALSA: pcm: playback silence - remove extra code Oswald Buddenhagen
2023-05-05 10:31 ` [PATCH 5/7] ALSA: pcm: playback silence - move silence variable updates to separate function Oswald Buddenhagen
2023-05-05 10:31 ` [PATCH 6/7] ALSA: pcm: simplify top-up mode init in snd_pcm_playback_silence() Oswald Buddenhagen
2023-05-05 10:31 ` [PATCH 7/7] ALSA: pcm: use exit controlled loop " Oswald Buddenhagen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.