[alsa-devel] [PATCH] ALSA: pcm_lib: avoid timing jitter in snd_pcm_read/write()
When using poll() to wait for the next period -- or avail_min samples -- one gets a consistent delay for each system call that is usually just a little short of the selected period time. However, When using snd_pcm_read/write(), one gets a jittery delay that alternates between less than a millisecond and approximately two period times. This is caused by snd_pcm_lib_{read,write}1() transferring any available samples to the user's buffer and adjusting the application pointer prior to sleeping to the end of the current period. When the next period interrupt occurs, there is then less than avail_min samples remaining to be transferred in the period, so we end up sleeping until a second period occurs.
This is solved by using runtime->twake as the number of samples needed for a wakeup in addition to selecting the proper wait queue to wake in snd_pcm_update_state(). This requires twake to be non-zero when used by snd_pcm_lib_{read,write}1() even if avail_min is zero.
Signed-off-by: Dave Dillow dave@thedillows.org --- Hans, you may want this one as well for the SIS7019, as it gives more margin to deal with captured data on alternating calls into the stack.
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index dd76cde..83c6fa6 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -313,7 +313,7 @@ struct snd_pcm_runtime { struct snd_pcm_mmap_control *control;
/* -- locking / scheduling -- */ - unsigned int twake: 1; /* do transfer (!poll) wakeup */ + snd_pcm_uframes_t twake; /* do transfer (!poll) wakeup if non-zero */ wait_queue_head_t sleep; /* poll sleep */ wait_queue_head_t tsleep; /* transfer sleep */ struct fasync_struct *fasync; diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index e9d98be..bcf95d3 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -287,8 +287,11 @@ int snd_pcm_update_state(struct snd_pcm_substream *substream, return -EPIPE; } } - if (avail >= runtime->control->avail_min) - wake_up(runtime->twake ? &runtime->tsleep : &runtime->sleep); + if (runtime->twake) { + if (avail >= runtime->twake) + wake_up(&runtime->tsleep); + } else if (avail >= runtime->control->avail_min) + wake_up(&runtime->sleep); return 0; }
@@ -1707,7 +1710,7 @@ EXPORT_SYMBOL(snd_pcm_period_elapsed); * The available space is stored on availp. When err = 0 and avail = 0 * on the capture stream, it indicates the stream is in DRAINING state. */ -static int wait_for_avail_min(struct snd_pcm_substream *substream, +static int wait_for_avail(struct snd_pcm_substream *substream, snd_pcm_uframes_t *availp) { struct snd_pcm_runtime *runtime = substream->runtime; @@ -1757,7 +1760,7 @@ 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) + if (avail >= runtime->twake) break; } _endloop: @@ -1820,7 +1823,7 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream, goto _end_unlock; }
- runtime->twake = 1; + runtime->twake = runtime->control->avail_min ? : 1; while (size > 0) { snd_pcm_uframes_t frames, appl_ptr, appl_ofs; snd_pcm_uframes_t avail; @@ -1833,7 +1836,9 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream, err = -EAGAIN; goto _end_unlock; } - err = wait_for_avail_min(substream, &avail); + runtime->twake = min_t(snd_pcm_uframes_t, size, + runtime->control->avail_min ? : 1); + err = wait_for_avail(substream, &avail); if (err < 0) goto _end_unlock; } @@ -2042,7 +2047,7 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream, goto _end_unlock; }
- runtime->twake = 1; + runtime->twake = runtime->control->avail_min ? : 1; while (size > 0) { snd_pcm_uframes_t frames, appl_ptr, appl_ofs; snd_pcm_uframes_t avail; @@ -2060,7 +2065,9 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream, err = -EAGAIN; goto _end_unlock; } - err = wait_for_avail_min(substream, &avail); + runtime->twake = min_t(snd_pcm_uframes_t, size, + runtime->control->avail_min ? : 1); + err = wait_for_avail(substream, &avail); if (err < 0) goto _end_unlock; if (!avail)
On Sun, Jun 27, 2010 at 7:13 AM, David Dillow dave@thedillows.org wrote:
When using poll() to wait for the next period -- or avail_min samples -- one gets a consistent delay for each system call that is usually just a little short of the selected period time. However, When using snd_pcm_read/write(), one gets a jittery delay that alternates between less than a millisecond and approximately two period times. This is caused by snd_pcm_lib_{read,write}1() transferring any available samples to the user's buffer and adjusting the application pointer prior to sleeping to the end of the current period. When the next period interrupt occurs, there is then less than avail_min samples remaining to be transferred in the period, so we end up sleeping until a second period occurs.
This is solved by using runtime->twake as the number of samples needed for a wakeup in addition to selecting the proper wait queue to wake in snd_pcm_update_state(). This requires twake to be non-zero when used by snd_pcm_lib_{read,write}1() even if avail_min is zero.
http://mailman.alsa-project.org/pipermail/alsa-devel/2010-June/028786.html
On Sun, 2010-06-27 at 12:29 +0900, Jassi Brar wrote:
On Sun, Jun 27, 2010 at 7:13 AM, David Dillow dave@thedillows.org wrote:
This is solved by using runtime->twake as the number of samples needed for a wakeup in addition to selecting the proper wait queue to wake in snd_pcm_update_state(). This requires twake to be non-zero when used by snd_pcm_lib_{read,write}1() even if avail_min is zero.
http://mailman.alsa-project.org/pipermail/alsa-devel/2010-June/028786.html
Hmm, yes, I should have search the archives a bit. I originally tuned out the thread you listed as a request for parameter help, and missed your original postings.
I think my patch is pretty close to preserving the existing semantics as it doesn't change poll() at all, but I do see a case where the user could get a read/write back prior to avail_min samples being ready. I think that's fixable -- if the user is requesting a read/write of less than avail_min samples, then we have to wait for avail_min regardless.
Takashi, is your concern about semantics the proper honoring of avail_min in all cases, or preserving the current behavior of waiting for two periods when avail_min is set to the size of one period?
Dave
On Sun, 27 Jun 2010, David Dillow wrote:
On Sun, 2010-06-27 at 12:29 +0900, Jassi Brar wrote:
On Sun, Jun 27, 2010 at 7:13 AM, David Dillow dave@thedillows.org wrote:
This is solved by using runtime->twake as the number of samples needed for a wakeup in addition to selecting the proper wait queue to wake in snd_pcm_update_state(). This requires twake to be non-zero when used by snd_pcm_lib_{read,write}1() even if avail_min is zero.
http://mailman.alsa-project.org/pipermail/alsa-devel/2010-June/028786.html
Hmm, yes, I should have search the archives a bit. I originally tuned out the thread you listed as a request for parameter help, and missed your original postings.
I think my patch is pretty close to preserving the existing semantics as it doesn't change poll() at all, but I do see a case where the user could get a read/write back prior to avail_min samples being ready. I think that's fixable -- if the user is requesting a read/write of less than avail_min samples, then we have to wait for avail_min regardless.
Takashi, is your concern about semantics the proper honoring of avail_min in all cases, or preserving the current behavior of waiting for two periods when avail_min is set to the size of one period?
I think that the avail_min semantics is quite clear. The problem is caused by the hw transfer acknowledge interrupt jitter. It seems that the only good solution is to postpone the wake_up() call to time when the avail_min condition becomes true - using an extra timing source (system timer for example).
Another possibility is to keep thing asis and keep to applications to handle this situation - use a different timing source than interrupts from soundcard for scheduling of I/O operations. But it's right that most of simple applications and use cases expect that I/O transfers will work even with 2 periods.
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
On Sun, 2010-06-27 at 20:01 +0200, Jaroslav Kysela wrote:
On Sun, 27 Jun 2010, David Dillow wrote:
On Sun, 2010-06-27 at 12:29 +0900, Jassi Brar wrote:
On Sun, Jun 27, 2010 at 7:13 AM, David Dillow dave@thedillows.org wrote:
This is solved by using runtime->twake as the number of samples needed for a wakeup in addition to selecting the proper wait queue to wake in snd_pcm_update_state(). This requires twake to be non-zero when used by snd_pcm_lib_{read,write}1() even if avail_min is zero.
http://mailman.alsa-project.org/pipermail/alsa-devel/2010-June/028786.html
Hmm, yes, I should have search the archives a bit. I originally tuned out the thread you listed as a request for parameter help, and missed your original postings.
I think my patch is pretty close to preserving the existing semantics as it doesn't change poll() at all, but I do see a case where the user could get a read/write back prior to avail_min samples being ready. I think that's fixable -- if the user is requesting a read/write of less than avail_min samples, then we have to wait for avail_min regardless.
Takashi, is your concern about semantics the proper honoring of avail_min in all cases, or preserving the current behavior of waiting for two periods when avail_min is set to the size of one period?
I think that the avail_min semantics is quite clear. The problem is caused by the hw transfer acknowledge interrupt jitter. It seems that the only good solution is to postpone the wake_up() call to time when the avail_min condition becomes true - using an extra timing source (system timer for example).
I think the avail_min is quite clear for poll() usage -- in that case, we get the expected behavior for wake ups. I believe the semantics are currently less well defined for read/write -- is the guarantee that at the end of the write call, there will be room for avail_min samples in the buffer, or is it guaranteed that there will be space for (avail_min - (size % avail_min)) samples, where size is the number of samples in the read/write call?
The current code gives no guarantee either way if the size of the call fits in the buffer. And there are cases where it doesn't give a guarantee that another write of avail_min will fit.
The only documentation a quick Google search turns up is the ALSA HOWTO, which only discusses poll().
Another possibility is to keep thing asis and keep to applications to handle this situation - use a different timing source than interrupts from soundcard for scheduling of I/O operations. But it's right that most of simple applications and use cases expect that I/O transfers will work even with 2 periods.
Indeed, that is how I noticed this issue -- capture running with 2 periods was causing unexpected overruns. As a user, I expected a read of avail_min (== period_size) samples to return when the period was up, not after a second period had elapsed.
Things can be easily changed so that case works as expected, the hard part is clarifying/defining the semantics for other cases, and ensuring those guarantees are met.
Perhaps documentation that recommends setting avail_min to 1 when using read/write to avoid this issue would be helpful, in lieu of changing the ALSA code or giving concrete guarantees.
Dave
2010/6/28 David Dillow dave@thedillows.org
Perhaps documentation that recommends setting avail_min to 1 when using read/write to avoid this issue would be helpful, in lieu of changing the ALSA code or giving concrete guarantees.
Dave
Refer to http://article.gmane.org/gmane.linux.alsa.devel/69333/match=avail_min
avail_min cannot be less than period size , however set_avail_min() did not return any error when the value is less than period size and this mislead the programmers to believe that they can set avail_min to 1
if you want low latency, you need to use small period size
Raymond, I've asked you before to not trim cc lists. Believe it or not, people may be involved in a conversation even if they are not on the mailing list. In this case, Hans probably doesn't care, but you cannot make that assumption.
On Mon, 2010-06-28 at 08:37 +0800, Raymond Yau wrote:
2010/6/28 David Dillow dave@thedillows.org
Perhaps documentation that recommends setting avail_min to 1 when using read/write to avoid this issue would be helpful, in lieu of changing the ALSA code or giving concrete guarantees.
Dave
Refer to http://article.gmane.org/gmane.linux.alsa.devel/69333/match=avail_min
avail_min cannot be less than period size , however set_avail_min() did not return any error when the value is less than period size and this mislead the programmers to believe that they can set avail_min to 1
They used to be able to, and it would work. You likely cannot get latency that low, but anyone that was expecting for that to magically change the way hardware works is fooling themselves. avail_min is like the timeout argument to poll() -- you're guaranteed at least that value, but you may get something much larger.
I understand the rationale given for patch b0b7d0 to alsa-lib, but limiting avail_min in this way removed a workaround for the muddled snd_pcm_read/write() semantics. I suppose it is still possible, if someone wants to color outside libasound -- the kernel still allows it.
if you want low latency, you need to use small period size
Small period sizes are often needed for low latency -- 10ms is common for VoIP -- but if you are stuck with a device that can only do two periods per buffer, the current read/write code will cause unstable audio with frequent overruns. You can add another round trip to the kernel with snd_pcm_wait(), but that's suboptimal and should not be required.
You can get reasonably good stability on limited devices with avail_min == 1, or with a read/write that doesn't force you to wait two periods when trying to work with one.
Dave
2010/6/28 David Dillow dave@thedillows.org
On Mon, 2010-06-28 at 08:37 +0800, Raymond Yau wrote:
2010/6/28 David Dillow dave@thedillows.org
Perhaps documentation that recommends setting avail_min to 1 when using read/write to avoid this issue would be helpful, in lieu of changing
the
ALSA code or giving concrete guarantees.
Dave
Refer to http://article.gmane.org/gmane.linux.alsa.devel/69333/match=avail_min
avail_min cannot be less than period size , however set_avail_min() did
not
return any error when the value is less than period size and this mislead the programmers to believe that they can set avail_min to 1
They used to be able to, and it would work. You likely cannot get latency that low, but anyone that was expecting for that to magically change the way hardware works is fooling themselves. avail_min is like the timeout argument to poll() -- you're guaranteed at least that value, but you may get something much larger.
I understand the rationale given for patch b0b7d0 to alsa-lib, but limiting avail_min in this way removed a workaround for the muddled snd_pcm_read/write() semantics. I suppose it is still possible, if someone wants to color outside libasound -- the kernel still allows it.
if you want low latency, you need to use small period size
Small period sizes are often needed for low latency -- 10ms is common for VoIP -- but if you are stuck with a device that can only do two periods per buffer, the current read/write code will cause unstable audio with frequent overruns. You can add another round trip to the kernel with snd_pcm_wait(), but that's suboptimal and should not be required.
You can get reasonably good stability on limited devices with avail_min == 1, or with a read/write that doesn't force you to wait two periods when trying to work with one.
Dave
The audio data record at the sample rate by ac97codec through ac97link to ac97 controller
e.g. cs46xx use snd_pcm_indirect_capture_transfer()
it is unlikely to set avail_min or minimum period size to a value less than the PCI brust size if your driver are using DMA transfer
you cannot set avail_min to 1 unless your period_size is 1 because
if (val < pcm->period_size) val = pcm->period_size;
http://git.alsa-project.org/?p=alsa-lib.git;a=commitdiff;h=b0b7d0280f977dee1...
int snd_pcm_sw_params_set_avail_min(snd_pcm_t *pcm, snd_pcm_sw_params_t *params, snd_pcm_uframes_t val)
#endif { assert(pcm && params); /* Fix avail_min if it's below period size. The period_size * defines the minimal wake-up timing accuracy, so it doesn't * make sense to set below that. */ if (val < pcm->period_size) val = pcm->period_size; params->avail_min = val; return 0; }
On Mon, 2010-06-28 at 11:13 +0800, Raymond Yau wrote:
2010/6/28 David Dillow dave@thedillows.org
On Mon, 2010-06-28 at 08:37 +0800, Raymond Yau wrote:
2010/6/28 David Dillow dave@thedillows.org
Perhaps documentation that recommends setting avail_min to 1 when using read/write to avoid this issue would be helpful, in lieu of changing the ALSA code or giving concrete guarantees.
Refer to
http://article.gmane.org/gmane.linux.alsa.devel/69333/match=avail_min
avail_min cannot be less than period size , however set_avail_min() di not return any error when the value is less than period size and this mislead the programmers to believe that they can set avail_min to 1
They used to be able to, and it would work. You likely cannot get latency that low, but anyone that was expecting for that to magically change the way hardware works is fooling themselves. avail_min is like the timeout argument to poll() -- you're guaranteed at least that value, but you may get something much larger.
I understand the rationale given for patch b0b7d0 to alsa-lib, but limiting avail_min in this way removed a workaround for the muddled snd_pcm_read/write() semantics. I suppose it is still possible, if someone wants to color outside libasound -- the kernel still allows it.
it is unlikely to set avail_min or minimum period size to a value less than the PCI brust size if your driver are using DMA transfer
Raymond, you seem to have missed the part where I said that setting avail_min to 1 is not going to magically change the way hardware works. Of course it will not get you wakeup latencies lower than the size of the DMA transfer; heck, it won't get you wake up latencies lower than a period size, should read/write have to sleep!
However, setting it to 1 was useful in that it said to wake up as soon as possible. Possible in this case means as soon as the next period has elapsed, which may be quite a bit longer than 1 sample. It is quite like the example I gave of poll() -- giving a timeout of 1ms to poll() on a machine with HZ=100 used to mean that you get at least a 10ms timeout. Even with NOHZ, HPET timers. and HZ=1000 today, you have no guarantee that your process is at the head of the run queue. You may not even get scheduled to run for several seconds on a busy system.
On Sat, 26 Jun 2010, David Dillow wrote:
When using poll() to wait for the next period -- or avail_min samples -- one gets a consistent delay for each system call that is usually just a little short of the selected period time. However, When using snd_pcm_read/write(), one gets a jittery delay that alternates between less than a millisecond and approximately two period times. This is caused by snd_pcm_lib_{read,write}1() transferring any available samples to the user's buffer and adjusting the application pointer prior to sleeping to the end of the current period. When the next period interrupt occurs, there is then less than avail_min samples remaining to be transferred in the period, so we end up sleeping until a second period occurs.
This is solved by using runtime->twake as the number of samples needed for a wakeup in addition to selecting the proper wait queue to wake in snd_pcm_update_state(). This requires twake to be non-zero when used by snd_pcm_lib_{read,write}1() even if avail_min is zero.
Signed-off-by: Dave Dillow dave@thedillows.org
Thanks. It looks like another way to get things right. I applied your patch to my tree.
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
On Mon, 2010-06-28 at 09:47 +0200, Jaroslav Kysela wrote:
On Sat, 26 Jun 2010, David Dillow wrote:
When using poll() to wait for the next period -- or avail_min samples -- one gets a consistent delay for each system call that is usually just a little short of the selected period time. However, When using snd_pcm_read/write(), one gets a jittery delay that alternates between less than a millisecond and approximately two period times. This is caused by snd_pcm_lib_{read,write}1() transferring any available samples to the user's buffer and adjusting the application pointer prior to sleeping to the end of the current period. When the next period interrupt occurs, there is then less than avail_min samples remaining to be transferred in the period, so we end up sleeping until a second period occurs.
This is solved by using runtime->twake as the number of samples needed for a wakeup in addition to selecting the proper wait queue to wake in snd_pcm_update_state(). This requires twake to be non-zero when used by snd_pcm_lib_{read,write}1() even if avail_min is zero.
Signed-off-by: Dave Dillow dave@thedillows.org
Thanks. It looks like another way to get things right. I applied your patch to my tree.
I think we should probably better define the semantics of read/write for sizes < avail_min, == avail_min, and > avail_min. As it is, I fear that this patch makes things worse as I hadn't thought through all of the cases when I posted it. If you've not pushed, please revert. If you have, then it improves one case, but let's makes sure we get the others correct before it hits the mainline.
Dave
On Mon, 28 Jun 2010, David Dillow wrote:
On Mon, 2010-06-28 at 09:47 +0200, Jaroslav Kysela wrote:
On Sat, 26 Jun 2010, David Dillow wrote:
When using poll() to wait for the next period -- or avail_min samples -- one gets a consistent delay for each system call that is usually just a little short of the selected period time. However, When using snd_pcm_read/write(), one gets a jittery delay that alternates between less than a millisecond and approximately two period times. This is caused by snd_pcm_lib_{read,write}1() transferring any available samples to the user's buffer and adjusting the application pointer prior to sleeping to the end of the current period. When the next period interrupt occurs, there is then less than avail_min samples remaining to be transferred in the period, so we end up sleeping until a second period occurs.
This is solved by using runtime->twake as the number of samples needed for a wakeup in addition to selecting the proper wait queue to wake in snd_pcm_update_state(). This requires twake to be non-zero when used by snd_pcm_lib_{read,write}1() even if avail_min is zero.
Signed-off-by: Dave Dillow dave@thedillows.org
Thanks. It looks like another way to get things right. I applied your patch to my tree.
I think we should probably better define the semantics of read/write for sizes < avail_min, == avail_min, and > avail_min. As it is, I fear that this patch makes things worse as I hadn't thought through all of the cases when I posted it. If you've not pushed, please revert. If you have, then it improves one case, but let's makes sure we get the others correct before it hits the mainline.
I don't see any drawbacks in your solution. When I/O size is greater than avail_min, the behaviour is same as in previous code (avail_min is used for wake_up). For smaller I/O sizes (requests are not aligned to period size), each period interrupt will cause a wake up for the application task. In this case, the application may do own optimization of I/O transfers (sizes).
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
At Mon, 28 Jun 2010 16:11:21 +0200 (CEST), Jaroslav Kysela wrote:
On Mon, 28 Jun 2010, David Dillow wrote:
On Mon, 2010-06-28 at 09:47 +0200, Jaroslav Kysela wrote:
On Sat, 26 Jun 2010, David Dillow wrote:
When using poll() to wait for the next period -- or avail_min samples -- one gets a consistent delay for each system call that is usually just a little short of the selected period time. However, When using snd_pcm_read/write(), one gets a jittery delay that alternates between less than a millisecond and approximately two period times. This is caused by snd_pcm_lib_{read,write}1() transferring any available samples to the user's buffer and adjusting the application pointer prior to sleeping to the end of the current period. When the next period interrupt occurs, there is then less than avail_min samples remaining to be transferred in the period, so we end up sleeping until a second period occurs.
This is solved by using runtime->twake as the number of samples needed for a wakeup in addition to selecting the proper wait queue to wake in snd_pcm_update_state(). This requires twake to be non-zero when used by snd_pcm_lib_{read,write}1() even if avail_min is zero.
Signed-off-by: Dave Dillow dave@thedillows.org
Thanks. It looks like another way to get things right. I applied your patch to my tree.
I think we should probably better define the semantics of read/write for sizes < avail_min, == avail_min, and > avail_min. As it is, I fear that this patch makes things worse as I hadn't thought through all of the cases when I posted it. If you've not pushed, please revert. If you have, then it improves one case, but let's makes sure we get the others correct before it hits the mainline.
I don't see any drawbacks in your solution. When I/O size is greater than avail_min, the behaviour is same as in previous code (avail_min is used for wake_up). For smaller I/O sizes (requests are not aligned to period size), each period interrupt will cause a wake up for the application task. In this case, the application may do own optimization of I/O transfers (sizes).
I pulled to my tree now, too. (sorry for the late response, as I've been on vacation.)
It looks good to me. Let's see whether it breaks something ;)
thanks,
Takashi
participants (5)
-
David Dillow -
Jaroslav Kysela -
Jassi Brar -
Raymond Yau -
Takashi Iwai