Hi,
On Jun 23 2017 01:06, Takashi Iwai wrote:
On Thu, 22 Jun 2017 17:35:34 +0200, Takashi Sakamoto wrote:
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-devel] [alsa-lib][RFC][PATCH 0/9] pcm: handle status/control mapping independently http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/122130.html
The patchset looks good through a quick glance. I'd need to test it more intensively later, but I'm basically fine to go in that direction. In general, this change wouldn't conflict with other improvements.
Thanks for your positive comment. I'm preparing for new patchset with my further self-check.
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.
Yeah, let's take some time.
Meanwhile, I'd like to merge the basic part in order to make maintenance easier. Let's sort out which ones to be applied right now.
At least, the following one should be OK:
[PATCH v2 1/3] ALSA: pcm: Add the explicit appl_ptr sync support
May I re-apply your reviewed-by tag for this?
I did it just now. Let's merge it.
For alsa-lib side, a few of yours and my patches are merely cleanups, so we should apply them beforehand, too.
[PATCH alsa-lib 3/4] pcm: hw: Remove superfluous call of snd_pcm_set_appl_ptr() [alsa-lib][RFC][PATCH 1/9] pcm: obsolete 'mmap_emulation' parameter of snd_pcm_hw_open_fd() [alsa-lib][RFC][PATCH 2/9] pcm: minor code refactoring for ioctl call
I'm OK to apply the cleanups. I already reviewed the first patch so let's merge it. I'll post the rest of two without RFC rebasing to current master a few hours later.
Thanks
Takashi Sakamoto