[RFC] ALSA: pcm_lib - read/write IO improvement

[RFC] ALSA: pcm_lib - read/write IO improvement

[Jarkko Nikula]

Hi

I've noticed a problem with the ALSA read/write IO as it tries to pad
buffer as full/empty as possible after each wake-up. Then there is no
wake-up during the next period interrupt since avail < avail_min and
effectively there could be very short time left for wake-up on an
interrupt after that.

What I've seen on OMAP3 and Intel HDA that typically avail_max is only
a frame or two less than size of two periods. I.e. very near to xrun
when using a buffer with two periods.

I came up with two ideas. The first one won't do padding and the second
one which does the padding as currently but checks are there available
frames in the next period and does a wake-up if necessary (even if the
avail < avail_min).

With the first one, the avail_max is typically a few frames more than
the size of one period. With the second one it's a few frames less than
size of one period.

So by not doing the padding the distance to xrun is about the same and
the wake-up semantics remains the same. I understood from the thread
below that not all applications like if the wake-up happens before
avail >= avail_min what would happen with the second idea.

http://mailman.alsa-project.org/pipermail/alsa-devel/2009-December/023986.html


-- 
Jarkko

========================== RFC 1 ==========================
From: Jarkko Nikula <jhnikula@gmail.com>
Subject: [PATCH] ALSA: pcm_lib - Do not copy less than avail_min amount of frames in RW IO

Copying less than avail_min amount of frames after each wake-up, i.e. by
padding buffer as full/empty as possible by following the running HW pointer
can actually cause xrun to be more possible than without padding.

Padding causes that the avail is less than the avail_min when the next
period interrupt occurs and thus no wake-up at that time. However, period
interrupt following that one will see a situation where the avail is near
the size of two periods. I.e. there could be only few frames left before
xrun would happen if the buffer size is only two periods.

Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
---
 sound/core/pcm_lib.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index a2ff861..a47a010 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -1821,7 +1821,7 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
 		if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)
 			snd_pcm_update_hw_ptr(substream);
 		avail = snd_pcm_playback_avail(runtime);
-		if (!avail) {
+		if (avail < runtime->control->avail_min) {
 			if (nonblock) {
 				err = -EAGAIN;
 				goto _end_unlock;
@@ -2043,7 +2043,7 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
 		if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)
 			snd_pcm_update_hw_ptr(substream);
 		avail = snd_pcm_capture_avail(runtime);
-		if (!avail) {
+		if (avail < runtime->control->avail_min) {
 			if (runtime->status->state ==
 			    SNDRV_PCM_STATE_DRAINING) {
 				snd_pcm_stop(substream, SNDRV_PCM_STATE_SETUP);
-- 

========================== RFC 2 ==========================
From: Jarkko Nikula <jhnikula@gmail.com>
Subject: [PATCH] ALSA: pcm_lib - Do wake-up and copy if the next period has available frames

It is possible that the next period has only a few frames before xrun would
happen after next interrupt but wakeup does not happen now since the avail
is less than the avail_min threshold.

This patch adds a wake-up and copy check if the appl_ptr is less than end of
next period so that application can transfer data already now.

Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
---
 sound/core/pcm_lib.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index a2ff861..424873d 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -268,7 +268,7 @@ static void xrun_log_show(struct snd_pcm_substream *substream)
 int snd_pcm_update_state(struct snd_pcm_substream *substream,
 			 struct snd_pcm_runtime *runtime)
 {
-	snd_pcm_uframes_t avail;
+	snd_pcm_uframes_t avail, next_end;
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 		avail = snd_pcm_playback_avail(runtime);
@@ -287,7 +287,11 @@ int snd_pcm_update_state(struct snd_pcm_substream *substream,
 			return -EPIPE;
 		}
 	}
-	if (avail >= runtime->control->avail_min)
+	next_end = runtime->hw_ptr_interrupt + 2 * runtime->period_size - 1;
+	if (next_end >=runtime->boundary)
+		next_end -= runtime->boundary;
+	if (avail >= runtime->control->avail_min ||
+	    runtime->control->appl_ptr <= next_end)
 		wake_up(runtime->twake ? &runtime->tsleep : &runtime->sleep);
 	return 0;
 }
@@ -1707,7 +1711,7 @@ static int wait_for_avail_min(struct snd_pcm_substream *substream,
 	int is_playback = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
 	wait_queue_t wait;
 	int err = 0;
-	snd_pcm_uframes_t avail = 0;
+	snd_pcm_uframes_t avail = 0, next_end;
 	long tout;
 
 	init_waitqueue_entry(&wait, current);
@@ -1750,7 +1754,12 @@ static int wait_for_avail_min(struct snd_pcm_substream *substream,
 			avail = snd_pcm_playback_avail(runtime);
 		else
 			avail = snd_pcm_capture_avail(runtime);
-		if (avail >= runtime->control->avail_min)
+		next_end = runtime->hw_ptr_interrupt +
+			   2 * runtime->period_size - 1;
+		if (next_end >=runtime->boundary)
+			next_end -= runtime->boundary;
+		if (avail >= runtime->control->avail_min ||
+		    runtime->control->appl_ptr <= next_end)
 			break;
 	}
  _endloop:
-- 

Re: [RFC] ALSA: pcm_lib - read/write IO improvement

[Takashi Iwai]

At Tue, 13 Apr 2010 14:49:59 +0300,
Jarkko Nikula wrote:
> 
> Hi
> 
> I've noticed a problem with the ALSA read/write IO as it tries to pad
> buffer as full/empty as possible after each wake-up. Then there is no
> wake-up during the next period interrupt since avail < avail_min and
> effectively there could be very short time left for wake-up on an
> interrupt after that.
> 
> What I've seen on OMAP3 and Intel HDA that typically avail_max is only
> a frame or two less than size of two periods. I.e. very near to xrun
> when using a buffer with two periods.
> 
> I came up with two ideas. The first one won't do padding and the second
> one which does the padding as currently but checks are there available
> frames in the next period and does a wake-up if necessary (even if the
> avail < avail_min).
> 
> With the first one, the avail_max is typically a few frames more than
> the size of one period. With the second one it's a few frames less than
> size of one period.
> 
> So by not doing the padding the distance to xrun is about the same and
> the wake-up semantics remains the same. I understood from the thread
> below that not all applications like if the wake-up happens before
> avail >= avail_min what would happen with the second idea.
> 
> http://mailman.alsa-project.org/pipermail/alsa-devel/2009-December/023986.html

Well, my understanding is that it's an application thing.
It feeds the unaligned data while it expects the aligned wake-up.

Or, any other real example that doesn't work?


thanks,

Takashi


> -- 
> Jarkko
> 
> ========================== RFC 1 ==========================
> From: Jarkko Nikula <jhnikula@gmail.com>
> Subject: [PATCH] ALSA: pcm_lib - Do not copy less than avail_min amount of frames in RW IO
> 
> Copying less than avail_min amount of frames after each wake-up, i.e. by
> padding buffer as full/empty as possible by following the running HW pointer
> can actually cause xrun to be more possible than without padding.
> 
> Padding causes that the avail is less than the avail_min when the next
> period interrupt occurs and thus no wake-up at that time. However, period
> interrupt following that one will see a situation where the avail is near
> the size of two periods. I.e. there could be only few frames left before
> xrun would happen if the buffer size is only two periods.
> 
> Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
> ---
>  sound/core/pcm_lib.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index a2ff861..a47a010 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -1821,7 +1821,7 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
>  		if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)
>  			snd_pcm_update_hw_ptr(substream);
>  		avail = snd_pcm_playback_avail(runtime);
> -		if (!avail) {
> +		if (avail < runtime->control->avail_min) {
>  			if (nonblock) {
>  				err = -EAGAIN;
>  				goto _end_unlock;
> @@ -2043,7 +2043,7 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
>  		if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)
>  			snd_pcm_update_hw_ptr(substream);
>  		avail = snd_pcm_capture_avail(runtime);
> -		if (!avail) {
> +		if (avail < runtime->control->avail_min) {
>  			if (runtime->status->state ==
>  			    SNDRV_PCM_STATE_DRAINING) {
>  				snd_pcm_stop(substream, SNDRV_PCM_STATE_SETUP);
> -- 
> 
> ========================== RFC 2 ==========================
> From: Jarkko Nikula <jhnikula@gmail.com>
> Subject: [PATCH] ALSA: pcm_lib - Do wake-up and copy if the next period has available frames
> 
> It is possible that the next period has only a few frames before xrun would
> happen after next interrupt but wakeup does not happen now since the avail
> is less than the avail_min threshold.
> 
> This patch adds a wake-up and copy check if the appl_ptr is less than end of
> next period so that application can transfer data already now.
> 
> Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
> ---
>  sound/core/pcm_lib.c |   17 +++++++++++++----
>  1 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index a2ff861..424873d 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -268,7 +268,7 @@ static void xrun_log_show(struct snd_pcm_substream *substream)
>  int snd_pcm_update_state(struct snd_pcm_substream *substream,
>  			 struct snd_pcm_runtime *runtime)
>  {
> -	snd_pcm_uframes_t avail;
> +	snd_pcm_uframes_t avail, next_end;
>  
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>  		avail = snd_pcm_playback_avail(runtime);
> @@ -287,7 +287,11 @@ int snd_pcm_update_state(struct snd_pcm_substream *substream,
>  			return -EPIPE;
>  		}
>  	}
> -	if (avail >= runtime->control->avail_min)
> +	next_end = runtime->hw_ptr_interrupt + 2 * runtime->period_size - 1;
> +	if (next_end >=runtime->boundary)
> +		next_end -= runtime->boundary;
> +	if (avail >= runtime->control->avail_min ||
> +	    runtime->control->appl_ptr <= next_end)
>  		wake_up(runtime->twake ? &runtime->tsleep : &runtime->sleep);
>  	return 0;
>  }
> @@ -1707,7 +1711,7 @@ static int wait_for_avail_min(struct snd_pcm_substream *substream,
>  	int is_playback = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
>  	wait_queue_t wait;
>  	int err = 0;
> -	snd_pcm_uframes_t avail = 0;
> +	snd_pcm_uframes_t avail = 0, next_end;
>  	long tout;
>  
>  	init_waitqueue_entry(&wait, current);
> @@ -1750,7 +1754,12 @@ static int wait_for_avail_min(struct snd_pcm_substream *substream,
>  			avail = snd_pcm_playback_avail(runtime);
>  		else
>  			avail = snd_pcm_capture_avail(runtime);
> -		if (avail >= runtime->control->avail_min)
> +		next_end = runtime->hw_ptr_interrupt +
> +			   2 * runtime->period_size - 1;
> +		if (next_end >=runtime->boundary)
> +			next_end -= runtime->boundary;
> +		if (avail >= runtime->control->avail_min ||
> +		    runtime->control->appl_ptr <= next_end)
>  			break;
>  	}
>   _endloop:
> -- 
> 

Re: [RFC] ALSA: pcm_lib - read/write IO improvement

[Jarkko Nikula]

On Tue, 13 Apr 2010 19:01:35 +0200
Takashi Iwai <tiwai@suse.de> wrote:

> > So by not doing the padding the distance to xrun is about the same and
> > the wake-up semantics remains the same. I understood from the thread
> > below that not all applications like if the wake-up happens before
> > avail >= avail_min what would happen with the second idea.
> > 
> > http://mailman.alsa-project.org/pipermail/alsa-devel/2009-December/023986.html
> 
> Well, my understanding is that it's an application thing.
> It feeds the unaligned data while it expects the aligned wake-up.
> 
> Or, any other real example that doesn't work?
> 
Actually I noticed this by wondering why there are wake-ups happening
only for every second period even if there are only two periods. E.g.

aplay -D hw:0 -f dat --period-size=1024 --buffer-size=2048 /dev/zero

Then I noticed that this situation occurs if the hw_ptr has advanced
from the period boundary when checking for the avail in
snd_pcm_lib_write1 and snd_pcm_lib_read1 happens.

Let's image with the above command. First the both periods are filled up
and stream started.

Int 1
- appl_ptr = 2048
- hw_ptr >= 1024
- avail >= avail_min
- wake-up

Process running
- Filling up the period 1 
- Let the hw_ptr be 1026 in next iteration of snd_pcm_lib_write1
-> padding two frames into period 2

Int 2
- appl_ptr = 3074
- hw_ptr >= 2048
- wake-only if hw_ptr >= 2050
- Let the hw_ptr be 2049 -> no wake-up

Int 3
- appl_ptr = 3074
- hw_ptr >= 3072
- avail >= 2046
-> wake-up

Process running
- Padding rest of frames into period 2
- Filling up period 1
- Again if the hw_ptr has advanced, then one or more frames are padded
into period 2 and avail can be less than 1024 when the next interrupt
happens.

Now since the hw_ptr is always greater or equal in process running time
than wake-up time, it means that padding usually happens and most
usually the avail < avail_min when the next interrupt happens (unless
there is a long int latency at that time).

What I'm typically seeing that the avail_max is very near to buffer
size even under idle system. However, xruns do not occur very often
even if there are some busy loops running so the scheduler seems to do
good work by waking up us early enough :-)


-- 
Jarkko

Re: [RFC] ALSA: pcm_lib - read/write IO improvement

[Jaroslav Kysela]

On Wed, 14 Apr 2010, Jarkko Nikula wrote:

> On Tue, 13 Apr 2010 19:01:35 +0200
> Takashi Iwai <tiwai@suse.de> wrote:
>
>>> So by not doing the padding the distance to xrun is about the same and
>>> the wake-up semantics remains the same. I understood from the thread
>>> below that not all applications like if the wake-up happens before
>>> avail >= avail_min what would happen with the second idea.
>>>
>>> http://mailman.alsa-project.org/pipermail/alsa-devel/2009-December/023986.html
>>
>> Well, my understanding is that it's an application thing.
>> It feeds the unaligned data while it expects the aligned wake-up.
>>
>> Or, any other real example that doesn't work?
>>
> Actually I noticed this by wondering why there are wake-ups happening
> only for every second period even if there are only two periods. E.g.

It's quite nice jitter problem in the ring buffer position updates from 
the drivers. Thanks for the detailed explanation.

Your first proposal (change wake up condition) looks more appropriate to 
me, but I need to think more about this issue.

Another solution might be to add another position check using a workqueue 
or any timer source when the expectation of missed wake up is close.

 					Jaroslav

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