[alsa-devel] 2.6.30: xruns caused by "ALSA: pcm - Safer boundary checks"
perex at perex.cz
Mon May 25 13:24:46 CEST 2009
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;
>> 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;
>> + 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 Kysela <perex at perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.
More information about the Alsa-devel