[alsa-devel] [PATCH 0/3] ALSA: Add the explicit appl_ptr sync support
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(-)
Currently x86 platforms use the PCM status/control mmaps for transferring the PCM status and appl_ptr between kernel and user-spaces. The mmap is a most efficient way of communication, but it has a drawback per its nature, namely, it can't notify the change explicitly to kernel.
The lack of appl_ptr update notification is a problem on a few existing drivers, but it's mostly a small issue and negligible. However, a new type of driver that uses DSP for a deep buffer management requires the exact position of appl_ptr for calculating the buffer prefetch size, and the asynchronous appl_ptr update between kernel and user-spaces becomes a significant problem for it.
How can we enforce user-space to report the appl_ptr update? The way is relatively simple. Just by disabling the PCM control mmap, the user-space is supposed to fall back to the mode using SYNC_PTR ioctl, and the kernel gets control over that. This fallback mode is used in all non-x86 platforms as default, and also in the 32bit compatible model on all platforms including x86. It's been implemented already over a decade, so we can say it's fairly safe and stably working.
With the help of the knowledge above, this patch introduces a new PCM info flag SNDRV_PCM_INFO_SYNC_APPLPTR for achieving the appl_ptr sync from user-space. When a driver sets this flag at open, the PCM status / control mmap is disabled, which effectively switches to SYNC_PTR mode in user-space side.
In this version, both PCM status and control mmaps are disabled although only the latter, control mmap, is the target. It's because the current alsa-lib implementation supposes that both status and control mmaps are always coupled, thus it handles a fatal error when only one of them fails.
Of course, the disablement of the status/control mmaps may bring a slight performance overhead. Thus, as of now, this should be used only for the dedicated devices that deserves.
Note that the disablement of mmap is a sort of workaround. In the later patch, we'll introduce the way to identify the protocol version alsa-lib supports, and keep mmap working while the sync_ptr is performed together.
Signed-off-by: Takashi Iwai tiwai@suse.de --- include/uapi/sound/asound.h | 1 + sound/core/pcm_native.c | 14 ++++++++++++++ 2 files changed, 15 insertions(+)
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index fd41697cb4d3..7eee52eb7462 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -268,6 +268,7 @@ typedef int __bitwise snd_pcm_subformat_t; #define SNDRV_PCM_INFO_MMAP_VALID 0x00000002 /* period data are valid during transfer */ #define SNDRV_PCM_INFO_DOUBLE 0x00000004 /* Double buffering needed for PCM start/stop */ #define SNDRV_PCM_INFO_BATCH 0x00000010 /* double buffering */ +#define SNDRV_PCM_INFO_SYNC_APPLPTR 0x00000020 /* need the explicit sync of appl_ptr update */ #define SNDRV_PCM_INFO_INTERLEAVED 0x00000100 /* channels are interleaved */ #define SNDRV_PCM_INFO_NONINTERLEAVED 0x00000200 /* channels are not interleaved */ #define SNDRV_PCM_INFO_COMPLEX 0x00000400 /* complex frame organization (mmap only) */ diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index d35c6614fdab..ef2ce64dcdb3 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -3330,6 +3330,16 @@ static int snd_pcm_mmap_status(struct snd_pcm_substream *substream, struct file struct vm_area_struct *area) { long size; + + /* Disallow the status/control mmap when SYNC_APPLPTR flag is set; + * it enforces the user-space to fall back to snd_pcm_sync_ptr(), + * thus it effectively assures the manual update of appl_ptr. + * In theory, it should be enough to disallow only PCM control mmap, + * but since the current alsa-lib implementation requires both status + * and control mmaps always paired, we have to disable both of them. + */ + if (substream->runtime->hw.info & SNDRV_PCM_INFO_SYNC_APPLPTR) + return -ENXIO; if (!(area->vm_flags & VM_READ)) return -EINVAL; size = area->vm_end - area->vm_start; @@ -3366,6 +3376,10 @@ static int snd_pcm_mmap_control(struct snd_pcm_substream *substream, struct file struct vm_area_struct *area) { long size; + + /* see the comment in snd_pcm_mmap_status() */ + if (substream->runtime->hw.info & SNDRV_PCM_INFO_SYNC_APPLPTR) + return -ENXIO; if (!(area->vm_flags & VM_READ)) return -EINVAL; size = area->vm_end - area->vm_start;
We have an ioctl to inform the PCM protocol version the running kernel supports, but there is no way to know which protocol version the user-space can understand. This lack of information caused headaches in the past when we tried to extend the ABI. For example, because we couldn't guarantee the validity of the reserved bytes, we had to introduce a new ioctl SNDRV_PCM_IOCTL_STATUS_EXT for assigning a few new fields in the formerly reserved bits. If we could know that it's a new alsa-lib, we could assume the availability of the new fields, thus we could have reused the existing SNDRV_PCM_IOCTL_STATUS.
In order to improve the ABI extensibility, this patch adds a new ioctl for user-space to inform its supporting protocol version to the kernel. By reporting the supported protocol from user-space, the kernel can judge which feature should be provided and which not.
With the addition of the new ioctl, the PCM protocol version is bumped to 2.0.14, too. User-space checks the kernel protocol version via SNDRV_PCM_INFO_PVERSION, then it sets the supported version back via SNDRV_PCM_INFO_USER_PVERSION.
Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/pcm.h | 1 + include/uapi/sound/asound.h | 3 ++- sound/core/pcm_compat.c | 1 + sound/core/pcm_native.c | 7 +++++++ 4 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 5a22075c5fcf..6a94f8865846 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -352,6 +352,7 @@ struct snd_pcm_runtime { unsigned long hw_ptr_buffer_jiffies; /* buffer time in jiffies */ snd_pcm_sframes_t delay; /* extra delay; typically FIFO size */ u64 hw_ptr_wrap; /* offset for hw_ptr due to boundary wrap-around */ + unsigned int user_pversion; /* supported protocol version */
/* -- HW params -- */ snd_pcm_access_t access; /* access mode */ diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 7eee52eb7462..1949923a40bf 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -152,7 +152,7 @@ struct snd_hwdep_dsp_image { * * *****************************************************************************/
-#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 13) +#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 14)
typedef unsigned long snd_pcm_uframes_t; typedef signed long snd_pcm_sframes_t; @@ -564,6 +564,7 @@ enum { #define SNDRV_PCM_IOCTL_INFO _IOR('A', 0x01, struct snd_pcm_info) #define SNDRV_PCM_IOCTL_TSTAMP _IOW('A', 0x02, int) #define SNDRV_PCM_IOCTL_TTSTAMP _IOW('A', 0x03, int) +#define SNDRV_PCM_IOCTL_USER_PVERSION _IOW('A', 0x04, int) #define SNDRV_PCM_IOCTL_HW_REFINE _IOWR('A', 0x10, struct snd_pcm_hw_params) #define SNDRV_PCM_IOCTL_HW_PARAMS _IOWR('A', 0x11, struct snd_pcm_hw_params) #define SNDRV_PCM_IOCTL_HW_FREE _IO('A', 0x12) diff --git a/sound/core/pcm_compat.c b/sound/core/pcm_compat.c index 8a0f8d51e95d..10f537f4d735 100644 --- a/sound/core/pcm_compat.c +++ b/sound/core/pcm_compat.c @@ -676,6 +676,7 @@ static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned l case SNDRV_PCM_IOCTL_INFO: case SNDRV_PCM_IOCTL_TSTAMP: case SNDRV_PCM_IOCTL_TTSTAMP: + case SNDRV_PCM_IOCTL_USER_PVERSION: case SNDRV_PCM_IOCTL_HWSYNC: case SNDRV_PCM_IOCTL_PREPARE: case SNDRV_PCM_IOCTL_RESET: diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index ef2ce64dcdb3..df3e0303409f 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2779,6 +2779,13 @@ static int snd_pcm_common_ioctl(struct file *file, return 0; case SNDRV_PCM_IOCTL_TTSTAMP: return snd_pcm_tstamp(substream, arg); + case SNDRV_PCM_IOCTL_USER_PVERSION: + if (substream->runtime->status->state != SNDRV_PCM_STATE_OPEN) + return -EBADFD; + if (get_user(substream->runtime->user_pversion, + (unsigned int __user*)arg)) + return -EFAULT; + return 0; case SNDRV_PCM_IOCTL_HW_REFINE: return snd_pcm_hw_refine_user(substream, arg); case SNDRV_PCM_IOCTL_HW_PARAMS:
Now that user-space (typically alsa-lib) can specify which protocol version it supports, we can optimize the kernel code depending on the reported protocol version.
In this patch, we change the previous hack for enforcing the appl_ptr sync by disabling status/control mmap. Instead of forcibly disabling, we disable the status/control mmap selectively only when the user-space does *not* understand the protocol. That is, when the user-space reports it supporting the new protocol version including the sync appl_ptr feature, we allow the mmap like the normal case. Then user-space is supposed to do the sync in addition to the mmap.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_native.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index df3e0303409f..3f91d5c4ed76 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -3336,6 +3336,7 @@ static const struct vm_operations_struct snd_pcm_vm_ops_status = static int snd_pcm_mmap_status(struct snd_pcm_substream *substream, struct file *file, struct vm_area_struct *area) { + struct snd_pcm_runtime *runtime = substream->runtime; long size;
/* Disallow the status/control mmap when SYNC_APPLPTR flag is set; @@ -3344,8 +3345,12 @@ static int snd_pcm_mmap_status(struct snd_pcm_substream *substream, struct file * In theory, it should be enough to disallow only PCM control mmap, * but since the current alsa-lib implementation requires both status * and control mmaps always paired, we have to disable both of them. + * + * If user_pversion is 2.0.14 or greater, it implies that user-space + * supports the explicit appl_ptr sync, so we still allow the mmap. */ - if (substream->runtime->hw.info & SNDRV_PCM_INFO_SYNC_APPLPTR) + if (runtime->user_pversion < SNDRV_PROTOCOL_VERSION(2, 0, 14) && + (runtime->hw.info & SNDRV_PCM_INFO_SYNC_APPLPTR)) return -ENXIO; if (!(area->vm_flags & VM_READ)) return -EINVAL; @@ -3382,10 +3387,12 @@ static const struct vm_operations_struct snd_pcm_vm_ops_control = static int snd_pcm_mmap_control(struct snd_pcm_substream *substream, struct file *file, struct vm_area_struct *area) { + struct snd_pcm_runtime *runtime = substream->runtime; long size;
/* see the comment in snd_pcm_mmap_status() */ - if (substream->runtime->hw.info & SNDRV_PCM_INFO_SYNC_APPLPTR) + if (runtime->user_pversion < SNDRV_PROTOCOL_VERSION(2, 0, 14) && + (runtime->hw.info & SNDRV_PCM_INFO_SYNC_APPLPTR)) return -ENXIO; if (!(area->vm_flags & VM_READ)) return -EINVAL;
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
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.
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
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
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.
... 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.
Thanks
Takashi Sakamoto
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
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
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
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.
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?
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
thanks,
Takashi
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
participants (2)
-
Takashi Iwai -
Takashi Sakamoto