Re: [alsa-devel] 2.6.30: xruns caused by "ALSA: pcm - Safer boundary checks"
At Tue, 26 May 2009 13:16:49 -0400, Robert Krakora wrote:
On Tue, May 26, 2009 at 11:41 AM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 26 May 2009 10:28:26 -0400, Robert Krakora wrote:
On Tue, May 26, 2009 at 4:03 AM, Takashi Iwai tiwai@suse.de wrote:
At Sun, 24 May 2009 23:56:40 -0700, Nathan Grennan wrote:
Takashi Iwai wrote: > Testing the patch would be helpful :)
Will do
Did you get any positive / negative result?
We are in the late rc stage, so a fix should be merged ASAP...
thanks,
Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Takashi:
I cannot speak for the other person experiencing this problem. However, neither your snapshot or Jarloslav's patch corrected the problem which I am experiencing.
So, did you get the same kernel messages when xrun_debug is set?
I have not looked at the code much in pcm_lib.c or pcm_native.c but it seems as though you are trying to manage FIFO overflow and underflow scenarios. Is my assumption correct? ;-)
No. The problem appears like an invalid check of jiffies in snd_pcm_hw_ptr_update*(). After the pause, jiffies isn't recorded properly, thus it gets screwed up.
To be sure, could you try the patch below? This will disable the jiffies check completely. If the problem still happens, then it's somewhere else where we need to check.
thanks,
Takashi
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index a2a792c..47837a6 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -249,6 +249,7 @@ static int snd_pcm_update_hw_ptr_interrupt(struct snd_pcm_substream *substream) new_hw_ptr = hw_base + pos; } } +#if 0 /* no jiffies check */ /* Skip the jiffies check for hardwares with BATCH flag. * Such hardware usually just increases the position at each IRQ, * thus it can't give any strange position. @@ -279,6 +280,7 @@ static int snd_pcm_update_hw_ptr_interrupt(struct snd_pcm_substream *substream) delta = 0; } no_jiffies_check: +#endif /* no jiffies check */ if (delta > runtime->period_size + runtime->period_size / 2) { hw_ptr_error(substream, "Lost interrupts? " @@ -336,6 +338,7 @@ int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream) hw_base = 0; new_hw_ptr = hw_base + pos; } +#if 0 /* no jiffies check */ if (((delta * HZ) / runtime->rate) > jdelta + HZ/100) { hw_ptr_error(substream, "hw_ptr skipping! " @@ -345,6 +348,7 @@ int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream) ((delta * HZ) / runtime->rate)); return 0; } +#endif /* no jiffies check */ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && runtime->silence_size > 0) snd_pcm_playback_silence(substream, new_hw_ptr);
Takashi:
Your patch worked. jiffies seems to be the cause. xrun_debug is on and I do not see any of the error messages.
[Re-added some dropped addresses to Cc]
Thanks for testing!
I have an impression that the jiffies check can behave pretty buggy in cases the hardware doesn't give a smooth pointer update. It's supposed to filter out spontaneous invalid pointer values. That'll work indeed (although I don't think the check in snd_pcm_period_elapsed() is worth). But what if a hardware returns the pointer value that is not always updated smoothly?
Assume a hardware returning only spontaneously updated values, 0 at t=0, 1000 at t=100, 2000 at t=200, and so on. If snd_pcm_update_hw_ptr() is called at t=99, it gets still position 0. Then when it's called at t=100, it suddenly increases to position 1000 while jiffies delta is only 1. The current code would filter out this update because it appears as if too much.
The problem continues, if snd_pcm_update_hw_ptr() is constantly called at each t=100, 101, ... At each call, jiffies delta is 1, but the position delta is still 1000 because it wasn't updated. That is, as long as the app keeps calling snd_pcm_update_hw_ptr(), the position won't be updated. This is pretty possible especially with an app like pulseaudio.
I have a feeling that we should disable the jiffies check in that part, at least for 2.6.30, until a better solution is found. The jiffies check is already done and works pretty well in the driver side (hda-intel and intel8x0). But this check in the PCM core can be (not always, though) buggy, and actually gives problems sometimes.
Takashi
On Wed, 27 May 2009, Takashi Iwai wrote:
Thanks for testing!
I have an impression that the jiffies check can behave pretty buggy in cases the hardware doesn't give a smooth pointer update. It's supposed to filter out spontaneous invalid pointer values. That'll work indeed (although I don't think the check in snd_pcm_period_elapsed() is worth). But what if a hardware returns the pointer value that is not always updated smoothly?
Assume a hardware returning only spontaneously updated values, 0 at t=0, 1000 at t=100, 2000 at t=200, and so on. If snd_pcm_update_hw_ptr() is called at t=99, it gets still position 0. Then when it's called at t=100, it suddenly increases to position 1000 while jiffies delta is only 1. The current code would filter out this update because it appears as if too much.
That's good point. The hw_ptr_jiffies should be updated only when position in the ring buffer moves:
if (runtime->status->hw_ptr != new_hw_ptr) runtime->hw_ptr_jiffies = jiffies;
Similar condition might be also used for runtime->status->tstamp variable.
The problem continues, if snd_pcm_update_hw_ptr() is constantly called at each t=100, 101, ... At each call, jiffies delta is 1, but the position delta is still 1000 because it wasn't updated. That is, as long as the app keeps calling snd_pcm_update_hw_ptr(), the position won't be updated. This is pretty possible especially with an app like pulseaudio.
I have a feeling that we should disable the jiffies check in that part, at least for 2.6.30, until a better solution is found. The jiffies check is already done and works pretty well in the driver side (hda-intel and intel8x0). But this check in the PCM core can be (not always, though) buggy, and actually gives problems sometimes.
As I already wrote, I have nothing against to make jiffies check conditional on xrun_debug() for 2.6.30 for debugging purposes, but I'd like to leave this code enabled in our development trees. It reveals some buggy drivers which should be checked (a new one is Vortex - au88x0).
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
At Wed, 27 May 2009 09:25:57 +0200 (CEST), Jaroslav Kysela wrote:
On Wed, 27 May 2009, Takashi Iwai wrote:
Thanks for testing!
I have an impression that the jiffies check can behave pretty buggy in cases the hardware doesn't give a smooth pointer update. It's supposed to filter out spontaneous invalid pointer values. That'll work indeed (although I don't think the check in snd_pcm_period_elapsed() is worth). But what if a hardware returns the pointer value that is not always updated smoothly?
Assume a hardware returning only spontaneously updated values, 0 at t=0, 1000 at t=100, 2000 at t=200, and so on. If snd_pcm_update_hw_ptr() is called at t=99, it gets still position 0. Then when it's called at t=100, it suddenly increases to position 1000 while jiffies delta is only 1. The current code would filter out this update because it appears as if too much.
That's good point. The hw_ptr_jiffies should be updated only when position in the ring buffer moves:
if (runtime->status->hw_ptr != new_hw_ptr) runtime->hw_ptr_jiffies = jiffies;
Similar condition might be also used for runtime->status->tstamp variable.
Yep, that'll be needed.
The problem continues, if snd_pcm_update_hw_ptr() is constantly called at each t=100, 101, ... At each call, jiffies delta is 1, but the position delta is still 1000 because it wasn't updated. That is, as long as the app keeps calling snd_pcm_update_hw_ptr(), the position won't be updated. This is pretty possible especially with an app like pulseaudio.
I have a feeling that we should disable the jiffies check in that part, at least for 2.6.30, until a better solution is found. The jiffies check is already done and works pretty well in the driver side (hda-intel and intel8x0). But this check in the PCM core can be (not always, though) buggy, and actually gives problems sometimes.
As I already wrote, I have nothing against to make jiffies check conditional on xrun_debug() for 2.6.30 for debugging purposes, but I'd like to leave this code enabled in our development trees. It reveals some buggy drivers which should be checked (a new one is Vortex - au88x0).
OK, I created a patch below and will included in the next pull request today or tomorrow.
thanks,
Takashi
=== From c87d9732004b3f8fd82d729f12ccfb96c0df279e Mon Sep 17 00:00:00 2001 From: Takashi Iwai tiwai@suse.de Date: Wed, 27 May 2009 10:53:33 +0200 Subject: [PATCH] ALSA: Enable PCM hw_ptr_jiffies check only in xrun_debug mode
The PCM hw_ptr jiffies check results sometimes in problems when a hardware doesn't give smooth hw_ptr updates. So far, au88x0 and some other drivers appear not working due to this strict check. However, this check is a nice debug tool, and the capability should be still kept.
Hence, we disable this check now as default unless the user enables it by setting the xrun_debug mode to the specific stream via a proc file.
Signed-off-by: Takashi Iwai tiwai@suse.de --- Documentation/sound/alsa/Procfile.txt | 5 +++++ sound/core/pcm_lib.c | 9 ++++++++- 2 files changed, 13 insertions(+), 1 deletions(-)
diff --git a/Documentation/sound/alsa/Procfile.txt b/Documentation/sound/alsa/Procfile.txt index bba2dbb..cfac20c 100644 --- a/Documentation/sound/alsa/Procfile.txt +++ b/Documentation/sound/alsa/Procfile.txt @@ -104,6 +104,11 @@ card*/pcm*/xrun_debug When this value is greater than 1, the driver will show the stack trace additionally. This may help the debugging.
+ Since 2.6.30, this option also enables the hwptr check using + jiffies. This detects spontaneous invalid pointer callback + values, but can be lead to too much corrections for a (mostly + buggy) hardware that doesn't give smooth pointer updates. + card*/pcm*/sub*/info The general information of this PCM sub-stream.
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 3eea98a..d659995 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -249,6 +249,11 @@ static int snd_pcm_update_hw_ptr_interrupt(struct snd_pcm_substream *substream) new_hw_ptr = hw_base + pos; } } + + /* Do jiffies check only in xrun_debug mode */ + if (!xrun_debug(substream)) + goto no_jiffies_check; + /* Skip the jiffies check for hardwares with BATCH flag. * Such hardware usually just increases the position at each IRQ, * thus it can't give any strange position. @@ -336,7 +341,9 @@ int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream) hw_base = 0; new_hw_ptr = hw_base + pos; } - if (((delta * HZ) / runtime->rate) > jdelta + HZ/100) { + /* Do jiffies check only in xrun_debug mode */ + if (xrun_debug(substream) && + ((delta * HZ) / runtime->rate) > jdelta + HZ/100) { hw_ptr_error(substream, "hw_ptr skipping! " "(pos=%ld, delta=%ld, period=%ld, jdelta=%lu/%lu)\n",
On Wed, May 27, 2009 at 6:41 AM, Takashi Iwai tiwai@suse.de wrote:
At Wed, 27 May 2009 09:25:57 +0200 (CEST), Jaroslav Kysela wrote:
On Wed, 27 May 2009, Takashi Iwai wrote:
Thanks for testing!
I have an impression that the jiffies check can behave pretty buggy in cases the hardware doesn't give a smooth pointer update. It's supposed to filter out spontaneous invalid pointer values. That'll work indeed (although I don't think the check in snd_pcm_period_elapsed() is worth). But what if a hardware returns the pointer value that is not always updated smoothly?
Assume a hardware returning only spontaneously updated values, 0 at t=0, 1000 at t=100, 2000 at t=200, and so on. If snd_pcm_update_hw_ptr() is called at t=99, it gets still position 0. Then when it's called at t=100, it suddenly increases to position 1000 while jiffies delta is only 1. The current code would filter out this update because it appears as if too much.
That's good point. The hw_ptr_jiffies should be updated only when position in the ring buffer moves:
if (runtime->status->hw_ptr != new_hw_ptr) runtime->hw_ptr_jiffies = jiffies;
Similar condition might be also used for runtime->status->tstamp variable.
Yep, that'll be needed.
The problem continues, if snd_pcm_update_hw_ptr() is constantly called at each t=100, 101, ... At each call, jiffies delta is 1, but the position delta is still 1000 because it wasn't updated. That is, as long as the app keeps calling snd_pcm_update_hw_ptr(), the position won't be updated. This is pretty possible especially with an app like pulseaudio.
I have a feeling that we should disable the jiffies check in that part, at least for 2.6.30, until a better solution is found. The jiffies check is already done and works pretty well in the driver side (hda-intel and intel8x0). But this check in the PCM core can be (not always, though) buggy, and actually gives problems sometimes.
As I already wrote, I have nothing against to make jiffies check conditional on xrun_debug() for 2.6.30 for debugging purposes, but I'd like to leave this code enabled in our development trees. It reveals some buggy drivers which should be checked (a new one is Vortex - au88x0).
OK, I created a patch below and will included in the next pull request today or tomorrow.
thanks,
Takashi
===
From c87d9732004b3f8fd82d729f12ccfb96c0df279e Mon Sep 17 00:00:00 2001
From: Takashi Iwai tiwai@suse.de Date: Wed, 27 May 2009 10:53:33 +0200 Subject: [PATCH] ALSA: Enable PCM hw_ptr_jiffies check only in xrun_debug mode
The PCM hw_ptr jiffies check results sometimes in problems when a hardware doesn't give smooth hw_ptr updates. So far, au88x0 and some other drivers appear not working due to this strict check. However, this check is a nice debug tool, and the capability should be still kept.
Hence, we disable this check now as default unless the user enables it by setting the xrun_debug mode to the specific stream via a proc file.
Signed-off-by: Takashi Iwai tiwai@suse.de
Documentation/sound/alsa/Procfile.txt | 5 +++++ sound/core/pcm_lib.c | 9 ++++++++- 2 files changed, 13 insertions(+), 1 deletions(-)
diff --git a/Documentation/sound/alsa/Procfile.txt b/Documentation/sound/alsa/Procfile.txt index bba2dbb..cfac20c 100644 --- a/Documentation/sound/alsa/Procfile.txt +++ b/Documentation/sound/alsa/Procfile.txt @@ -104,6 +104,11 @@ card*/pcm*/xrun_debug When this value is greater than 1, the driver will show the stack trace additionally. This may help the debugging.
- Since 2.6.30, this option also enables the hwptr check using
- jiffies. This detects spontaneous invalid pointer callback
- values, but can be lead to too much corrections for a (mostly
- buggy) hardware that doesn't give smooth pointer updates.
card*/pcm*/sub*/info The general information of this PCM sub-stream.
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 3eea98a..d659995 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -249,6 +249,11 @@ static int snd_pcm_update_hw_ptr_interrupt(struct snd_pcm_substream *substream) new_hw_ptr = hw_base + pos; } }
- /* Do jiffies check only in xrun_debug mode */
- if (!xrun_debug(substream))
- goto no_jiffies_check;
/* Skip the jiffies check for hardwares with BATCH flag. * Such hardware usually just increases the position at each IRQ, * thus it can't give any strange position. @@ -336,7 +341,9 @@ int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream) hw_base = 0; new_hw_ptr = hw_base + pos; }
- if (((delta * HZ) / runtime->rate) > jdelta + HZ/100) {
- /* Do jiffies check only in xrun_debug mode */
- if (xrun_debug(substream) &&
- ((delta * HZ) / runtime->rate) > jdelta + HZ/100) {
hw_ptr_error(substream, "hw_ptr skipping! " "(pos=%ld, delta=%ld, period=%ld, jdelta=%lu/%lu)\n", -- 1.6.3
Takashi:
I will test this patch. Also, I have some results from the test with arecord that Jaroslav wanted.
Best Regards,
participants (3)
-
Jaroslav Kysela -
Robert Krakora -
Takashi Iwai