[alsa-devel] [PATCH v2] ALSA: core: let low-level driver or userspace disable rewinds
Pierre-Louis Bossart
pierre-louis.bossart at linux.intel.com
Mon Jun 12 14:09:20 CEST 2017
On 6/12/17 1:35 AM, Takashi Sakamoto wrote:
> Hi,
>
> I have a question to your patch, in a point of the relationship between
> queued PCM frames and position of appl_ptr.
>
> On Jun 12 2017 14:27, Subhransu S. Prusty wrote:
>> From: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
>>
>> Add new hw_params flag to explicitly tell driver that rewinds will never
>> be used. This can be used by low-level driver to optimize DMA operations
>> and reduce power consumption. Use this flag only when data written in
>> ring buffer will never be invalidated, e.g. any update of appl_ptr is
>> final.
>>
>> Note that the update of appl_ptr include both a read/write data
>> operation as well as snd_pcm_forward() whose behavior is not modified.
>>
>> Signed-off-by: Pierre-Louis Bossart
>> <pierre-louis.bossart at linux.intel.com>
>> Signed-off-by: Ramesh Babu <ramesh.babu at intel.com>
>> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty at intel.com>
>> ---
>>
>> v1->v2: No change
>>
>> Since the ack callback change is now dropped and "ALSA: pcm:
>> conditionally
>> avoid mmap of control data" will be dropped with recent change "ALSA:
>> pcm:
>> Suppress status/control mmap when ack ops is present", I think this patch
>> can be merged independently.
>>
>> include/sound/pcm.h | 1 +
>> include/uapi/sound/asound.h | 1 +
>> sound/core/pcm_native.c | 8 ++++++++
>> 3 files changed, 10 insertions(+)
>>
>> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
>> index 79fedf517070..c1e2b87cd409 100644
>> --- a/include/sound/pcm.h
>> +++ b/include/sound/pcm.h
>> @@ -368,6 +368,7 @@ struct snd_pcm_runtime {
>> unsigned int rate_num;
>> unsigned int rate_den;
>> unsigned int no_period_wakeup: 1;
>> + unsigned int no_rewinds:1;
>> /* -- SW params -- */
>> int tstamp_mode; /* mmap timestamp is updated */
>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>> index fd41697cb4d3..c697ff90450d 100644
>> --- a/include/uapi/sound/asound.h
>> +++ b/include/uapi/sound/asound.h
>> @@ -365,6 +365,7 @@ struct snd_pcm_info {
>> #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 */
>> +#define SNDRV_PCM_HW_PARAMS_NO_REWINDS (1<<3) /*
>> disable rewinds */
>> struct snd_interval {
>> unsigned int min, max;
>> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
>> index bf5d0f2acfb9..0e8b7ea0d38d 100644
>> --- a/sound/core/pcm_native.c
>> +++ b/sound/core/pcm_native.c
>> @@ -554,6 +554,8 @@ static int snd_pcm_hw_params(struct
>> snd_pcm_substream *substream,
>> runtime->no_period_wakeup =
>> (params->info & SNDRV_PCM_INFO_NO_PERIOD_WAKEUP) &&
>> (params->flags & SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP);
>> + runtime->no_rewinds =
>> + (params->flags & SNDRV_PCM_HW_PARAMS_NO_REWINDS) ? 1 : 0;
>> bits = snd_pcm_format_physical_width(runtime->format);
>> runtime->sample_bits = bits;
>> @@ -2521,6 +2523,9 @@ static snd_pcm_sframes_t
>> snd_pcm_playback_rewind(struct snd_pcm_substream *subst
>> if (frames == 0)
>> return 0;
>> + if (runtime->no_rewinds)
>> + return 0;
>> +
>> snd_pcm_stream_lock_irq(substream);
>> ret = do_pcm_hwsync(substream);
>> if (!ret)
>> @@ -2539,6 +2544,9 @@ static snd_pcm_sframes_t
>> snd_pcm_capture_rewind(struct snd_pcm_substream *substr
>> if (frames == 0)
>> return 0;
>> + if (runtime->no_rewinds)
>> + return 0;
>> +
>> snd_pcm_stream_lock_irq(substream);
>> ret = do_pcm_hwsync(substream);
>> if (!ret)
>
> In my understanding, your intention for this patch is to prevent queue
> PCM frames in PCM buffer from being dropped. You have a plan to use
> 'struct snd_pcm_ops.ack' to notify the number of available PCM frames in
> the buffer to your hardware. I don't know exactly about design of your
> hardware because Intel developers never declare it.
>
> Well, For playback PCM substream, the patch satisfies your intension. On
> the other hand, for capture PCM substream, it doesn't.
>
> See below diagram. In this case, one PCM buffer consists of three
> period. The 'equal' sign represents queued PCM frames. You can see the
> relationship between hwptr and applptr is inverse for playback/capture
> substream.
>
> For playback:
> 0 1p 2p 3p
> |--======|========|==------|
> ^ ^
> hwptr applptr
>
> For capture:
> 0 1p 2p 3p
> |-----===|========|=====---|
> ^ ^
> applptr hwptr
>
> When operate rewinding, for playback substream, some queued PCM frames
> are dropped.
>
> For playback:
> 0 1p 2p 3p
> |--======|===_____|__------|
> ^ ^
> hwptr applptr<-
>
> The underscore sign represents the dropped PCM frames. But for capture
> substream, no queued PCM frames are dropped.
>
> For capture:
> 0 1p 2p 3p
> |#####===|========|=====---|
> ^ ^
> applptr<- hwptr
>
> The sharp sign represent PCM frames which are available again for the
> application.
>
> In your case, for capture substream, 'forward' operation should be
> avoided because it can drop queued PCM frames.
>
> For capture:
> 0 1p 2p 3p
> |-----___|__======|=====---|
> ^ ^
> ->applptr hwptr
>
> If data transmission in your hardware somehow depends on a callback of
> 'struct snd_pcm_ops.ack()'. I can imagine a unit of data transmission as
> an disadvantage of the forward operation.
>
> If describing the detail design of your hardware, you might get more helps.
>
> * I intentionally ignore discussion about burstness of data
> transmission and accuracy of hwptr.
You are probably reading too much into this.
The intent is to optimize DMA operations where the hardware now knows
both hw_ptr and appl_ptr. there is no use of the .ack() for copies into
intermediate buffers and the code for rewind on capture is largely for
consistency - we've never seen anyone using rewind on capture on
DSP-based platforms.
More information about the Alsa-devel
mailing list