Hi,
On Jun 22 2017 17:44, Takashi Iwai wrote:
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.
I posted patchset for alsa-lib for your C) idea. In this patchset,
mapping for status/control data is processed independently,
[alsa-lib][RFC][PATCH 0/9] pcm: handle status/control
mapping independently
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/122130.html
Additionally, some helper functions to call ioctl(2) with
SNDRV_PCM_IOCTL_SYNC_PTR for several purposes. I think to get help from
it for our arrangement to A) and B).
I agree with the idea in A) to add a flag to update state only. For the
other probabilities, I need time to consider about it.
... 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.
Regards
Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
