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

Mon Jun 19 14:26:32 CEST 2017

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 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.

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

Regards

Takashi Sakamoto