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

[Takashi Iwai]

On Wed, 21 Jun 2017 22:08:40 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Jun 21 2017 22:10, Takashi Iwai wrote:
> >> 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.
> 
> I prefer C) and I'm working for it, because protocol validation of
> userland is not needed in kernel land, as long as I understand. But if
> allowed to add more changes to protocol/interface, A) and B) could be
> within my candidates.

(C) was my original idea, too, so it doesn't sound bad to me, too :)
But, we're bumping the protocol version, so more intensive changes
should be OK.  Let's see.


> > ... 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.
> 
> Oh, I overlooked a case of open(2) with O_APPEND, as well. I also
> think it reasonable to utilize the private data of file descriptor for
> this purpose, good.
> 
> > 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.
> 
> No worry.

So, at this point, I think it's safe to apply the first patch.

The second one is introducing USER_PVERSION ioctl, but it makes little 
sense without the actual usage.  And the last one is obviously not
working well as is, and we may have a better optimization.


thanks,

Takashi