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

[Takashi Iwai <tiwai@suse.de>]

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