[RFC][PATCH] ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to v2.0.14

[RFC][PATCH] ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to v2.0.14

[Takashi Sakamoto]

Hi,

This patchset includes commits for both kernel/user land for an issue
addressed in this message thread[1].

In a development period for v4.13, it's clear that there's a type of
devices which demand userspace applications to change their behaviour.
These devices can take care of application-side pointer on PCM buffer
for their data transmission. As a result, these devices achieve some
advantages. For these devices, drivers need to get feedback about
current position of the pointer from applications when they
queue/dequeue PCM frames on the buffer.

ALSA PCM core partly assist the above design. The core can call a
handler in driver side when applications operate PCM frames by
ioctl(2) with SNDRV_PCM_IOCTL_[READ|WRITE][N|I] command. However,
when operating for mapped PCM buffer, it can't assist applications
for the above design. In this scenario, applications should call
ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR after handle any PCM frames on
the buffer.

This patchset bumps up interface version with new info flag to
notify the above change to user land developers. This patchset is
written with below ideas:

1. Stuffs in user land can tell their assists for the above design
   to kernel land by using hw_param flag. When they don't assist,
   they see on hw_params data that mmap operation is not supported.
2. Drivers can acknowledge the above flag. When registering
   appropriate PCM rules in advance, they can calculate values for
   each parameters in both cases that stuffs in user land can/cannot
   assist it. This is useful for packet-oriented drivers to judge
   utilizing any interrupt context for packetization.
3. This is device-specific issue, independently of platform
   architecture. On x86/ppc/alpha platforms, status/control data of
   PCM substream is still available in process' VMA of application via
   page frame mapping.

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/121438.html

For kernel land:
Takashi Sakamoto (1):
  ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to
    2.0.14

 include/uapi/sound/asound.h | 11 ++++++++++-
 sound/core/pcm_native.c     | 25 +++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

For user land:
Takashi Sakamoto (2):
  ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to
    2.0.14
  pcm: add support for interface version 2.0.14

 include/sound/asound.h | 11 ++++++++++-
 src/pcm/pcm_hw.c       | 43 +++++++++++++++++++++++++++++++++++++------
 2 files changed, 47 insertions(+), 7 deletions(-)

-- 
2.11.0

[PATCH 1/1] ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to 2.0.14

[Takashi Sakamoto]

In a development period for v4.13, it's clear that there's a type of
devices which demand userspace applications to change their behaviour.
These devices can take care of application-side pointer on PCM buffer
for their data transmission. As a result, these devices achieve some
advantages. For these devices, drivers need to get feedback about
current position of the pointer from applications when they
queue/dequeue PCM frames on the buffer.

When applications operates with SNDRV_PCM_IOCTL_[READ|WRITE][N|I] command
for ioctl(2) to transmit/receive PCM frames, the above is achieved with
an assist of ALSA PCM core. On the other hand, when applications run with
mmap operations, the above is not achieved. In this case, they should call
ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR after operating any PCM frames on
the buffer. But existent applications are not programmed to do it.

This commit adds a new flag; SNDRV_PCM_INFO_ACK_APPLPTR. When
applications find this flag on 'struct snd_pcm_hw_params.info', they're
expected to perform as the above. When programmed so, they should set
SNDRV_PCM_HW_PARAMS_ACK_APPLPTR flag to the structure in advance. Else,
they're not allowed to do mmap operation. This is for backward
compatibility because existent stuffs in user land don't perform like
the above.

As of this type of device, digital signal processor (DSP) integrated
to Intel's processor is reported. The DSP can prefetch the PCM frames on
the buffer and go to a deep sleep mode till next awakening. The awakening
is done by drivers. When applications deliver information about the number
of handled PCM frames to a corresponding driver in kernel land, then the
driver calculates according to the number, then awakens the DSP from the
sleep.

Additionally, there's two types of devices which potentially receive
advantages from this change. One is packet-oriented drivers, and another
is drivers with ALSA indirect layer. The former may process packets
immediately in process context when handling the feedback. This results
in reduction of audio latency. The latter may copy PCM frames in process
context. This results in reduction CPU time on any interrupt context.

Anyway, this flag requires userspace applications to change their
behaviour. This commit bumps up interface version to v2.0.14.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 include/uapi/sound/asound.h | 11 ++++++++++-
 sound/core/pcm_native.c     | 25 +++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index fd41697cb4d3..ff7bb89e65a6 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;
@@ -268,6 +268,8 @@ 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 */
+/* Driver/hardware acknowledge current appl_ptr for specific purposes. */
+#define SNDRV_PCM_INFO_ACK_APPLPTR	0x00000012
 #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) */
@@ -365,6 +367,13 @@ typedef int snd_pcm_hw_param_t;
 #define SNDRV_PCM_HW_PARAMS_NORESAMPLE	(1<<0)	/* avoid rate resampling */
 #define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER	(1<<1)	/* export buffer */
 #define SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP	(1<<2)	/* disable period wakeups */
+/*
+ * When applications are programmed to execute ioctl(2) with
+ * SNDRV_PCM_IOCTL_SYNC_PTR for notification of current appl_ptr after handling
+ * any frames on mapped PCM buffer for driver/hardware with
+ * SNDRV_PCM_INFO_ACK_APPLPTR, this should be set.
+ */
+#define SNDRV_PCM_HW_PARAMS_ACK_APPLPTR	(1<<3)
 
 struct snd_interval {
 	unsigned int min, max;
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 7e8f3656b695..48772a2bd027 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -249,6 +249,27 @@ static bool hw_support_mmap(struct snd_pcm_substream *substream)
 	return true;
 }
 
+static int constrain_params_with_precondition(struct snd_pcm_substream *substream,
+					      struct snd_pcm_hw_params *params)
+{
+	struct snd_mask *mask;
+
+	/*
+	 * When drivers require applications' cooperation to report current
+	 * appl_ptr and the applications voluntarily perform it, mmap operation
+	 * is allowed. Else, read/write operations are allowed.
+	 */
+	if ((substream->runtime->hw.info & SNDRV_PCM_INFO_ACK_APPLPTR) &&
+	    !(params->flags & SNDRV_PCM_HW_PARAMS_ACK_APPLPTR)) {
+		mask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_ACCESS);
+		snd_mask_reset(mask, SNDRV_PCM_ACCESS_MMAP_INTERLEAVED);
+		snd_mask_reset(mask, SNDRV_PCM_ACCESS_MMAP_NONINTERLEAVED);
+		snd_mask_reset(mask, SNDRV_PCM_ACCESS_MMAP_COMPLEX);
+	}
+
+	return 0;
+}
+
 static int constrain_mask_params(struct snd_pcm_substream *substream,
 				 struct snd_pcm_hw_params *params)
 {
@@ -495,6 +516,10 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 		params->rate_den = 0;
 	}
 
+	err = constrain_params_with_precondition(substream, params);
+	if (err < 0)
+		return err;
+
 	err = constrain_mask_params(substream, params);
 	if (err < 0)
 		return err;
-- 
2.11.0

[alsa-lib][PATCH 1/2] ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to 2.0.14

[Takashi Sakamoto]

In a development period for v4.13, it's clear that there's a type of
devices which demand userspace applications to change their behaviour.
These devices can take care of application-side pointer on PCM buffer
for their data transmission. As a result, these devices achieve some
advantages. For these devices, drivers need to get feedback about
current position of the pointer from applications when they
queue/dequeue PCM frames on the buffer.

When applications operates with SNDRV_PCM_IOCTL_[READ|WRITE][N|I] command
for ioctl(2) to transmit/receive PCM frames, the above is achieved with
an assist of ALSA PCM core. On the other hand, when applications run with
mmap operations, the above is not achieved. In this case, they should call
ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR after operating any PCM frames on
the buffer. But existent applications are not programmed to do it.

This commit adds a new flag; SNDRV_PCM_INFO_ACK_APPLPTR. When
applications find this flag on 'struct snd_pcm_hw_params.info', they're
expected to perform as the above. When programmed so, they should set
SNDRV_PCM_HW_PARAMS_ACK_APPLPTR flag to the structure in advance. Else,
they're not allowed to do mmap operation. This is for backward
compatibility because existent stuffs in user land don't perform like
the above.

As of this type of device, digital signal processor (DSP) integrated
to Intel's processor is reported. The DSP can prefetch the PCM frames on
the buffer and go to a deep sleep mode till next awakening. The awakening
is done by drivers. When applications deliver information about the number
of handled PCM frames to a corresponding driver in kernel land, then the
driver calculates according to the number, then awakens the DSP from the
sleep.

Additionally, there's two types of devices which potentially receive
advantages from this change. One is packet-oriented drivers, and another
is drivers with ALSA indirect layer. The former may process packets
immediately in process context when handling the feedback. This results
in reduction of audio latency. The latter may copy PCM frames in process
context. This results in reduction CPU time on any interrupt context.

Anyway, this flag requires userspace applications to change their
behaviour. This commit bumps up interface version to v2.0.14.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 include/sound/asound.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/sound/asound.h b/include/sound/asound.h
index fb8d7d7e..dc367cf0 100644
--- a/include/sound/asound.h
+++ b/include/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;
@@ -268,6 +268,8 @@ 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 */
+/* Driver/hardware acknowledge current appl_ptr for specific purposes. */
+#define SNDRV_PCM_INFO_ACK_APPLPTR	0x00000012
 #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) */
@@ -365,6 +367,13 @@ typedef int snd_pcm_hw_param_t;
 #define SNDRV_PCM_HW_PARAMS_NORESAMPLE	(1<<0)	/* avoid rate resampling */
 #define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER	(1<<1)	/* export buffer */
 #define SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP	(1<<2)	/* disable period wakeups */
+/*
+ * When applications are programmed to execute ioctl(2) with
+ * SNDRV_PCM_IOCTL_SYNC_PTR for notification of current appl_ptr after handling
+ * any frames on mapped PCM buffer for driver/hardware with
+ * SNDRV_PCM_INFO_ACK_APPLPTR, this should be set.
+ */
+#define SNDRV_PCM_HW_PARAMS_ACK_APPLPTR	(1<<3)
 
 struct snd_interval {
 	unsigned int min, max;
-- 
2.11.0

[alsa-lib][PATCH 2/2] pcm: add support for interface version 2.0.14

[Takashi Sakamoto]

In interface version 2.0.14, ALSA PCM core request feedback to
applications when they operate any PCM frames in mmap operation for a
type of devices with SNDRV_PCM_INFO_ACK_APPLPTR flag.

This commit adds support for this change to keep compatibility for
existent applications which uses alsa-lib API.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 src/pcm/pcm_hw.c | 43 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index 30cd5d0f..880109ad 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -105,6 +105,7 @@ typedef struct {
 	/* for chmap */
 	unsigned int chmap_caps;
 	snd_pcm_chmap_query_t **chmap_override;
+	int ack_applptr;
 } snd_pcm_hw_t;
 
 #define SNDRV_FILE_PCM_STREAM_PLAYBACK		ALSA_DEVICE_DIRECTORY "pcmC%iD%ip"
@@ -128,11 +129,12 @@ struct timespec snd_pcm_hw_fast_tstamp(snd_pcm_t *pcm)
 }
 #endif /* DOC_HIDDEN */
 
-static int sync_ptr1(snd_pcm_hw_t *hw, unsigned int flags)
+static int sync_ptr1(snd_pcm_hw_t *hw, struct snd_pcm_sync_ptr *sync_ptr,
+		     unsigned int flags)
 {
 	int err;
-	hw->sync_ptr->flags = flags;
-	err = ioctl((hw)->fd, SNDRV_PCM_IOCTL_SYNC_PTR, (hw)->sync_ptr);
+	sync_ptr->flags = flags;
+	err = ioctl((hw)->fd, SNDRV_PCM_IOCTL_SYNC_PTR, sync_ptr);
 	if (err < 0) {
 		err = -errno;
 		SYSMSG("SNDRV_PCM_IOCTL_SYNC_PTR failed (%i)", err);
@@ -143,7 +145,7 @@ static int sync_ptr1(snd_pcm_hw_t *hw, unsigned int flags)
 
 static inline int sync_ptr(snd_pcm_hw_t *hw, unsigned int flags)
 {
-	return hw->sync_ptr ? sync_ptr1(hw, flags) : 0;
+	return hw->sync_ptr ? sync_ptr1(hw, hw->sync_ptr, flags) : 0;
 }
 
 static int snd_pcm_hw_clear_timer_queue(snd_pcm_hw_t *hw)
@@ -295,6 +297,14 @@ static int snd_pcm_hw_hw_refine(snd_pcm_t *pcm, snd_pcm_hw_params_t *params)
 			return err;
 	}
 
+	/*
+	 * For drivers with SNDRV_PCM_INFO_ACK_APPLPTR, this has an effect
+	 * to tell that user land can perform MMAP operation as corresponding
+	 * hardware expects. For the other drivers, this has no side-effect.
+	 */
+	if (hw->version >= SNDRV_PROTOCOL_VERSION(2, 0, 14))
+		params->flags |= SNDRV_PCM_INFO_ACK_APPLPTR;
+
 	if (hw_refine_call(hw, params) < 0) {
 		err = -errno;
 		// SYSMSG("SNDRV_PCM_IOCTL_HW_REFINE failed");
@@ -322,6 +332,15 @@ static int snd_pcm_hw_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t * params)
 {
 	snd_pcm_hw_t *hw = pcm->private_data;
 	int err;
+
+	/*
+	 * For drivers with SNDRV_PCM_INFO_ACK_APPLPTR, this has an effect
+	 * to tell that user land can perform MMAP operation as corresponding
+	 * hardware expects. For the other drivers, this has no side-effect.
+	 */
+	if (hw->version >= SNDRV_PROTOCOL_VERSION(2, 0, 14))
+		params->flags |= SNDRV_PCM_INFO_ACK_APPLPTR;
+
 	if (hw_params_call(hw, params) < 0) {
 		err = -errno;
 		SYSMSG("SNDRV_PCM_IOCTL_HW_PARAMS failed (%i)", err);
@@ -337,6 +356,11 @@ static int snd_pcm_hw_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t * params)
 		snd_pcm_set_appl_ptr(pcm, &hw->mmap_control->appl_ptr, hw->fd,
 				     SNDRV_PCM_MMAP_OFFSET_CONTROL);
 	}
+
+	if (hw->version >= SNDRV_PROTOCOL_VERSION(2, 0, 14) &&
+	    (params->info & SNDRV_PCM_INFO_ACK_APPLPTR))
+		hw->ack_applptr = 1;
+
 	return 0;
 }
 
@@ -568,7 +592,8 @@ static int snd_pcm_hw_hwsync(snd_pcm_t *pcm)
 	int fd = hw->fd, err;
 	if (SNDRV_PROTOCOL_VERSION(2, 0, 3) <= hw->version) {
 		if (hw->sync_ptr) {
-			err = sync_ptr1(hw, SNDRV_PCM_SYNC_PTR_HWSYNC);
+			err = sync_ptr1(hw, hw->sync_ptr,
+					SNDRV_PCM_SYNC_PTR_HWSYNC);
 			if (err < 0)
 				return err;
 		} else {
@@ -995,7 +1020,13 @@ static snd_pcm_sframes_t snd_pcm_hw_mmap_commit(snd_pcm_t *pcm,
 	snd_pcm_hw_t *hw = pcm->private_data;
 
 	snd_pcm_mmap_appl_forward(pcm, size);
-	sync_ptr(hw, 0);
+	if (hw->sync_ptr) {
+		sync_ptr1(hw, hw->sync_ptr, 0);
+	} else if (hw->ack_applptr) {
+		struct snd_pcm_sync_ptr sync_ptr = {0};
+		sync_ptr.c.control = *hw->mmap_control;
+		sync_ptr1(hw, &sync_ptr, 0);
+	}
 #ifdef DEBUG_MMAP
 	fprintf(stderr, "appl_forward: hw_ptr = %li, appl_ptr = %li, size = %li\n", *pcm->hw.ptr, *pcm->appl.ptr, size);
 #endif
-- 
2.11.0

Re: [RFC][PATCH] ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to v2.0.14

[Takashi Iwai]

On Sun, 18 Jun 2017 18:49:18 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> This patchset includes commits for both kernel/user land for an issue
> addressed in this message thread[1].
> 
> In a development period for v4.13, it's clear that there's a type of
> devices which demand userspace applications to change their behaviour.
> These devices can take care of application-side pointer on PCM buffer
> for their data transmission. As a result, these devices achieve some
> advantages. For these devices, drivers need to get feedback about
> current position of the pointer from applications when they
> queue/dequeue PCM frames on the buffer.
> 
> ALSA PCM core partly assist the above design. The core can call a
> handler in driver side when applications operate PCM frames by
> ioctl(2) with SNDRV_PCM_IOCTL_[READ|WRITE][N|I] command. However,
> when operating for mapped PCM buffer, it can't assist applications
> for the above design. In this scenario, applications should call
> ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR after handle any PCM frames on
> the buffer.
> 
> This patchset bumps up interface version with new info flag to
> notify the above change to user land developers. This patchset is
> written with below ideas:
> 
> 1. Stuffs in user land can tell their assists for the above design
>    to kernel land by using hw_param flag. When they don't assist,
>    they see on hw_params data that mmap operation is not supported.
> 2. Drivers can acknowledge the above flag. When registering
>    appropriate PCM rules in advance, they can calculate values for
>    each parameters in both cases that stuffs in user land can/cannot
>    assist it. This is useful for packet-oriented drivers to judge
>    utilizing any interrupt context for packetization.
> 3. This is device-specific issue, independently of platform
>    architecture. On x86/ppc/alpha platforms, status/control data of
>    PCM substream is still available in process' VMA of application via
>    page frame mapping.
> 
> [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/121438.html
> 
> For kernel land:
> Takashi Sakamoto (1):
>   ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to
>     2.0.14
> 
>  include/uapi/sound/asound.h | 11 ++++++++++-
>  sound/core/pcm_native.c     | 25 +++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> For user land:
> Takashi Sakamoto (2):
>   ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to
>     2.0.14
>   pcm: add support for interface version 2.0.14
> 
>  include/sound/asound.h | 11 ++++++++++-
>  src/pcm/pcm_hw.c       | 43 +++++++++++++++++++++++++++++++++++++------
>  2 files changed, 47 insertions(+), 7 deletions(-)

Well, these mandate both kernel and user-side changes, so if you run
the old applications with old alsa-lib, it can't work, right?

A similar patchset is already in my local queue (my version called it
differently, SNDRV_PCM_INFO_SYNC_APPLPTR :), but it wasn't submitted
yet because we wanted to fix the issue without affecting alsa-lib at
first.  Do you have the patch for that?


thanks,

Takashi

Re: [PATCH 1/1] ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to 2.0.14

[Takashi Iwai]

On Sun, 18 Jun 2017 18:49:19 +0200,
Takashi Sakamoto wrote:
> 
> In a development period for v4.13, it's clear that there's a type of
> devices which demand userspace applications to change their behaviour.
> These devices can take care of application-side pointer on PCM buffer
> for their data transmission. As a result, these devices achieve some
> advantages. For these devices, drivers need to get feedback about
> current position of the pointer from applications when they
> queue/dequeue PCM frames on the buffer.
> 
> When applications operates with SNDRV_PCM_IOCTL_[READ|WRITE][N|I] command
> for ioctl(2) to transmit/receive PCM frames, the above is achieved with
> an assist of ALSA PCM core. On the other hand, when applications run with
> mmap operations, the above is not achieved. In this case, they should call
> ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR after operating any PCM frames on
> the buffer. But existent applications are not programmed to do it.

... they are programmed to behave so, when the PCM status mmap fails.


> This commit adds a new flag; SNDRV_PCM_INFO_ACK_APPLPTR. When
> applications find this flag on 'struct snd_pcm_hw_params.info', they're
> expected to perform as the above. When programmed so, they should set
> SNDRV_PCM_HW_PARAMS_ACK_APPLPTR flag to the structure in advance. Else,
> they're not allowed to do mmap operation. This is for backward
> compatibility because existent stuffs in user land don't perform like
> the above.
> 
> As of this type of device, digital signal processor (DSP) integrated
> to Intel's processor is reported. The DSP can prefetch the PCM frames on
> the buffer and go to a deep sleep mode till next awakening. The awakening
> is done by drivers. When applications deliver information about the number
> of handled PCM frames to a corresponding driver in kernel land, then the
> driver calculates according to the number, then awakens the DSP from the
> sleep.
> 
> Additionally, there's two types of devices which potentially receive
> advantages from this change. One is packet-oriented drivers, and another
> is drivers with ALSA indirect layer. The former may process packets
> immediately in process context when handling the feedback. This results
> in reduction of audio latency. The latter may copy PCM frames in process
> context. This results in reduction CPU time on any interrupt context.
> 
> Anyway, this flag requires userspace applications to change their
> behaviour. This commit bumps up interface version to v2.0.14.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  include/uapi/sound/asound.h | 11 ++++++++++-
>  sound/core/pcm_native.c     | 25 +++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index fd41697cb4d3..ff7bb89e65a6 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;
> @@ -268,6 +268,8 @@ 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 */
> +/* Driver/hardware acknowledge current appl_ptr for specific purposes. */
> +#define SNDRV_PCM_INFO_ACK_APPLPTR	0x00000012
>  #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) */
> @@ -365,6 +367,13 @@ typedef int snd_pcm_hw_param_t;
>  #define SNDRV_PCM_HW_PARAMS_NORESAMPLE	(1<<0)	/* avoid rate resampling */
>  #define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER	(1<<1)	/* export buffer */
>  #define SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP	(1<<2)	/* disable period wakeups */
> +/*
> + * When applications are programmed to execute ioctl(2) with
> + * SNDRV_PCM_IOCTL_SYNC_PTR for notification of current appl_ptr after handling
> + * any frames on mapped PCM buffer for driver/hardware with
> + * SNDRV_PCM_INFO_ACK_APPLPTR, this should be set.
> + */
> +#define SNDRV_PCM_HW_PARAMS_ACK_APPLPTR	(1<<3)
>  
>  struct snd_interval {
>  	unsigned int min, max;
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 7e8f3656b695..48772a2bd027 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -249,6 +249,27 @@ static bool hw_support_mmap(struct snd_pcm_substream *substream)
>  	return true;
>  }
>  
> +static int constrain_params_with_precondition(struct snd_pcm_substream *substream,
> +					      struct snd_pcm_hw_params *params)
> +{
> +	struct snd_mask *mask;
> +
> +	/*
> +	 * When drivers require applications' cooperation to report current
> +	 * appl_ptr and the applications voluntarily perform it, mmap operation
> +	 * is allowed. Else, read/write operations are allowed.
> +	 */
> +	if ((substream->runtime->hw.info & SNDRV_PCM_INFO_ACK_APPLPTR) &&
> +	    !(params->flags & SNDRV_PCM_HW_PARAMS_ACK_APPLPTR)) {
> +		mask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_ACCESS);
> +		snd_mask_reset(mask, SNDRV_PCM_ACCESS_MMAP_INTERLEAVED);
> +		snd_mask_reset(mask, SNDRV_PCM_ACCESS_MMAP_NONINTERLEAVED);
> +		snd_mask_reset(mask, SNDRV_PCM_ACCESS_MMAP_COMPLEX);

Well, it's only the lack of sync_ptr, and the mmap of the ring buffer
itself is fine.  Disabling the ring-buffer mmap is a much bigger
hammer and the significant performance penalty than disabling PCM
control/status mmap.


thanks,

Takashi

Re: [RFC][PATCH] ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to v2.0.14

[Takashi Iwai]

On Sun, 18 Jun 2017 19:08:05 +0200,
Takashi Iwai wrote:
> 
> On Sun, 18 Jun 2017 18:49:18 +0200,
> Takashi Sakamoto wrote:
> > 
> > Hi,
> > 
> > This patchset includes commits for both kernel/user land for an issue
> > addressed in this message thread[1].
> > 
> > In a development period for v4.13, it's clear that there's a type of
> > devices which demand userspace applications to change their behaviour.
> > These devices can take care of application-side pointer on PCM buffer
> > for their data transmission. As a result, these devices achieve some
> > advantages. For these devices, drivers need to get feedback about
> > current position of the pointer from applications when they
> > queue/dequeue PCM frames on the buffer.
> > 
> > ALSA PCM core partly assist the above design. The core can call a
> > handler in driver side when applications operate PCM frames by
> > ioctl(2) with SNDRV_PCM_IOCTL_[READ|WRITE][N|I] command. However,
> > when operating for mapped PCM buffer, it can't assist applications
> > for the above design. In this scenario, applications should call
> > ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR after handle any PCM frames on
> > the buffer.
> > 
> > This patchset bumps up interface version with new info flag to
> > notify the above change to user land developers. This patchset is
> > written with below ideas:
> > 
> > 1. Stuffs in user land can tell their assists for the above design
> >    to kernel land by using hw_param flag. When they don't assist,
> >    they see on hw_params data that mmap operation is not supported.
> > 2. Drivers can acknowledge the above flag. When registering
> >    appropriate PCM rules in advance, they can calculate values for
> >    each parameters in both cases that stuffs in user land can/cannot
> >    assist it. This is useful for packet-oriented drivers to judge
> >    utilizing any interrupt context for packetization.
> > 3. This is device-specific issue, independently of platform
> >    architecture. On x86/ppc/alpha platforms, status/control data of
> >    PCM substream is still available in process' VMA of application via
> >    page frame mapping.
> > 
> > [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/121438.html
> > 
> > For kernel land:
> > Takashi Sakamoto (1):
> >   ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to
> >     2.0.14
> > 
> >  include/uapi/sound/asound.h | 11 ++++++++++-
> >  sound/core/pcm_native.c     | 25 +++++++++++++++++++++++++
> >  2 files changed, 35 insertions(+), 1 deletion(-)
> > 
> > For user land:
> > Takashi Sakamoto (2):
> >   ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to
> >     2.0.14
> >   pcm: add support for interface version 2.0.14
> > 
> >  include/sound/asound.h | 11 ++++++++++-
> >  src/pcm/pcm_hw.c       | 43 +++++++++++++++++++++++++++++++++++++------
> >  2 files changed, 47 insertions(+), 7 deletions(-)
> 
> Well, these mandate both kernel and user-side changes, so if you run
> the old applications with old alsa-lib, it can't work, right?

I see now that you achieved it by dropping the whole mmap.

This is a very big concern from the performance POV.  It merely kills
the whole merit of adding the appl_ptr notification (e.g. PA switches
to a completely different mode), so this is likely no-go, sorry.


thanks,

Takashi

Re: [PATCH 1/1] ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to 2.0.14

[Takashi Sakamoto]

On Jun 19 2017 02:15, Takashi Iwai wrote:
> On Sun, 18 Jun 2017 18:49:19 +0200,
> Takashi Sakamoto wrote:
>>
>> In a development period for v4.13, it's clear that there's a type of
>> devices which demand userspace applications to change their behaviour.
>> These devices can take care of application-side pointer on PCM buffer
>> for their data transmission. As a result, these devices achieve some
>> advantages. For these devices, drivers need to get feedback about
>> current position of the pointer from applications when they
>> queue/dequeue PCM frames on the buffer.
>>
>> When applications operates with SNDRV_PCM_IOCTL_[READ|WRITE][N|I] command
>> for ioctl(2) to transmit/receive PCM frames, the above is achieved with
>> an assist of ALSA PCM core. On the other hand, when applications run with
>> mmap operations, the above is not achieved. In this case, they should call
>> ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR after operating any PCM frames on
>> the buffer. But existent applications are not programmed to do it.
> 
> ... they are programmed to behave so, when the PCM status mmap fails.

As I said, the solution to disable mmap for status/control data has 
different scope from this issue.

>> This commit adds a new flag; SNDRV_PCM_INFO_ACK_APPLPTR. When
>> applications find this flag on 'struct snd_pcm_hw_params.info', they're
>> expected to perform as the above. When programmed so, they should set
>> SNDRV_PCM_HW_PARAMS_ACK_APPLPTR flag to the structure in advance. Else,
>> they're not allowed to do mmap operation. This is for backward
>> compatibility because existent stuffs in user land don't perform like
>> the above.
>>
>> As of this type of device, digital signal processor (DSP) integrated
>> to Intel's processor is reported. The DSP can prefetch the PCM frames on
>> the buffer and go to a deep sleep mode till next awakening. The awakening
>> is done by drivers. When applications deliver information about the number
>> of handled PCM frames to a corresponding driver in kernel land, then the
>> driver calculates according to the number, then awakens the DSP from the
>> sleep.
>>
>> Additionally, there's two types of devices which potentially receive
>> advantages from this change. One is packet-oriented drivers, and another
>> is drivers with ALSA indirect layer. The former may process packets
>> immediately in process context when handling the feedback. This results
>> in reduction of audio latency. The latter may copy PCM frames in process
>> context. This results in reduction CPU time on any interrupt context.
>>
>> Anyway, this flag requires userspace applications to change their
>> behaviour. This commit bumps up interface version to v2.0.14.
>>
>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>> ---
>>   include/uapi/sound/asound.h | 11 ++++++++++-
>>   sound/core/pcm_native.c     | 25 +++++++++++++++++++++++++
>>   2 files changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>> index fd41697cb4d3..ff7bb89e65a6 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;
>> @@ -268,6 +268,8 @@ 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 */
>> +/* Driver/hardware acknowledge current appl_ptr for specific purposes. */
>> +#define SNDRV_PCM_INFO_ACK_APPLPTR	0x00000012
>>   #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) */
>> @@ -365,6 +367,13 @@ typedef int snd_pcm_hw_param_t;
>>   #define SNDRV_PCM_HW_PARAMS_NORESAMPLE	(1<<0)	/* avoid rate resampling */
>>   #define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER	(1<<1)	/* export buffer */
>>   #define SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP	(1<<2)	/* disable period wakeups */
>> +/*
>> + * When applications are programmed to execute ioctl(2) with
>> + * SNDRV_PCM_IOCTL_SYNC_PTR for notification of current appl_ptr after handling
>> + * any frames on mapped PCM buffer for driver/hardware with
>> + * SNDRV_PCM_INFO_ACK_APPLPTR, this should be set.
>> + */
>> +#define SNDRV_PCM_HW_PARAMS_ACK_APPLPTR	(1<<3)
>>   
>>   struct snd_interval {
>>   	unsigned int min, max;
>> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
>> index 7e8f3656b695..48772a2bd027 100644
>> --- a/sound/core/pcm_native.c
>> +++ b/sound/core/pcm_native.c
>> @@ -249,6 +249,27 @@ static bool hw_support_mmap(struct snd_pcm_substream *substream)
>>   	return true;
>>   }
>>   
>> +static int constrain_params_with_precondition(struct snd_pcm_substream *substream,
>> +					      struct snd_pcm_hw_params *params)
>> +{
>> +	struct snd_mask *mask;
>> +
>> +	/*
>> +	 * When drivers require applications' cooperation to report current
>> +	 * appl_ptr and the applications voluntarily perform it, mmap operation
>> +	 * is allowed. Else, read/write operations are allowed.
>> +	 */
>> +	if ((substream->runtime->hw.info & SNDRV_PCM_INFO_ACK_APPLPTR) &&
>> +	    !(params->flags & SNDRV_PCM_HW_PARAMS_ACK_APPLPTR)) {
>> +		mask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_ACCESS);
>> +		snd_mask_reset(mask, SNDRV_PCM_ACCESS_MMAP_INTERLEAVED);
>> +		snd_mask_reset(mask, SNDRV_PCM_ACCESS_MMAP_NONINTERLEAVED);
>> +		snd_mask_reset(mask, SNDRV_PCM_ACCESS_MMAP_COMPLEX);
> 
> Well, it's only the lack of sync_ptr, and the mmap of the ring buffer
> itself is fine.  Disabling the ring-buffer mmap is a much bigger
> hammer and the significant performance penalty than disabling PCM
> control/status mmap.

No. Old stuffs don't satisfy requirements of the new devices/drivers. 
Constraint to read/write operation is fair enough and not penalty 
because programs are written so that they cannot assist the new 
devices/drivers.

Well-programmed applications can handle a case in which mmap operation 
is not allowed. This patchset is enough reasonable.


Regards

Takashi Sakamoto

Re: [[RFC][PATCH] ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to v2.0.14

[Takashi Sakamoto]

On 2017年06月19日 02:08, Takashi Iwai wrote:
> On Sun, 18 Jun 2017 18:49:18 +0200,
> Takashi Sakamoto wrote:
>>
>> Hi,
>>
>> This patchset includes commits for both kernel/user land for an issue
>> addressed in this message thread[1].
>>
>> In a development period for v4.13, it's clear that there's a type of
>> devices which demand userspace applications to change their behaviour.
>> These devices can take care of application-side pointer on PCM buffer
>> for their data transmission. As a result, these devices achieve some
>> advantages. For these devices, drivers need to get feedback about
>> current position of the pointer from applications when they
>> queue/dequeue PCM frames on the buffer.
>>
>> ALSA PCM core partly assist the above design. The core can call a
>> handler in driver side when applications operate PCM frames by
>> ioctl(2) with SNDRV_PCM_IOCTL_[READ|WRITE][N|I] command. However,
>> when operating for mapped PCM buffer, it can't assist applications
>> for the above design. In this scenario, applications should call
>> ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR after handle any PCM frames on
>> the buffer.
>>
>> This patchset bumps up interface version with new info flag to
>> notify the above change to user land developers. This patchset is
>> written with below ideas:
>>
>> 1. Stuffs in user land can tell their assists for the above design
>>     to kernel land by using hw_param flag. When they don't assist,
>>     they see on hw_params data that mmap operation is not supported.
>> 2. Drivers can acknowledge the above flag. When registering
>>     appropriate PCM rules in advance, they can calculate values for
>>     each parameters in both cases that stuffs in user land can/cannot
>>     assist it. This is useful for packet-oriented drivers to judge
>>     utilizing any interrupt context for packetization.
>> 3. This is device-specific issue, independently of platform
>>     architecture. On x86/ppc/alpha platforms, status/control data of
>>     PCM substream is still available in process' VMA of application via
>>     page frame mapping.
>>
>> [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/121438.html
>>
>> For kernel land:
>> Takashi Sakamoto (1):
>>    ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to
>>      2.0.14
>>
>>   include/uapi/sound/asound.h | 11 ++++++++++-
>>   sound/core/pcm_native.c     | 25 +++++++++++++++++++++++++
>>   2 files changed, 35 insertions(+), 1 deletion(-)
>>
>> For user land:
>> Takashi Sakamoto (2):
>>    ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to
>>      2.0.14
>>    pcm: add support for interface version 2.0.14
>>
>>   include/sound/asound.h | 11 ++++++++++-
>>   src/pcm/pcm_hw.c       | 43 +++++++++++++++++++++++++++++++++++++------
>>   2 files changed, 47 insertions(+), 7 deletions(-)
> 
> Well, these mandate both kernel and user-side changes, so if you run
> the old applications with old alsa-lib, it can't work, right?

Applications still works well because they receive proper set of 
hw_params. They works as programmed, thus for the new devices/drivers 
they work without mmap operation.

> A similar patchset is already in my local queue (my version called it
> differently, SNDRV_PCM_INFO_SYNC_APPLPTR :), but it wasn't submitted
> yet because we wanted to fix the issue without affecting alsa-lib at
> first.  Do you have the patch for that?

I will not have this direction, past and future, because this issue 
demands userland stuffs to perform as the new drivers/devices expects, 
thus userland stuffs _should_ be changed.

# When I investigated name of the flag, I dropped
# 'SNDRV_PCM_INFO_SYNC_APPLPTR' at first, because hardware doesn't
# need to synchronize to applptr always. It's enough for hardwares to
# get it when requires. Drivers and applications assist it.


Regards

Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/1] ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to 2.0.14

[Takashi Iwai]

On Mon, 19 Jun 2017 14:26:32 +0200,
Takashi Sakamoto wrote:
> 
> On Jun 19 2017 02:15, Takashi Iwai wrote:
> > On Sun, 18 Jun 2017 18:49:19 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> In a development period for v4.13, it's clear that there's a type of
> >> devices which demand userspace applications to change their behaviour.
> >> These devices can take care of application-side pointer on PCM buffer
> >> for their data transmission. As a result, these devices achieve some
> >> advantages. For these devices, drivers need to get feedback about
> >> current position of the pointer from applications when they
> >> queue/dequeue PCM frames on the buffer.
> >>
> >> When applications operates with SNDRV_PCM_IOCTL_[READ|WRITE][N|I] command
> >> for ioctl(2) to transmit/receive PCM frames, the above is achieved with
> >> an assist of ALSA PCM core. On the other hand, when applications run with
> >> mmap operations, the above is not achieved. In this case, they should call
> >> ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR after operating any PCM frames on
> >> the buffer. But existent applications are not programmed to do it.
> >
> > ... they are programmed to behave so, when the PCM status mmap fails.
> 
> As I said, the solution to disable mmap for status/control data has
> different scope from this issue.

The mechanism is used not only for coherency mmap issues, but already
deployed for other purpose in the situation that require the explicit
status/contorl copy.  It's already generically used for x86.


> >> This commit adds a new flag; SNDRV_PCM_INFO_ACK_APPLPTR. When
> >> applications find this flag on 'struct snd_pcm_hw_params.info', they're
> >> expected to perform as the above. When programmed so, they should set
> >> SNDRV_PCM_HW_PARAMS_ACK_APPLPTR flag to the structure in advance. Else,
> >> they're not allowed to do mmap operation. This is for backward
> >> compatibility because existent stuffs in user land don't perform like
> >> the above.
> >>
> >> As of this type of device, digital signal processor (DSP) integrated
> >> to Intel's processor is reported. The DSP can prefetch the PCM frames on
> >> the buffer and go to a deep sleep mode till next awakening. The awakening
> >> is done by drivers. When applications deliver information about the number
> >> of handled PCM frames to a corresponding driver in kernel land, then the
> >> driver calculates according to the number, then awakens the DSP from the
> >> sleep.
> >>
> >> Additionally, there's two types of devices which potentially receive
> >> advantages from this change. One is packet-oriented drivers, and another
> >> is drivers with ALSA indirect layer. The former may process packets
> >> immediately in process context when handling the feedback. This results
> >> in reduction of audio latency. The latter may copy PCM frames in process
> >> context. This results in reduction CPU time on any interrupt context.
> >>
> >> Anyway, this flag requires userspace applications to change their
> >> behaviour. This commit bumps up interface version to v2.0.14.
> >>
> >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> >> ---
> >>   include/uapi/sound/asound.h | 11 ++++++++++-
> >>   sound/core/pcm_native.c     | 25 +++++++++++++++++++++++++
> >>   2 files changed, 35 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> >> index fd41697cb4d3..ff7bb89e65a6 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;
> >> @@ -268,6 +268,8 @@ 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 */
> >> +/* Driver/hardware acknowledge current appl_ptr for specific purposes. */
> >> +#define SNDRV_PCM_INFO_ACK_APPLPTR	0x00000012
> >>   #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) */
> >> @@ -365,6 +367,13 @@ typedef int snd_pcm_hw_param_t;
> >>   #define SNDRV_PCM_HW_PARAMS_NORESAMPLE	(1<<0)	/* avoid rate resampling */
> >>   #define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER	(1<<1)	/* export buffer */
> >>   #define SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP	(1<<2)	/* disable period wakeups */
> >> +/*
> >> + * When applications are programmed to execute ioctl(2) with
> >> + * SNDRV_PCM_IOCTL_SYNC_PTR for notification of current appl_ptr after handling
> >> + * any frames on mapped PCM buffer for driver/hardware with
> >> + * SNDRV_PCM_INFO_ACK_APPLPTR, this should be set.
> >> + */
> >> +#define SNDRV_PCM_HW_PARAMS_ACK_APPLPTR	(1<<3)
> >>     struct snd_interval {
> >>   	unsigned int min, max;
> >> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> >> index 7e8f3656b695..48772a2bd027 100644
> >> --- a/sound/core/pcm_native.c
> >> +++ b/sound/core/pcm_native.c
> >> @@ -249,6 +249,27 @@ static bool hw_support_mmap(struct snd_pcm_substream *substream)
> >>   	return true;
> >>   }
> >>   +static int constrain_params_with_precondition(struct
> >> snd_pcm_substream *substream,
> >> +					      struct snd_pcm_hw_params *params)
> >> +{
> >> +	struct snd_mask *mask;
> >> +
> >> +	/*
> >> +	 * When drivers require applications' cooperation to report current
> >> +	 * appl_ptr and the applications voluntarily perform it, mmap operation
> >> +	 * is allowed. Else, read/write operations are allowed.
> >> +	 */
> >> +	if ((substream->runtime->hw.info & SNDRV_PCM_INFO_ACK_APPLPTR) &&
> >> +	    !(params->flags & SNDRV_PCM_HW_PARAMS_ACK_APPLPTR)) {
> >> +		mask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_ACCESS);
> >> +		snd_mask_reset(mask, SNDRV_PCM_ACCESS_MMAP_INTERLEAVED);
> >> +		snd_mask_reset(mask, SNDRV_PCM_ACCESS_MMAP_NONINTERLEAVED);
> >> +		snd_mask_reset(mask, SNDRV_PCM_ACCESS_MMAP_COMPLEX);
> >
> > Well, it's only the lack of sync_ptr, and the mmap of the ring buffer
> > itself is fine.  Disabling the ring-buffer mmap is a much bigger
> > hammer and the significant performance penalty than disabling PCM
> > control/status mmap.
> 
> No. Old stuffs don't satisfy requirements of the new
> devices/drivers. Constraint to read/write operation is fair enough and
> not penalty because programs are written so that they cannot assist
> the new devices/drivers.

The disablement of mmap *CAN* assist the new devices/drivers.
"they cannot"?  Well, it works as is.

> Well-programmed applications can handle a case in which mmap operation
> is not allowed. This patchset is enough reasonable.

Oh, this is a big "NO".  The behavior difference between mmap and
read/write is enormous, and we can't blame the application to be
broken if it doesn't support both.

That is, the probability of breakage is much bigger than the
disablement of status/control mmap, which is absorbed already in the
existing alsa-lib code, and it's been proven to work for decades.


thanks,

Takashi

Re: [[RFC][PATCH] ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to v2.0.14

[Takashi Iwai]

On Mon, 19 Jun 2017 14:33:50 +0200,
Takashi Sakamoto wrote:
> 
> On 2017年06月19日 02:08, Takashi Iwai wrote:
> > On Sun, 18 Jun 2017 18:49:18 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> Hi,
> >>
> >> This patchset includes commits for both kernel/user land for an issue
> >> addressed in this message thread[1].
> >>
> >> In a development period for v4.13, it's clear that there's a type of
> >> devices which demand userspace applications to change their behaviour.
> >> These devices can take care of application-side pointer on PCM buffer
> >> for their data transmission. As a result, these devices achieve some
> >> advantages. For these devices, drivers need to get feedback about
> >> current position of the pointer from applications when they
> >> queue/dequeue PCM frames on the buffer.
> >>
> >> ALSA PCM core partly assist the above design. The core can call a
> >> handler in driver side when applications operate PCM frames by
> >> ioctl(2) with SNDRV_PCM_IOCTL_[READ|WRITE][N|I] command. However,
> >> when operating for mapped PCM buffer, it can't assist applications
> >> for the above design. In this scenario, applications should call
> >> ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR after handle any PCM frames on
> >> the buffer.
> >>
> >> This patchset bumps up interface version with new info flag to
> >> notify the above change to user land developers. This patchset is
> >> written with below ideas:
> >>
> >> 1. Stuffs in user land can tell their assists for the above design
> >>     to kernel land by using hw_param flag. When they don't assist,
> >>     they see on hw_params data that mmap operation is not supported.
> >> 2. Drivers can acknowledge the above flag. When registering
> >>     appropriate PCM rules in advance, they can calculate values for
> >>     each parameters in both cases that stuffs in user land can/cannot
> >>     assist it. This is useful for packet-oriented drivers to judge
> >>     utilizing any interrupt context for packetization.
> >> 3. This is device-specific issue, independently of platform
> >>     architecture. On x86/ppc/alpha platforms, status/control data of
> >>     PCM substream is still available in process' VMA of application via
> >>     page frame mapping.
> >>
> >> [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/121438.html
> >>
> >> For kernel land:
> >> Takashi Sakamoto (1):
> >>    ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to
> >>      2.0.14
> >>
> >>   include/uapi/sound/asound.h | 11 ++++++++++-
> >>   sound/core/pcm_native.c     | 25 +++++++++++++++++++++++++
> >>   2 files changed, 35 insertions(+), 1 deletion(-)
> >>
> >> For user land:
> >> Takashi Sakamoto (2):
> >>    ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to
> >>      2.0.14
> >>    pcm: add support for interface version 2.0.14
> >>
> >>   include/sound/asound.h | 11 ++++++++++-
> >>   src/pcm/pcm_hw.c       | 43 +++++++++++++++++++++++++++++++++++++------
> >>   2 files changed, 47 insertions(+), 7 deletions(-)
> >
> > Well, these mandate both kernel and user-side changes, so if you run
> > the old applications with old alsa-lib, it can't work, right?
> 
> Applications still works well because they receive proper set of
> hw_params. They works as programmed, thus for the new devices/drivers
> they work without mmap operation.

This solution is obviously worse from most of perspectives (the
performance, the possible regressions).


> > A similar patchset is already in my local queue (my version called it
> > differently, SNDRV_PCM_INFO_SYNC_APPLPTR :), but it wasn't submitted
> > yet because we wanted to fix the issue without affecting alsa-lib at
> > first.  Do you have the patch for that?
> 
> I will not have this direction, past and future, because this issue
> demands userland stuffs to perform as the new drivers/devices expects,
> thus userland stuffs _should_ be changed.

Have you ever heard of a distribution named "Debian"?  Wait for
another 10 years until the fix in a newer alsa-lib version will be
distributed as stable :)


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC][PATCH] ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to v2.0.14

[Takashi Sakamoto]

On Jun 19 2017 02:17, Takashi Iwai wrote:
> On Sun, 18 Jun 2017 19:08:05 +0200,
> Takashi Iwai wrote:
>>
>> On Sun, 18 Jun 2017 18:49:18 +0200,
>> Takashi Sakamoto wrote:
>>>
>>> Hi,
>>>
>>> This patchset includes commits for both kernel/user land for an issue
>>> addressed in this message thread[1].
>>>
>>> In a development period for v4.13, it's clear that there's a type of
>>> devices which demand userspace applications to change their behaviour.
>>> These devices can take care of application-side pointer on PCM buffer
>>> for their data transmission. As a result, these devices achieve some
>>> advantages. For these devices, drivers need to get feedback about
>>> current position of the pointer from applications when they
>>> queue/dequeue PCM frames on the buffer.
>>>
>>> ALSA PCM core partly assist the above design. The core can call a
>>> handler in driver side when applications operate PCM frames by
>>> ioctl(2) with SNDRV_PCM_IOCTL_[READ|WRITE][N|I] command. However,
>>> when operating for mapped PCM buffer, it can't assist applications
>>> for the above design. In this scenario, applications should call
>>> ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR after handle any PCM frames on
>>> the buffer.
>>>
>>> This patchset bumps up interface version with new info flag to
>>> notify the above change to user land developers. This patchset is
>>> written with below ideas:
>>>
>>> 1. Stuffs in user land can tell their assists for the above design
>>>     to kernel land by using hw_param flag. When they don't assist,
>>>     they see on hw_params data that mmap operation is not supported.
>>> 2. Drivers can acknowledge the above flag. When registering
>>>     appropriate PCM rules in advance, they can calculate values for
>>>     each parameters in both cases that stuffs in user land can/cannot
>>>     assist it. This is useful for packet-oriented drivers to judge
>>>     utilizing any interrupt context for packetization.
>>> 3. This is device-specific issue, independently of platform
>>>     architecture. On x86/ppc/alpha platforms, status/control data of
>>>     PCM substream is still available in process' VMA of application via
>>>     page frame mapping.
>>>
>>> [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/121438.html
>>>
>>> For kernel land:
>>> Takashi Sakamoto (1):
>>>    ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to
>>>      2.0.14
>>>
>>>   include/uapi/sound/asound.h | 11 ++++++++++-
>>>   sound/core/pcm_native.c     | 25 +++++++++++++++++++++++++
>>>   2 files changed, 35 insertions(+), 1 deletion(-)
>>>
>>> For user land:
>>> Takashi Sakamoto (2):
>>>    ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to
>>>      2.0.14
>>>    pcm: add support for interface version 2.0.14
>>>
>>>   include/sound/asound.h | 11 ++++++++++-
>>>   src/pcm/pcm_hw.c       | 43 +++++++++++++++++++++++++++++++++++++------
>>>   2 files changed, 47 insertions(+), 7 deletions(-)
>>
>> Well, these mandate both kernel and user-side changes, so if you run
>> the old applications with old alsa-lib, it can't work, right?
> 
> I see now that you achieved it by dropping the whole mmap.

Just for drivers/hardwares with the new flag. I won't disable page frame 
mapping of control/status data indiscriminately for drivers with .ack 
callback.

> This is a very big concern from the performance POV.  It merely kills
> the whole merit of adding the appl_ptr notification (e.g. PA switches
> to a completely different mode), so this is likely no-go, sorry.

Old stuffs were not programmed to assist the new devices/drivers. It's 
quite natural to get a constrain to its working mode for several 
drivers/devices.

I focus on describing hardware/driver capabilities in interface for user 
land reasonably, instead of introducing them ex post facto.

...Well, I've already done for my proposal to this issue. Nothing left. 
I think you can understand what I insists for this issue and my concerns 
about your patch.

If you updates change log in the patch and post it, I'm willing to 
review it. I don't oppose it so hard (I understand the intention 
correctly), however it's important to pay enough attention to interfaces 
for applications. If the new stuffs bring changes to interface and 
applications get affects, we, developers for kernel land, should record 
and explain about it.


Regards

Takashi Sakamoto

Re: [RFC][PATCH] ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to v2.0.14

[Takashi Iwai]

On Mon, 19 Jun 2017 14:54:12 +0200,
Takashi Sakamoto wrote:
> 
> On Jun 19 2017 02:17, Takashi Iwai wrote:
> > On Sun, 18 Jun 2017 19:08:05 +0200,
> > Takashi Iwai wrote:
> >>
> >> On Sun, 18 Jun 2017 18:49:18 +0200,
> >> Takashi Sakamoto wrote:
> >>>
> >>> Hi,
> >>>
> >>> This patchset includes commits for both kernel/user land for an issue
> >>> addressed in this message thread[1].
> >>>
> >>> In a development period for v4.13, it's clear that there's a type of
> >>> devices which demand userspace applications to change their behaviour.
> >>> These devices can take care of application-side pointer on PCM buffer
> >>> for their data transmission. As a result, these devices achieve some
> >>> advantages. For these devices, drivers need to get feedback about
> >>> current position of the pointer from applications when they
> >>> queue/dequeue PCM frames on the buffer.
> >>>
> >>> ALSA PCM core partly assist the above design. The core can call a
> >>> handler in driver side when applications operate PCM frames by
> >>> ioctl(2) with SNDRV_PCM_IOCTL_[READ|WRITE][N|I] command. However,
> >>> when operating for mapped PCM buffer, it can't assist applications
> >>> for the above design. In this scenario, applications should call
> >>> ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR after handle any PCM frames on
> >>> the buffer.
> >>>
> >>> This patchset bumps up interface version with new info flag to
> >>> notify the above change to user land developers. This patchset is
> >>> written with below ideas:
> >>>
> >>> 1. Stuffs in user land can tell their assists for the above design
> >>>     to kernel land by using hw_param flag. When they don't assist,
> >>>     they see on hw_params data that mmap operation is not supported.
> >>> 2. Drivers can acknowledge the above flag. When registering
> >>>     appropriate PCM rules in advance, they can calculate values for
> >>>     each parameters in both cases that stuffs in user land can/cannot
> >>>     assist it. This is useful for packet-oriented drivers to judge
> >>>     utilizing any interrupt context for packetization.
> >>> 3. This is device-specific issue, independently of platform
> >>>     architecture. On x86/ppc/alpha platforms, status/control data of
> >>>     PCM substream is still available in process' VMA of application via
> >>>     page frame mapping.
> >>>
> >>> [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/121438.html
> >>>
> >>> For kernel land:
> >>> Takashi Sakamoto (1):
> >>>    ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to
> >>>      2.0.14
> >>>
> >>>   include/uapi/sound/asound.h | 11 ++++++++++-
> >>>   sound/core/pcm_native.c     | 25 +++++++++++++++++++++++++
> >>>   2 files changed, 35 insertions(+), 1 deletion(-)
> >>>
> >>> For user land:
> >>> Takashi Sakamoto (2):
> >>>    ALSA: pcm: introduce ACK_APPLPTR flag and bump up interface version to
> >>>      2.0.14
> >>>    pcm: add support for interface version 2.0.14
> >>>
> >>>   include/sound/asound.h | 11 ++++++++++-
> >>>   src/pcm/pcm_hw.c       | 43 +++++++++++++++++++++++++++++++++++++------
> >>>   2 files changed, 47 insertions(+), 7 deletions(-)
> >>
> >> Well, these mandate both kernel and user-side changes, so if you run
> >> the old applications with old alsa-lib, it can't work, right?
> >
> > I see now that you achieved it by dropping the whole mmap.
> 
> Just for drivers/hardwares with the new flag. I won't disable page
> frame mapping of control/status data indiscriminately for drivers with
> .ack callback.
> 
> > This is a very big concern from the performance POV.  It merely kills
> > the whole merit of adding the appl_ptr notification (e.g. PA switches
> > to a completely different mode), so this is likely no-go, sorry.
> 
> Old stuffs were not programmed to assist the new devices/drivers. It's
> quite natural to get a constrain to its working mode for several
> drivers/devices.

If there is no other way, I'd agree with such a change.  But in this
case, this isn't.

> I focus on describing hardware/driver capabilities in interface for
> user land reasonably, instead of introducing them ex post facto.
> 
> ...Well, I've already done for my proposal to this issue. Nothing
> left. I think you can understand what I insists for this issue and my
> concerns about your patch.

I'm not a big fan of my proposed change, either.  It's not very
intuitive if you don't know of the mechanism behind the scene.
But, judging from the practice (as a similar technique already
deployed, also on x86 for long time), it's a sensible solution.

> If you updates change log in the patch and post it, I'm willing to
> review it. I don't oppose it so hard (I understand the intention
> correctly), however it's important to pay enough attention to
> interfaces for applications. If the new stuffs bring changes to
> interface and applications get affects, we, developers for kernel
> land, should record and explain about it.

Sure, the revised patchset will be post later.


thanks,

Takashi