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