[alsa-devel] [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Takashi Iwai]

On Thu, 18 May 2017 08:18:21 +0200,
Subhransu S. Prusty wrote:
> 
> On Tue, May 16, 2017 at 01:06:57PM +0530, Subhransu S. Prusty wrote:
> > On Tue, May 16, 2017 at 07:56:44AM +0200, Takashi Iwai wrote:
> > > On Tue, 16 May 2017 03:01:57 +0200,
> > > Subhransu S. Prusty wrote:
> > > > 
> > > > From: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> > > > 
> > > > When appl_ptr is updated let low-level driver know, e.g.  to let the
> > > > low-level driver/hardware pre-fetch data opportunistically.
> > > > 
> > > > The existing .ack callback is extended with new attribute argument, to
> > > > support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
> > > > doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
> > > > process the application ptr update in the driver like in the skylake
> > > > driver which can use this to inform position of appl pointer to host DMA
> > > > controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
> > > > submitted with a separate patch set.
> > > > 
> > > > In the ALSA core, this capability is independent from the NO_REWIND
> > > > hardware flag. The low-level driver may however tie both options and only
> > > > use the updated appl_ptr when rewinds are disabled due to hardware
> > > > limitations.
> > > > 
> > > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> > > > Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi at intel.com>
> > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty at intel.com>
> > > 
> > > It might be me who suggested the extension of the ack ops, but now
> > > looking at the result, I reconsider whether it'd be a better choice if
> > > we add another ops (e.g. update_appl_ptr()) instead.  Could you try to
> > > rewrite the patch in that way for comparison?
> > 
> > Here is the version using update_appl_ptr.
> 
> Hi Takashi,
> 
> Did you get a chance to look at the update_appl_ptr changes?
> Please let us know which one will be preferable, will submit the patches
> accordingly.

Now I have a mixed feeling.  Using ack() is basically "the right
thing".  The update of appl_ptr in forward/rewind and sync_ptr should
be notified to ack() in general.  It's the purpose of ack(), after
all.  In that sense, we may call ack() without any argument from any
places.

The only problem is that the rewind is broken on some drivers, and
calling ack() may lead to unexpected results.

That is, we should look at these existing drivers and handle the
rewind case (negative appl_ptr diff) appropriately -- or maybe we
should add a flag to disallow the rewind on such drivers.
After that, ack() can be called safely from all places that update
appl_ptr.

... this is one way.  Another way is to allow a quick hack and doubly
call a new callback.

I prefer the former, but obviously it'll take longer.  So it depends
on urgency.


Takashi