[PATCH 1/2] pcm: hw: reinterpret the drain_silence setting
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)
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;
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
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
participants (2)
-
Jaroslav Kysela -
Oswald Buddenhagen