Re: [PATCH v3] ALSA: core: let low-level driver or userspace disable rewinds

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 6/19/17 10:25 AM, Takashi Iwai wrote:
> On Mon, 19 Jun 2017 17:16:28 +0200,
> Pierre-Louis Bossart wrote:
>>
>> On 6/19/17 3:44 AM, Takashi Iwai wrote:
>>> On Mon, 19 Jun 2017 08:22:37 +0200,
>>> Subhransu S. Prusty wrote:
>>>>
>>>> From: Pierre-Louis Bossart <pierre-louis.bossart@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@linux.intel.com>
>>>> Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
>>>> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
>>>> ---
>>>>
>>>> v2 -> v3
>>>> 	return error for rewind operation if no_rewinds set.
>>>
>>> Now I'm thinking whether this change is really needed or in the right
>>> direction.  I suppose that the driver would change the behavior
>>> depending on the flag of this flag.  But then how can application know
>>> that it should not rewind on a certain device and set the flag?
>>
>> The application (which is in most cases an audio server) *knows* if it
>> requires rewinds or not. It's part of its design, with rewinds
>> typically disabled if period interrupts are required. It's been that
>> way for a number of years now. The use of rewinds is typically
>> associated with the combination of a large buffer and no interrupts
>> (having either of the two would not require rewinds).
>>
>> So the idea is that the application makes a statement that rewinds
>> will not be used, and the low-level driver makes use of the
>> information to enable whatever optimizations are available at the
>> hardware level.
>
> I could imagine that, but the usefulness of such a flag is shown only
> with the actual usage.  So I'd say, let's include this together with
> the actual user of this flag in the driver side.
>
> The only question at this point is whether this addition of the flag
> is appropriate.  Someone may think whether it's only about rewind?
> What about forward?  Is hw_params flags the best?

Rewinds is already a problematic feature, e.g. in PulseAudio we had to 
introduce a notion of 'guard band' to avoid rewinding too close to the 
actual hardware pointer and update parts of the ring buffer that have 
already been fetched by a DMA transaction.
Disabling rewinds brings two main benefits
- if you work with small buffers, the DMA/DSP has information on how 
much information it can fetch in each transaction. This was always a 
nightmare for DSP firmware.
- if you work with large buffers, the DMA/DSP can fetch data whenever 
the path to memory is enabled at the system level due to other non-audio 
transfers, thus reducing unnecessary partial wakes in an SoC.
HW flags were chosen mainly because you would not want to change this 
dynamically (which excludes sw_params) and it follows the same logic as 
the no-interrupt flag.


Forward is just fine, it moves the application pointer away from the 
hardware one. What to do with the data in the ring buffer is a different 
story though.


>
> I find it "OK, not very sexy but acceptable" for now.  But I'd like to
> hear from other opinions.
>
>
>> Exposing more information to userspace would quickly lead to a
>> confusing decision-making and would require more than just a flag.
>
> Yes, that's another side of the coin, too.
>
>
> thanks,
>
> Takashi
>
>>> We may, instead, let the driver behaving in the deep buffer mode as
>>> default (or turned on/off via the kcontrol).  Then you can simply
>>> return an error for the negative appl_ptr change in ack callback --
>>> which effectively disables the rewind.
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>>>
>>>>  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..6769f4751fa0 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 -ENODEV;
>>>> +
>>>>  	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 -ENODEV;
>>>> +
>>>>  	snd_pcm_stream_lock_irq(substream);
>>>>  	ret = do_pcm_hwsync(substream);
>>>>  	if (!ret)
>>>> --
>>>> 1.9.1
>>>>
>>