[alsa-devel] [PATCH 0/3] ALSA: Add the explicit appl_ptr sync support

[Takashi Iwai]

On Wed, 21 Jun 2017 13:21:06 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Jun 21 2017 00:33, Takashi Iwai wrote:
> > Hi,
> >
> > this is a patchset for the issues we've discussed recently lengthily,
> > the explicit sync of appl_ptr by disabling the control mmap.
> > The first patch achieves it for a driver setting the new INFO flag.
> >
> > The second patch is for a generic ABI extension I wanted to implement
> > for long time.  It allows user-space reporting its supporting protocol
> > version, so that the kernel can behave differently.  It's especially
> > useful for extending the ABI struct to use the reserved bits.
> >
> > The third patch is the optimization by the second patch; it simply
> > skips the previous workaround when the user-space declares itself
> > being new enough to support the feature.
> >
> > The corresponding alsa-lib patchset will follow in another thread.
> >
> >
> > thanks,
> >
> > Takashi
> >
> > ===
> >
> > Takashi Iwai (3):
> >    ALSA: pcm: Add the explicit appl_ptr sync support
> >    ALSA: pcm: Add an ioctl to specify the supported protocol version
> >    ALSA: pcm: Limit the appl_ptr sync workaround only for old user-space
> >
> >   include/sound/pcm.h         |  1 +
> >   include/uapi/sound/asound.h |  4 +++-
> >   sound/core/pcm_compat.c     |  1 +
> >   sound/core/pcm_native.c     | 28 ++++++++++++++++++++++++++++
> >   4 files changed, 33 insertions(+), 1 deletion(-)
> 
> I prefer this direction and support this patchset.
> 
> Reviewed-by: Takashi Sakamoto <o-takashi at sakamocchi.jp>
> Tested-by: Takashi Sakamoto <o-takashi at sakamocchi.jp>

Thank you for your support!

> However, when running with patched alsa-lib[1], due to a commit
> f8ff2f28ba49 ("ALSA: pcm: Skip ack callback without actual appl_ptr
> update")[2], callback functions in driver side for .ack are not called
> from ioctl handler in ALSA PCM core. When page frames for control data
> of PCM substream is mapped to VMA of the application, the application
> updates the value of pointer in the frame, then transfer the value via
> ioctl. Comparing these two values should be the same and ALSA PCM core
> skip the callback.

Oh, that's a damn good point.

Currently alsa-lib issues sync_ptr() also for obtaining the PCM state
from the kernel, and this also triggers ack unless SYNC_APPL_PTR flag
is set.  The commit f8ff2f28ba49 tried to reduce the ack calls  by
filtering in pcm_lib_apply_appl_ptr() side, but it has a side-effect
as you noticed.

There can be a few options to cover this for the newer alsa-lib, for
example:

A. Add a new bit flag to sync_ptr() allowing only the state update
   and/or the explicit applptr update.

B. Introduce a completely new ioctl for the appl_ptr sync.

C. Keep disabling control mmap while allowing state mmap.

D. Track the last appl_ptr individually from runtime->control->appl_ptr
   and compare/update with this value in pcm_lib_apply_appl_ptr().

E. Remove the filtering from pcm_lib_apply_appl_ptr(), let each driver
   optimize if needed.

Currently I'm thinking of (A), but open for all and other possible
options.  All these may need the check of user_pversion so that the
old code still works as is.


... and, while testing the patchset today, I noticed a bug in the
user_pversion check.  Namely, the user_pversion check must not be done
against a substream.  A substream may be shared among multiple
processes (typically by dmix), and changing from one process may hit
others when it's set in the substream object.  For identifying the
setup for each stream (from the user-space POV), the flag has to be
stored in snd_pcm_file instead, just like no_compat_mmap flag.

That said, I have to respin a new patch in anyway, at least for the
patches 2 & 3 :) I'll leave your reviewed-by tag from these, please
review the new patchset again.  Sorry for doubly works.


thanks,

Takashi

> When considering about behaviour of old userland stuffs, it's not
> necessarily easy to reduce call frequency of the callback, mmm...


> [1]
> http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/122066.html
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/sound/core/pcm_lib.c?h=topic/pcm-sync-applptr&id=f8ff2f28ba49fa41a06215ac3187dede347bc9a7
> 
> Thanks
> 
> Takashi Sakamoto
>