Re: [RESEND][PATCH v4 1/3] ALSA: core: let low-level driver or userspace disable rewinds

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

On 3/28/18 1:35 PM, Takashi Iwai wrote:
> On Wed, 28 Mar 2018 19:58:54 +0200,
> Pierre-Louis Bossart wrote:
>>
>> On 3/28/18 10:20 AM, Takashi Iwai wrote:
>>> On Wed, 28 Mar 2018 16:30:09 +0200,
>>> Pierre-Louis Bossart wrote:
>>>>
>>>> On 3/25/18 9:58 AM, Takashi Iwai wrote:
>>>>> On Sun, 25 Mar 2018 12:46:43 +0200,
>>>>> Sriram Periyasamy wrote:
>>>>>>
>>>>>> On Tue, Mar 20, 2018 at 05:17:35PM +0100, Takashi Iwai wrote:
>>>>>>> On Tue, 20 Mar 2018 17:01:06 +0100,
>>>>>>> Sriram Periyasamy 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>
>>>>>>>> Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>
>>>>>>>
>>>>>>> Well, I'm still not convinced with this flag.
>>>>>>>
>>>>>>> First off, does it really need to be per PCM stream?  The introducing
>>>>>>
>>>>>> Flag per PCM stream helps where each stream in given system may have
>>>>>> different requirement such as low power or low latency based on the
>>>>>> use case. For example in case of low power stream, driver can perform
>>>>>> required optimizations at hardware level based on the no_rewind flag.
>>>>>
>>>>> Yes, but does it really need to be PCM stream, i.e. per application?
>>>>> Certainly the system will be using some sound backend (like PA).  In
>>>>> which scenario can such behavior change -- some application uses a
>>>>> different backend on a phone or a tablet?
>>>>
>>>> This is intended for the case where the system server exposes a
>>>> 'deep-buffer' PCM device for music playback in low-power mode and a
>>>> separate one for system sounds or anything that requires
>>>> interactivity.
>>>> The need for rewinding is really for the case where the interactive
>>>> system sounds are mixed with music, when you have separation between
>>>> types of sounds and hardware/firmware mixing then the rewinds are
>>>> unnecessary.
>>>
>>> Yes, but why application must tell no-rewind flag if it wants to
>>> save a bit of power?  IOW, how each application can know it needs to
>>> set no-rewind flag *for saving power*?
>>>
>>> Or, put in another way: you want to make all applications running in
>>> lower power generically.  What would you do?  Adding no-rewind flag to
>>> all calls?  It makes no sense if the application runs on non-Intel
>>> chips, so can't be hard-coded.
>>>
>>>> If there are multiple applications using different PCM devices each
>>>> (which is a bit hypothetical to me) there is no way to know ahead of
>>>> time when the modules are loaded if the application will perform
>>>> rewinds due to its interactive nature or will just stream without ever
>>>> invalidating the ring buffer. So yes it's per stream.
>>>
>>> Fair enough, per stream is a requirement.
>>>
>>> But still my argument below applies: what you really want to set
>>> is to make the stream low-power.  It's not about to make the stream
>>> non-rewindable.  And this makes me feel uneasy.
>>>
>>>
>>>>>>> something to hw_parms implies that it varies per application.  But I
>>>>>>> can't imagine that a system requires different behavior per stream
>>>>>>> regarding such a thing.
>>>>>>>
>>>>>>> Second, the driver can implement a check in PCM ack callback to
>>>>>>> prevent the rewind, too.  Then there is no need to touch the PCM
>>>>>>> core.
>>>>>>>
>>>>>>
>>>>>> As per the previous discussion at [1],
>>>>>>
>>>>>> [1]
>>>>>> https://patchwork.kernel.org/patch/9795233/
>>>>>>
>>>>>> from Pierre,
>>>>>>
>>>>>> "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.
>>>>>>
>>>>>> Exposing more information to userspace would quickly lead to a confusing
>>>>>> decision-making and would require more than just a flag."
>>>>>
>>>>> And, requiring *that* information is already confusing, IMO.
>>>>> Think from the application writer POV: what is the relation between
>>>>> the power-saving and no_rewind behavior in application level at all?
>>>>> If you have no idea about the hardware details, they are totally
>>>>> irrelevant.
>>>>
>>>> I feel like disabling IRQs or disabling rewinds is the same level of
>>>> information, you set the flags without necessary knowing all the power
>>>> savings down to the mW level. But it provides an opportunity to save
>>>> power with additional degrees of freedom for implementations.
>>>    Sure, I do understand this will bring the merit.  But the question
>>> is
>>> the API design.
>>>
>>>> An additional benefit of using the underlying SPIB register on Intel
>>>> hardware is that the DMA hardware will not wrap-around, which can lead
>>>> to better detection of real-time issues and a guarantee that stale
>>>> data will not be played.
>>>
>>> So, again, the purpose of no-rewind isn't the rewind thing itself.
>>> It's set for obtaining other benefits.
>>>
>>>>> Or, think like this way: imagine a hardware that requires a different
>>>>> constraint, e.g. the power-of-two buffer size, for power-efficient
>>>>> operation.  What would you do?  Adding a new power_of_two bit flag
>>>>> into hw_params?  Likely not.
>>>>
>>>> we've added the noIRQ mode in the past using flags, if now you are
>>>> saying that flags is a bad idea then fine, but let's be consistent...
>>>
>>> The no-IRQ is rather a more drastic behavior change.  The ALSA PCM
>>> mandated the period update per definition, and setting this flag
>>> really switches to a different mode, hence it deserves for an API
>>> extension.  And, the flag itself is self-explaining: the less IRQ is
>>> less power.  But no-rewind is...?
>>>
>>>>> In such a case, I would expect some operation mode switch
>>>>> (e.g. power-saving vs low latency or whatever) instead of a very
>>>>> specific hw_parmas flag.  It might be a module option, via ALSA
>>>>> control, or something else.  But it's clearer for which purpose it's
>>>>> present, at least, and it can be implemented well without changing the
>>>>> existing API.
>>>>
>>>> We have no way of predicting what the application will do so the
>>>> module option is not possible.
>>>>
>>>> Using an ALSA control is possible, but it's odd to me.
>>>>
>>>> I really don't see what's so problematic about adding flags. I uses an
>>>> existing capability of the API, it's consistent with the previous
>>>> usages. There is no change in behavior for existing apps, only newer
>>>> can benefit for better use of the hardware. There is no complicated
>>>> decision making, you set the flags if you don't use IRQ or rewinds.
>>>> And it's not like we will have new flags every week, we've been
>>>> talking about this SPIB capability since Skylake which is 3 years old
>>>> already.
>>>
>>> Again, my concern is that you swapped between the purpose and the
>>> method.  The no-irq isn't any purpose, per se.  It's just a
>>> requirement some hardware casually applies for power saving.
>>>
>>> The real need isn't about each detailed hardware-specific flag, but
>>> rather some API to give a hint for the preferred operation mode.
>>
>> let me try a different explanation (don't want to be pedantic but try
>> to explain the line of thought).
>>
>> There are two needs in terms of application/driver interaction.
>>
>> The first need is to let the application know about hardware
>> capabilities or restrictions. This is handled today with the .info
>> fields. We have quite a few of them that provide information to
>> userspace and let the application use the ALSA API in different ways
>> (e.g. BATCH, BLOCK_TRANSFER, PAUSE, SYNC_START, WALL_CLK). To take the
>> example above, if a specific hardware could handle optimizations for
>> powers of two, it could add a flag in the .info field and let the
>> application make that decision.
>>
>> The second need is to establish a contract between application and
>> driver, set in stone and non modifiable dynamically while the stream
>> is open. We are using hw_params for this, and the flags are one way to
>> extend the API to new capabilities. the no-irq flag is one example of
>> this contract which fundamentally changes the way the application is
>> written. It's not limited to power savings but can also be used to
>> reduce the latency as done by Android/AAudio, and it's not a 'casual'
>> way of doing things but a fundamental design decision.
>>
>> The proposal in this patchset is to restrict the use of rewinds which
>> can have two known benefits
>> 1. better DMA handling with opportunistic/bursty transfers
>> 2. no wrap-around and handling of stale data.
>> This capability may be used for low-power and low-latency, in a
>> similar way to the no-irq mode. Whether it makes sense for a system is
>> not the debate here, we want to leave the decision making to system
>> integrators. Since we use the hw_params flags in the past, we chose
>> the same solution. We also did not add any flag in the .info field
>> since the application doesn't really need to know what hardware will
>> do or not. If it doesn't use rewinds, it just states it and lets the
>> hardware enable new capabilities.
>>
>> I am really struggling to see how the proposal is viewed as different
>> from previous ones and where we confused 'purpose and method'. We used
>> the same hooks for the same purposes.
> 
> I'm fine to change something in the driver internal, and we've
> prepared it with the extension of ack callback, so far.  But, again,
> if it comes to the question about API, I have to be stubborn.
> 
> The no-IRQ mode was designed for the clear behavior change, thus it's
> mandatory to tie with the hw_params.  However, the no-rewind is
> different.  It's simple to restrict the rewind from the driver side as
> is without changing the current API.  So why it's in hw_params and why
> each application must set explicitly?

I don't see how the driver could make that decision on its own, sorry.
If we restrict the rewinds and then the application calls rewind, we 
would have a run-time issue and stop playback/capture.

> 
> Yes, it's opt-out.  But it means that it's useless unless application
> really knows the very h/w specific feature; i.e. it has to detect the
> running system, determine whether this flag is required or not, set it
> and stop using rewind.  Doing this for each application?
> 
> And, if you think "hey it's overreaction, it's nothing but an opt-out
> feature for only specific machines and specific applications", then
> why do we have to implement it in the generic hw_parmams API level,
> instead of some driver-specific thing like kcontrol or whatever?

It's not just a driver-specific thing, we have to deal with errors in 
the core if rewinds are called. So no matter what signalling mechanism 
is used by the application this capability has to be known by the core.

> 
> (Again, no-irq is a different situation; the no-IRQ is a special mode
>   for timer-based scheduling that makes application to skip the
>   mandatory period programming model.  But for no-rewind, there is
>   nothing required in the application side, since the rewind operation
>   is optional from the very beginning.)

but the driver doesn't know if rewinds will ever be used or not...

> 
> 
>> I also don't think that either the no-irq and no-rewinds can be
>> assigned to low-power or low-latency usages. I don't think we are in a
>> position to make that call and I don't think we want to add a
>> 'low-latency' or 'low-power' flag to the ALSA API - this would be
>> extremely confusing to applications.
>>
>> If your position is that we can no longer report or enable new
>> capabilities with the .info and hw_params fields, then what are the
>> options?
>> a) do nothing and leave ALSA handle hardware designed prior to 2013
>> b) define a new API and transition both the info and hw_params to
>> something else
> 
> Extending the hw_params flag itself is fine, but iff it really makes
> sense as a generic API usage.  I'm not fully convinced yet that
> no_rewind is mandatory, so far.  That's all.
> 
> 
> And, I'm really interested in how much impact we'd have if we disable
> the rewind completely for compensation of SPIB and other DSP stuff.
> AFAIK, it's only PA who really uses the rewind feature.  And, PA uses
> it for saving power (more exactly, for low-latency with power-saving
> operation using timer scheduling).  Now, practically seen, which mode
> should PA use?  Would disabling rewind cost too much even with other
> benefits?

The power savings implemented by PulseAudio with the timer-based 
scheduling are based on 10-year old designs where the core was the 
dominant issue.

Now the power savings have moved to a different level of optimization. 
With more buffering in the audio cluster, it's the path to memory and 
the opportunities to let the audio cluster work in a stand-alone matter 
than really save power. SPIB and friends really address how the memory 
is accessed and if transfers can be scheduled e.g. when the path to 
memory is already active due to some other event.


PulseAudio cannot benefit from proposal with the current implementation. 
It would only benefit when there are two streams, one for deep-buffer 
and one for system/low-latency streams, which is where the transfers can 
be optimized.

Chrome can be benefit immediately since rewinds are never used.
Jack can benefit immediately since rewinds are never used
Android HALs can benefit immediately since rewinds are never used.