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@xxxxxxxx> Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc. _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
