[alsa-devel] Driver code with mpc5200 pointer problem.

[Takashi Iwai]

At Tue, 28 Apr 2009 13:30:09 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Tue, 28 Apr 2009, Takashi Iwai wrote:
> 
> > At Tue, 28 Apr 2009 12:15:25 +0200 (CEST),
> > Jaroslav Kysela wrote:
> >>
> >> On Tue, 28 Apr 2009, Takashi Iwai wrote:
> >>
> >>> Well, I think it'd be better, at this moment for 2.6.30, to allow
> >>> problematic drivers to skip the jiffies check.  Adding the fundamental
> >>> change at this late stage is bad, especially if the change wasn't
> >>> tested much by many others and has an influence on user-side API.
> >>>
> >>> So, as I proposed, we can simply add the pcm info check, and add
> >>> BATCH flag to needed drivers.  Of course, it still requires some
> >>> more testing, but basically it'll take back to the old behavior and
> >>> should be safer.
> >>>
> >>> Meanwhile, applying the delay account patch now for 2.6.31 should be
> >>> good (ealier is better).  It doesn't conflict with the info flag
> >>> check, so we can work parallel.
> >>
> >> Could we just add the runtime->delay variable without touching other code
> >> (pcm status), to correct the jiffies based check now?
> >
> > That's possible, of course.
> >
> >> It seems like a good
> >> step forward to me and the lowlevel drivers will be more prepared for next
> >> PCM midlevel code changes.
> >
> > Well, but I guess providing the proper FIFO size isn't trivial to each
> > driver.  There are many devices, as Mark suggested, which may be
> > broken right now, and we can't determine all h/w specs and test them.
> >
> > OTOH, adding INFO_BATCH is fairly easy, because it can be found from
> > the pointer callback implementation of each driver.  On these devices,
> > the jiffies check doesn't help much anyway because it cannot update
> > the hwptr any other than the period size.  So, skipping jiffies check
> > is logically correct as a regression fix.
> 
> I don't see much difference between adding INFO_BATCH and adding 
> runtime->delay = (runtime->period_size)-1 to avoid jiffies check for one 
> period to these drivers (with some notice to author of the driver to 
> correct this value).

INFO_BATCH is the correct flag for such drivers.  They should have the
flag, independent from the fix.

Also, setting -1 to runtime->delay is not what intended for the delay
accounting.  Starting from a (sort of) abuse doesn't sound good...

> INFO_BATCH is currently in:
> 
> /soc/fsl/mpc5200_psc_i2s.c
> /soc/au1x/dbdma2.c
> /soc/sh/dma-sh7760.c
> /usb/usbaudio.c (which works with the current jiffies check at least for
>  		 44100+ rates)
> 
> Another (much cleaner) possibility is to make jiffies check conditional to 
> xrun_check() for 2.6.30 and do not bother with the INFO_BATCH flag.
> 
> Jiffies check helped me to debug HDA/intel8x0 pointer problems but cannot 
> be triggered until driver is really buggy - the HDA/intel8x0 driver should 
> be fixed now (I know about one intel8x0 pointer problem to this time which 
> triggers this check, but there is still something wrong with returned 
> hardware pointer because audio clicks anyway - no regression from previous 
> state).

That makes sense, too.  Maybe even safer.


Takashi