Re: [PATCH v2 09/11] ASoC: Intel: Skylake: Fix DMA position reporting for capture stream

[Vinod Koul <vinod.koul@intel.com>]

On Mon, Mar 27, 2017 at 03:12:02PM +0200, Takashi Iwai wrote:
> On Mon, 27 Mar 2017 12:29:54 +0200,
> Vinod Koul wrote:
> > 
> > On Sat, Mar 25, 2017 at 02:21:13AM +0530, Ughreja, Rakesh A wrote:
> > > >> >Are these workarounds needed for the legacy driver?
> > > >> >If yes, which chips require it?
> > > >> >
> > > >>
> > > >> Yes, these are needed in legacy driver as well.
> > > >> From SKL and BXT onwards, it is needed.
> > > >
> > > >OK, thanks for confirmation.
> > > >
> > > >Now, from what I read in the above, is the workaround required *only*
> > > >after the interrupt is generated?  20us delay isn't so cheap, and we
> > > >tend to inquire PCM positions often.  If the workaround is needed only
> > > >after the PCM period elapse, we can set some flag in the irq handler
> > > >and apply the workaround only when necessary.
> > > >
> > > 
> > > Yes, Takashi the workaround is required only in the period elapsed
> > > interrupt. In some cases the DMA Position updates are delayed and so
> > > when the period elapsed interrupt occurs the wait_for_avail thinks that
> > > one period worth of data is not available and so returns only on the 
> > > next period elapsed interrupt. This creates problem for 2 periods
> > > playback/capture streams.
> > > 
> > > So even in the period elapsed interrupt the wait is required only if the
> > > position is less than the period boundary.
> > 
> > Hi Takashi,
> > 
> > So we need this for 2 periods when the device is in irq mode. Not for other
> > cases. ie SKL_PLUS..
> 
> Yeah, thanks.  I'd cook a couple of patches to do that for the legacy
> driver.  But I still wonder whether the wait is always needed.
> 
> Judging from the description, does the discrepancy of posbuf read
> happen *only* when the DMA position goes across the BD boundary?
> Or does it happen at any time?

I think it can happen anytime, but the impact is not felt unless we have a 2
period case. The update is in-flight, so read returns a value lesser than
period boundary. We will sleep till next period ie next wake, which results
in overrun. For more than 2 periods it doesn't impact much as the overrun
case should not happen

> When you trace, you can see that the apps frequently inquires the
> position.  So, an unconditional wait should be really avoided.

If they enquire after a while then we should be okay, but if they are
written nicely with good power behaviour then we may have issue, as writes
are typically done after period boundaries.

> 
> > But have you seen any user reports on this till now.
> 
> I've seen some bug reports mentioning about crackling audio capture on
> SKL (I forgot URLs).  It might be triggered by that.

Quiet possible..

-- 
~Vinod