[alsa-devel] 2.6.30: xruns caused by "ALSA: pcm - Safer boundary checks"

[Robert Krakora]

On Wed, May 27, 2009 at 6:41 AM, Takashi Iwai <tiwai at 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 at 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 at 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,

-- 
Rob Krakora
Senior Software Engineer
MessageNet Systems
101 East Carmel Dr. Suite 105
Carmel, IN 46032
(317)566-1677 Ext. 206
(317)663-0808 Fax