[alsa-devel] [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 at 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