[PATCH 1/2] pcm: hw: reinterpret the drain_silence setting

[PATCH 1/2] pcm: hw: reinterpret the drain_silence setting

[Oswald Buddenhagen]

It makes no sense to have a config option which determines the entire
silencing time, because useful values strongly depend on the period size
runtime setting.

So instead we interpret the setting as an override for the "overhang"
which is used to compensate FIFOs and IRQ delays. The reasonable worst
case of this is determined by the hardware, so it makes sense to have
this as a config option.

In a next step, we may want to derive the default value from the
declared FIFO size (whiche few drivers do) and the minimal period size
(which is generally linked to the FIFO size).

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 src/pcm/pcm_hw.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index 8ffebed9..ecc47a76 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -737,18 +737,19 @@ static int snd_pcm_hw_drain(snd_pcm_t *pcm)
 {
 	snd_pcm_hw_t *hw = pcm->private_data;
 	snd_pcm_sw_params_t sw_params;
+	snd_pcm_uframes_t silence_slack;
 	snd_pcm_uframes_t silence_size;
 	int err;
 
 	if (pcm->stream != SND_PCM_STREAM_PLAYBACK)
 		goto __skip_silence;
 	if (hw->drain_silence == 0 || hw->perfect_drain)
 		goto __skip_silence;
 	snd_pcm_sw_params_current_no_lock(pcm, &sw_params);
-	if (hw->drain_silence > 0) {
-		silence_size = (pcm->rate * hw->drain_silence) / 1000;
-		goto __manual_silence;
-	}
+	if (hw->drain_silence > 0)
+		silence_slack = (pcm->rate * hw->drain_silence) / 1000;
+	else
+		silence_slack = pcm->rate / 10;	/* 1/10th of second */
 	/* compute end silence size, align to period size + extra time */
 	if ((pcm->boundary % pcm->period_size) == 0) {
 		silence_size = pcm->period_size - (*pcm->appl.ptr % pcm->period_size);
@@ -761,8 +762,7 @@ static int snd_pcm_hw_drain(snd_pcm_t *pcm)
 		 */
 		silence_size = pcm->period_size;
 	}
-	silence_size += pcm->rate / 10;	/* 1/10th of second */
-__manual_silence:
+	silence_size += silence_slack;
 	if (sw_params.silence_size < silence_size) {
 		/* fill the silence soon as possible (in the bellow ioctl
 		 * or the next period wake up)
-- 
2.40.0.152.g15d061e6df

[PATCH 2/2] pcm: hw: fix excessive silence fill on drain

[Oswald Buddenhagen]

Setting the threshold to the size of the buffer results in additional
filling each time a period elapses. As draining is usually initiated
with a rather full buffer, this would usually result in filling way in
excess of what was intended.

A sufficient threshold is the required worst-case fill, that is, one
period size plus the "overhang".

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

---

This patch is entirely untested! But it's derived from my previously
posted v3 kernel patch, which was successfully tested.
---
 src/pcm/pcm_hw.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index ecc47a76..a5f87215 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -764,12 +764,11 @@ static int snd_pcm_hw_drain(snd_pcm_t *pcm)
 	}
 	silence_size += silence_slack;
 	if (sw_params.silence_size < silence_size) {
-		/* fill the silence soon as possible (in the bellow ioctl
-		 * or the next period wake up)
-		 */
-		sw_params.silence_threshold = pcm->buffer_size;
-		if (silence_size > pcm->buffer_size)
-			silence_size = pcm->buffer_size;
+		sw_params.silence_threshold = pcm->period_size + silence_slack;
+		if (sw_params.silence_threshold > pcm->buffer_size)
+			sw_params.silence_threshold = pcm->buffer_size;
+		if (silence_size > sw_params.silence_threshold)
+			silence_size = sw_params.silence_threshold;
 		sw_params.silence_size = silence_size;
 		if (ioctl(hw->fd, SNDRV_PCM_IOCTL_SW_PARAMS, &sw_params) < 0) {
 			err = -errno;
-- 
2.40.0.152.g15d061e6df

Re: [PATCH 1/2] pcm: hw: reinterpret the drain_silence setting

[Jaroslav Kysela]

On 15. 05. 23 10:41, Oswald Buddenhagen wrote:
> It makes no sense to have a config option which determines the entire
> silencing time, because useful values strongly depend on the period size
> runtime setting.

I would prefer a drain silence mode configuration field for this, if the 
interpretation of the positive value is different. The current value is easy 
to understand and it's really a corner case to not use the default (auto) 
code. Even the period time may be forced in the configuration, so users can 
compute the right value themselves.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH 1/2] pcm: hw: reinterpret the drain_silence setting

[Oswald Buddenhagen]

On Mon, May 15, 2023 at 11:06:54AM +0200, Jaroslav Kysela wrote:
>On 15. 05. 23 10:41, Oswald Buddenhagen wrote:
>> It makes no sense to have a config option which determines the entire
>> silencing time, because useful values strongly depend on the period size
>> runtime setting.
>
>I would prefer a drain silence mode configuration field for this, if the 
>interpretation of the positive value is different. The current value is easy 
>to understand and it's really a corner case to not use the default (auto) 
>code.
>
but why would you *want* that? what would one use it for? what is the 
advantage over my proposal?
i don't think "easier to understand" is true. it may seem easier on the 
surface, but one cannot make an informed decision without actually 
understanding the implications.

>Even the period time may be forced in the configuration, so users can 
>compute the right value themselves.
>
that means that only by combining the two options you'd actually get 
something that makes sense.
my proposal otoh works both with and without a forced period time.

regards