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

[Takashi Iwai]

At Tue, 26 May 2009 13:16:49 -0400,
Robert Krakora wrote:
> 
> On Tue, May 26, 2009 at 11:41 AM, Takashi Iwai <tiwai at 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 at 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 at 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