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

[Jaroslav Kysela]

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 ;-)

> 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.

 					Jaroslav

-----
Jaroslav Kysela <perex at perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.