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@sakamocchi.jp Tested-by: Takashi Sakamoto o-takashi@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...
Thanks
Takashi Sakamoto