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

[Pierre-Louis Bossart]

On 03/28/2018 04:09 PM, Takashi Iwai wrote:
> On Wed, 28 Mar 2018 21:50:27 +0200,
> Pierre-Louis Bossart wrote:
>> 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 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>
>>>>>>>>>> Signed-off-by: Sriram Periyasamy <sriramx.periyasamy at 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.
> The decision should be done by user, of course.  But the question is
> how it's told to the driver.  And I hesitate to add no-rewind flag to
> the generic API just for an Intel device-specific quirk.
>
>> If we restrict the rewinds and then the application calls rewind, we
>> would have a run-time issue and stop playback/capture.
> Yes, the driver needs to know.  It's no doubt there.
>
>>> 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.
> The core already handles it by calling the ack callback.
> The driver can simply return an error from there for the unexpected /
> unwanted rewind calls.  It's only the matter how to tell to the
> driver.
>
>>> (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.
> ... if you patch all these.  That's not immediate.
It's a single flag to add to the hw_params. It'll take time to ripple to 
distros but it's not really complex.
> OTOH, if you do introduce a single switch, it's immediately
> effective, even with the old user-space without compilation.
Humm, now you lost me.
In your replies, you stated that the applications need to tell the 
driver - but disagreed that a hw_params flag was the right way. I am not 
religious here, as long as it remains simple enough we can look at other 
options.

I don't get however what 'single switch' are you referring to here?
In all cases, I don't see how we can enable this without rebuilding the 
apps.
Except for ChromeOS, I don't know how a distro would enable a switch 
that would impact all apps using the ALSA API.

>
> And, now back to the question: in which situation you'll have to mix
> both no-rewind and rewind operations on a single machine?  In theory,
> yes, we may keep rewind working, but from the practical POV, it is
> only PA who uses the rewind feature.  And, even if we disable rewind,
> PA will still work as is.  (I know it because I broke it previously :)
define 'work'. If PulseAudio cannot use the rewinds, then the system 
sounds will be heard with a delay and transitions between use cases will 
only happen when the queued audio is finished playing.

If an app wants to play a sound immediately due to user interaction 
requirements, rewinds are an attractive solution. I don't know how we 
could determine ahead of time what userspace will do, certainly the use 
of rewinds is on a per-card basis and likely a per-stream decision.

> So, what's wrong with introducing a global switch instead of changing
> the existing API and requiring the code change in each application and
> rebuild?
I totally agree that pretty much all apps don't do any rewinds, and 
rewinds on capture is quite useless. But I stuck to the 'we don't break 
userspace' mantra with the idea of an opt-in flag requiring an explicit 
code change to take effect. we will break userspace if we add a 
kernel-level switch, and the net result is that no one will ever use 
this option.