[RFC] ALSA: Reduce delay for a blocking pcm_drain.

[RFC] ALSA: Reduce delay for a blocking pcm_drain.

[Dylan Reid]

This patch addresses two issues related to calling snd_pcm_drain.

If no_period_wakeup is set, then snd_pcm_drain would wait
MAX_SCHEDULE_TIMEOUT jiffies, without a wakeup pending this will leave
the calling task blocked indefinitely.  Instead block for double the
buffer size played back at a low sample rate (4k chosen arbitrarily).

Also if the stream is running with period wakeups but with a long
period, the delay could be seconds.  If only a small part of the
buffer is being used, this is unnecessary.  Instead wait for the
remaining samples to play out, plus one millisecond.  This allows
systems that fill the buffer only part way to still do a blocking
drain before closing to avoid losing samples.

Change-Id: I84ee1a52d9bdf80ea6065fb9533e565d3e6c8021
Signed-off-by: Dylan Reid <dgreid@chromium.org>
---

This can be worked around in user space as well by using a nonblocking drain and
sleeping based on how many samples are queued.  Would that be the right thing to
do or is this (or something similar) worth fixing here.

To test this I opened a pcm and set no period wakeups, played a few samples and
then called drain, which hung indefinitely.  Using a USB headset with a
max period size of 128k but only 15ms of the buffer filled, having the
drain time shorter without losing samples makes a blocking call
feasible.

---
 sound/core/pcm_native.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index f4aaf5a..a5f0695 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1481,7 +1481,7 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream,
 	}
 
 	for (;;) {
-		long tout;
+		long tout, tout_ms;
 		struct snd_pcm_runtime *to_check;
 		if (signal_pending(current)) {
 			result = -ERESTARTSYS;
@@ -1505,16 +1505,13 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream,
 		snd_pcm_stream_unlock_irq(substream);
 		up_read(&snd_pcm_link_rwsem);
 		snd_power_unlock(card);
-		if (runtime->no_period_wakeup)
-			tout = MAX_SCHEDULE_TIMEOUT;
-		else {
-			tout = 10;
-			if (runtime->rate) {
-				long t = runtime->period_size * 2 / runtime->rate;
-				tout = max(t, tout);
-			}
-			tout = msecs_to_jiffies(tout * 1000);
+		tout_ms = runtime->buffer_size * 2 * 1000 / 4000;
+		if (runtime->rate) {
+			snd_pcm_sframes_t hw_avail;
+			hw_avail = snd_pcm_playback_hw_avail(runtime);
+			tout_ms = 1 + hw_avail * 1000 / runtime->rate;
 		}
+		tout = msecs_to_jiffies(tout_ms);
 		tout = schedule_timeout_interruptible(tout);
 		snd_power_lock(card);
 		down_read(&snd_pcm_link_rwsem);
-- 
1.8.1.3.605.g02339dd

Re: [RFC] ALSA: Reduce delay for a blocking pcm_drain.

[Takashi Iwai]

At Fri, 27 Sep 2013 20:59:45 -0700,
Dylan Reid wrote:
> 
> This patch addresses two issues related to calling snd_pcm_drain.
> 
> If no_period_wakeup is set, then snd_pcm_drain would wait
> MAX_SCHEDULE_TIMEOUT jiffies, without a wakeup pending this will leave
> the calling task blocked indefinitely.  Instead block for double the
> buffer size played back at a low sample rate (4k chosen arbitrarily).
> 
> Also if the stream is running with period wakeups but with a long
> period, the delay could be seconds.  If only a small part of the
> buffer is being used, this is unnecessary.  Instead wait for the
> remaining samples to play out, plus one millisecond.  This allows
> systems that fill the buffer only part way to still do a blocking
> drain before closing to avoid losing samples.
> 
> Change-Id: I84ee1a52d9bdf80ea6065fb9533e565d3e6c8021
> Signed-off-by: Dylan Reid <dgreid@chromium.org>
> ---
> 
> This can be worked around in user space as well by using a nonblocking drain and
> sleeping based on how many samples are queued.  Would that be the right thing to
> do or is this (or something similar) worth fixing here.
> 
> To test this I opened a pcm and set no period wakeups, played a few samples and
> then called drain, which hung indefinitely.  Using a USB headset with a
> max period size of 128k but only 15ms of the buffer filled, having the
> drain time shorter without losing samples makes a blocking call
> feasible.

The no_period_wakeup option is a kind of "all your PCM timing control
are belong to us" policy.  So it's better to keep the kernel behavior
as simple as possible in that case, IMO.


thanks,

Takashi

> ---
>  sound/core/pcm_native.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index f4aaf5a..a5f0695 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -1481,7 +1481,7 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream,
>  	}
>  
>  	for (;;) {
> -		long tout;
> +		long tout, tout_ms;
>  		struct snd_pcm_runtime *to_check;
>  		if (signal_pending(current)) {
>  			result = -ERESTARTSYS;
> @@ -1505,16 +1505,13 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream,
>  		snd_pcm_stream_unlock_irq(substream);
>  		up_read(&snd_pcm_link_rwsem);
>  		snd_power_unlock(card);
> -		if (runtime->no_period_wakeup)
> -			tout = MAX_SCHEDULE_TIMEOUT;
> -		else {
> -			tout = 10;
> -			if (runtime->rate) {
> -				long t = runtime->period_size * 2 / runtime->rate;
> -				tout = max(t, tout);
> -			}
> -			tout = msecs_to_jiffies(tout * 1000);
> +		tout_ms = runtime->buffer_size * 2 * 1000 / 4000;
> +		if (runtime->rate) {
> +			snd_pcm_sframes_t hw_avail;
> +			hw_avail = snd_pcm_playback_hw_avail(runtime);
> +			tout_ms = 1 + hw_avail * 1000 / runtime->rate;
>  		}
> +		tout = msecs_to_jiffies(tout_ms);
>  		tout = schedule_timeout_interruptible(tout);
>  		snd_power_lock(card);
>  		down_read(&snd_pcm_link_rwsem);
> -- 
> 1.8.1.3.605.g02339dd
> 

Re: [RFC] ALSA: Reduce delay for a blocking pcm_drain.

[Clemens Ladisch]

Dylan Reid wrote:
> This patch addresses two issues related to calling snd_pcm_drain.
>
> If no_period_wakeup is set, then snd_pcm_drain would wait
> MAX_SCHEDULE_TIMEOUT jiffies, without a wakeup pending this will leave
> the calling task blocked indefinitely.

no_period_wakeup is used by applications that do _not_ want to use any
of ALSA's blocking functions (and thus want to avoid the interrupt
overhead) and use their own timers instead.  This also applies to
snd_pcm_drain.

Is there any actual application that tries to use snd_pcm_drain together
with no_period_wakeup?

> Also if the stream is running with period wakeups but with a long
> period, the delay could be seconds.  If only a small part of the
> buffer is being used, this is unnecessary.

Wakeup at period boundaries is part of the ALSA API.  (This is all what
periods are for.)

> Instead wait for the remaining samples to play out, plus one
> millisecond.

The device's clock and the Linux system clock might have larger
differences.  The only reliable synchronization source is the period
interrupt.


Regards,
Clemens

Re: [RFC] ALSA: Reduce delay for a blocking pcm_drain.

[Dylan Reid]

On Mon, Sep 30, 2013 at 1:45 AM, Clemens Ladisch <clemens@ladisch.de> wrote:
> Dylan Reid wrote:
>> This patch addresses two issues related to calling snd_pcm_drain.
>>
>> If no_period_wakeup is set, then snd_pcm_drain would wait
>> MAX_SCHEDULE_TIMEOUT jiffies, without a wakeup pending this will leave
>> the calling task blocked indefinitely.
>
> no_period_wakeup is used by applications that do _not_ want to use any
> of ALSA's blocking functions (and thus want to avoid the interrupt
> overhead) and use their own timers instead.  This also applies to
> snd_pcm_drain.
>
> Is there any actual application that tries to use snd_pcm_drain together
> with no_period_wakeup?

Not a real one.  I am using no_period_wakeup and looking for an easy
way to flush at the end.  User space already knows the buffer level,
so it can wait for the appropriate time and wait for an underrun just
as easily.  Thanks for the explanation all.

Dylan

Re: [RFC] ALSA: Reduce delay for a blocking pcm_drain.

[Raymond Yau]

>
> This can be worked around in user space as well by using a nonblocking
drain and
> sleeping based on how many samples are queued.  Would that be the right
thing to
> do or is this (or something similar) worth fixing here.
>
> To test this I opened a pcm and set no period wakeups, played a few
samples and
> then called drain, which hung indefinitely.  Using a USB headset with a
> max period size of 128k but only 15ms of the buffer filled, having the
> drain time shorter without losing samples makes a blocking call
> feasible.
>

Do you mean that you tested on a patched version of USB audio driver since
only three drivers support
SNDRV_PCM_INFO_NO_PERIOD_WAKEUP

http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/log/sound?qt=grep&q=period+wakeup

Re: [RFC] ALSA: Reduce delay for a blocking pcm_drain.

[Clemens Ladisch]

Raymond Yau wrote:
>> To test this I opened a pcm and set no period wakeups, played a few samples and
>> then called drain, which hung indefinitely.  Using a USB headset with a
>> max period size of 128k but only 15ms of the buffer filled, having the
>> drain time shorter without losing samples makes a blocking call
>> feasible.
>
> Do you mean that you tested on a patched version of USB audio driver since
> only three drivers support SNDRV_PCM_INFO_NO_PERIOD_WAKEUP

The flag is ignored if not supported.

In this test, the important part was the large period size.


Regards,
Clemens