[PATCH v2 0/3] ALSA: Add the explicit appl_ptr sync support

[PATCH v2 0/3] ALSA: Add the explicit appl_ptr sync support

[Takashi Iwai]

Hi,

this is a revised patchset for supporting the explicit appl_ptr
update.  Since the protocol version from user-space needs to be stored
locally for each opened file, the check of status/control mmap was
slightly changed as well.  But the basic strategy is same as the
previous patchset.

Note that this patchset doesn't solve the issue that appl_ptr update
isn't properly notified to ack ops as Sakamoto-san pointed out
previously.  We'll still need to sort it out.  But the hack I
introduced here doesn't basically conflict with it, and it'll even
make it easier to solve later, by the new USER_PVERSION ioctl.


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     | 34 ++++++++++++++++++++++++++++++++--
 4 files changed, 37 insertions(+), 3 deletions(-)

-- 
2.13.1

[PATCH v2 1/3] ALSA: pcm: Add the explicit appl_ptr sync support

[Takashi Iwai]

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     | 23 +++++++++++++++++++++--
 2 files changed, 22 insertions(+), 2 deletions(-)

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..9ade0c8b54a3 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3376,10 +3376,29 @@ static int snd_pcm_mmap_control(struct snd_pcm_substream *substream, struct file
 	area->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
 	return 0;
 }
+
+static bool pcm_status_mmap_allowed(struct snd_pcm_file *pcm_file)
+{
+	if (pcm_file->no_compat_mmap)
+		return false;
+	/* 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 (pcm_file->substream->runtime->hw.info & SNDRV_PCM_INFO_SYNC_APPLPTR)
+		return false;
+	return true;
+}
+
 #else /* ! coherent mmap */
 /*
  * don't support mmap for status and control records.
  */
+#define pcm_status_mmap_allowed(pcm_file)	false
+
 static int snd_pcm_mmap_status(struct snd_pcm_substream *substream, struct file *file,
 			       struct vm_area_struct *area)
 {
@@ -3563,11 +3582,11 @@ static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area)
 	offset = area->vm_pgoff << PAGE_SHIFT;
 	switch (offset) {
 	case SNDRV_PCM_MMAP_OFFSET_STATUS:
-		if (pcm_file->no_compat_mmap)
+		if (!pcm_status_mmap_allowed(pcm_file))
 			return -ENXIO;
 		return snd_pcm_mmap_status(substream, file, area);
 	case SNDRV_PCM_MMAP_OFFSET_CONTROL:
-		if (pcm_file->no_compat_mmap)
+		if (!pcm_status_mmap_allowed(pcm_file))
 			return -ENXIO;
 		return snd_pcm_mmap_control(substream, file, area);
 	default:
-- 
2.13.1

[PATCH v2 2/3] ALSA: pcm: Add an ioctl to specify the supported protocol version

[Takashi Iwai]

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..24febf9e177c 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -218,6 +218,7 @@ struct snd_pcm_ops {
 struct snd_pcm_file {
 	struct snd_pcm_substream *substream;
 	int no_compat_mmap;
+	unsigned int user_pversion;	/* supported protocol version */
 };
 
 struct snd_pcm_hw_rule;
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 9ade0c8b54a3..1c53d93e68f2 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2770,6 +2770,8 @@ static int snd_pcm_common_ioctl(struct file *file,
 				 struct snd_pcm_substream *substream,
 				 unsigned int cmd, void __user *arg)
 {
+	struct snd_pcm_file *pcm_file = file->private_data;
+
 	switch (cmd) {
 	case SNDRV_PCM_IOCTL_PVERSION:
 		return put_user(SNDRV_PCM_VERSION, (int __user *)arg) ? -EFAULT : 0;
@@ -2779,6 +2781,11 @@ 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 (get_user(pcm_file->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:
-- 
2.13.1

[PATCH v2 3/3] ALSA: pcm: Limit the appl_ptr sync workaround only for old user-space

[Takashi Iwai]

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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 1c53d93e68f2..88c939a86f36 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3394,8 +3394,12 @@ static bool pcm_status_mmap_allowed(struct snd_pcm_file *pcm_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 (pcm_file->substream->runtime->hw.info & SNDRV_PCM_INFO_SYNC_APPLPTR)
+	if (pcm_file->user_pversion < SNDRV_PROTOCOL_VERSION(2, 0, 14) &&
+	    (pcm_file->substream->runtime->hw.info & SNDRV_PCM_INFO_SYNC_APPLPTR))
 		return false;
 	return true;
 }
-- 
2.13.1

Re: [PATCH v2 1/3] ALSA: pcm: Add the explicit appl_ptr sync support

[Takashi Sakamoto]

Hi,

On Jun 22 2017 00:31, Takashi Iwai wrote:
> 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     | 23 +++++++++++++++++++++--
>   2 files changed, 22 insertions(+), 2 deletions(-)

Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

I note that this patch changed 'asound.h'. We have a plan to bump up 
protocol version in future patches.

> 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..9ade0c8b54a3 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -3376,10 +3376,29 @@ static int snd_pcm_mmap_control(struct snd_pcm_substream *substream, struct file
>   	area->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
>   	return 0;
>   }
> +
> +static bool pcm_status_mmap_allowed(struct snd_pcm_file *pcm_file)
> +{
> +	if (pcm_file->no_compat_mmap)
> +		return false;
> +	/* 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 (pcm_file->substream->runtime->hw.info & SNDRV_PCM_INFO_SYNC_APPLPTR)
> +		return false;
> +	return true;
> +}
> +
>   #else /* ! coherent mmap */
>   /*
>    * don't support mmap for status and control records.
>    */
> +#define pcm_status_mmap_allowed(pcm_file)	false
> +
>   static int snd_pcm_mmap_status(struct snd_pcm_substream *substream, struct file *file,
>   			       struct vm_area_struct *area)
>   {
> @@ -3563,11 +3582,11 @@ static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area)
>   	offset = area->vm_pgoff << PAGE_SHIFT;
>   	switch (offset) {
>   	case SNDRV_PCM_MMAP_OFFSET_STATUS:
> -		if (pcm_file->no_compat_mmap)
> +		if (!pcm_status_mmap_allowed(pcm_file))
>   			return -ENXIO;
>   		return snd_pcm_mmap_status(substream, file, area);
>   	case SNDRV_PCM_MMAP_OFFSET_CONTROL:
> -		if (pcm_file->no_compat_mmap)
> +		if (!pcm_status_mmap_allowed(pcm_file))
>   			return -ENXIO;
>   		return snd_pcm_mmap_control(substream, file, area);
>   	default:


Regards

Takashi Sakamoto

Re: [PATCH v2 1/3] ALSA: pcm: Add the explicit appl_ptr sync support

[Takashi Iwai]

On Fri, 23 Jun 2017 10:07:50 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Jun 22 2017 00:31, Takashi Iwai wrote:
> > 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     | 23 +++++++++++++++++++++--
> >   2 files changed, 22 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> 
> I note that this patch changed 'asound.h'. We have a plan to bump up
> protocol version in future patches.

Right, it's still compatible with the existing ABI, so it should work
without the protocol version bump.  We can increase it now, but we'll
do another update soon, so let's wait for a while.

I merged this patch now.


thanks,

Takashi