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

[Takashi Iwai]

At Mon, 25 May 2009 13:24:46 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Mon, 25 May 2009, Takashi Iwai wrote:
> 
> > At Mon, 25 May 2009 12:19:19 +0200 (CEST),
> > Jaroslav Kysela wrote:
> >>
> >> On Sat, 23 May 2009, Takashi Iwai wrote:
> >>
> >>> At Sat, 23 May 2009 14:31:57 +0200 (CEST),
> >>> Jaroslav Kysela wrote:
> >>>>
> >>>> On Fri, 22 May 2009, Takashi Iwai wrote:
> >>>>
> >>>>> At Fri, 22 May 2009 16:27:09 -0400,
> >>>>> Chuck Ebbert wrote:
> >>>>>>
> >>>>>> Bug report: https://bugzilla.redhat.com/show_bug.cgi?id=498858
> >>>>>>
> >>>>>> To reproduce, pause a video in mplayer and then hit play:
> >>>>>>
> >>>>>> ALSA sound/core/pcm_lib.c:263: hda_codec: hw_ptr skipping! [Q] (pos=11263,
> >>>>>> delta=17400, period=1024, jdelta=21/362/0)
> >>>>>
> >>>>> Does the patch fix the problem?
> >>>>
> >>>> This patch does not look as a proper fix.
> >>>
> >>> Hm, at which point?
> >>> It just skips the first unreliable jiffies check.
> >>> At least, it's interesting whether it works around the problem.
> >>
> >> I meant that it is not necessary to introduce a next variable. This patch
> >> is more elegant:
> >>
> >> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> >> index a2a792c..3eea98a 100644
> >> --- a/sound/core/pcm_lib.c
> >> +++ b/sound/core/pcm_lib.c
> >> @@ -1478,7 +1478,6 @@ static int snd_pcm_lib_ioctl_reset(struct snd_pcm_substream *substream,
> >>   		runtime->status->hw_ptr %= runtime->buffer_size;
> >>   	else
> >>   		runtime->status->hw_ptr = 0;
> >> -	runtime->hw_ptr_jiffies = jiffies;
> >>   	snd_pcm_stream_unlock_irqrestore(substream, flags);
> >>   	return 0;
> >>   }
> >> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> >> index fc6f98e..7faec82 100644
> >> --- a/sound/core/pcm_native.c
> >> +++ b/sound/core/pcm_native.c
> >> @@ -848,6 +848,7 @@ static void snd_pcm_post_start(struct snd_pcm_substream *substream, int state)
> >>   {
> >>   	struct snd_pcm_runtime *runtime = substream->runtime;
> >>   	snd_pcm_trigger_tstamp(substream);
> >> +	runtime->hw_ptr_jiffies = jiffies;
> >>   	runtime->status->state = state;
> >>   	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
> >>   	    runtime->silence_size > 0)
> >> @@ -961,6 +962,8 @@ static int snd_pcm_do_pause(struct snd_pcm_substream *substream, int push)
> >>   {
> >>   	if (substream->runtime->trigger_master != substream)
> >>   		return 0;
> >> +	/* skip one jiffies check */
> >> +	runtime->hw_ptr_jiffies = jiffies - HZ * 1000;
> >
> > This part is a bit tricky from readability POV, but I agree it's
> > better to avoid a new field.
> 
> Perhaps a better comment might improve readability ;-)

Yep :)

> > However, one thing I thought in my patch is that it's safer *not* to
> > check the jiffies even for the first update after snd_pcm_start().
> > The potential problem we have now is that the first position check
> > might be too early when the device has a large FIFO or so.  By
> > skipping the first jiffies check, this could be worked around.
> > Usually the stream is stabilized and give periodic position updates
> > once when it's started.  But, after the first start, it's not always
> > stable enough.
> 
> I would use runtime->delay to update the check margin (hdelta -= 
> runtime->delay) for such hardware instead this assumption. The 
> runtime->delay can be updated in the lowlevel driver - pointer() 
> callback. It will work for both fixed and variable FIFOs.

Right, but it's always hard to guess and set proper FIFO values to all
possible devices, and it's too late for 2.6.30.
As a working practice for 2.6.30, I'd vote for taking a safer option.


thanks,

Takashi