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

[Takashi Iwai]

At Tue, 28 Apr 2009 10:55:53 +0200 (CEST),
Jaroslav Kysela wrote:
> 
> On Mon, 27 Apr 2009, Mark Brown wrote:
> 
> > On Mon, Apr 27, 2009 at 05:09:27PM +0200, Jaroslav Kysela wrote:
> >
> >> Appearently, FIFO is too big. I'm working on some changes to take account
> >> fifo_size to the jiffies based check. Please, set fifo_size correctly in
> >> your driver. USB drivers with big "URB FIFOs" should be changed as well,
> >> too.
> >
> > Could you expand a bit on what qualifies as "too big" here?  I've not
> > been following the thread here or the changes that caused problems.
> 
> The problem is that period_elapsed() callback checks if data are not 
> "consumed" (playback) or "produced" (capture) too quickly using jiffies 
> timing (with HZ/100 margin). In case of large FIFO, the data are 
> buffered, but the real playback position is different.

Well, but the hardware accepts the data of period_size already at this 
point.  In that sense, the driver returns the correct hwptr value,
i.e. pointer callback behaves in a correct way.


> The question is, if driver's pointer function should estimate more real 
> stream position (taking in account the FIFO) in this case or if upper 
> layers of the ALSA driver and applications should handle the situation 
> using FIFO size value.

Right.  The real problem is that hwptr doesn't represent the real
playing/recording position in these hardware.  But, it worked like that,
thus there is no reason to break this behavior at this point.  The
identity of hwptr and the real position wasn't (isn't) strictly
defined yet.

> I would suggest this:
> 
> - implement the stream position corrections in the pointer callback -
>    in this case applications get valid realtime timing source for
>    A/V synchronization etc. - in case of mpc driver, another timing
>    and interrupt source should be used to compute the pointer value not
>    DMA engine

I don't think the pointer callback should be changed for that.
We have two different pointer representations, and the pointer callback
itself works as is.

> - leave current fifo_size without change (there will be only small FIFO
>    values to notify applications about small extra delays before samples
>    reaches DAC or leaves ADC)

fifo_size field is supposed to be constant, and shouldn't be changed
dynamically after open.  This is a problem if the FIFO size varies
depending on the parameter.

> - add cache_size value to snd_pcm_hardware struct - here will be
>    stored area of buffer which might be cached in the hardware for large
>    FIFOs including prepared USB's URBs (the upper layer should not work
>    with this buffer area - rewind operation etc.)

I already submitted a similar patch quite ago.  Actually, it exposes
the delay account in snd_pcm_status() delay calculation.  The delay
value can be dynamically changed.

The current test patches are found on sound unstable tree
topic/pcm-delay branch.
  git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-unstable-2.6.git


Takashi