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

[Takashi Iwai]

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.

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.

So, I think the second hunk in your patch could be also reset
hw_ptr_jiffies to skip.  (If so, we should define a macro to serve
that to improve the readability, BTW.)


thanks,

Takashi