| Message ID | 20230515084106.3447657-1-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] pcm: hw: reinterpret the drain_silence setting | expand |
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
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)
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(-)