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 03/29/2018 03:10 PM, Takashi Iwai wrote:
> On Thu, 29 Mar 2018 21:16:58 +0200,
> Pierre-Louis Bossart wrote:
>>
>>
>> On 03/29/2018 10:42 AM, Takashi Iwai wrote:
>>> On Wed, 28 Mar 2018 23:51:46 +0200,
>>> Pierre-Louis Bossart wrote:
>>>>>> 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.
>>> Yes, but you have to modify *each* of them.  That's flaky.
>>> Sure, it's safer for not enabling it as default, but is it your goal?
>>>
>>>>> 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?
>>> Just a simple module option or a kctl, for example.
>>> That is, if we can forget about the ability for adjustment per stream,
>>> the operation mode can be flipped by a switch on the fly.  This makes
>>> things a lot easier.
>>>
>>>> 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.
>>> So here came the question how it would impact on PA.
>>> If it doesn't work for PA, it means that essentially all traditional
>>> Linux distros will turn it off as default.  It's only for Android and
>>> Chrome OS, and they have own setup.  They can turn it on as default.
>>>
>>> If anyone wants to turn it on for JACK, they can do it.  But I don't
>>> think people would mix up JACK and PA on the same system at the very
>>> same time.  (One can hook up as a client, but then it doesn't matter
>>> about this hardware feature.)
>> I do recall Lennart implementing a mechanism to hand-over the ALSA
>> resources between PulseAudio and Jack so there is a precedent here.
>>>>> 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.
>>> I just tried music players with PA, and disabled the rewind forcibly
>>> (by putting return 0 in rewind_appl_ptr() in pcm_native.c).  I
>>> couldn't hear any audible difference in its response for changing the
>>> stream position, etc, interactively.  There was no audible latency
>>> increase by that change.
>>>
>>> It was the test weeks ago, so I refreshed testing now.  And the
>>> result is same, I saw no response difference.
>>> Any specific workload to make the clear difference in your mind?
>> It depends on how the app is written and what the configuration of the
>> server is.
> Isn't it about what PA does, not apps?
The PulseAudio server has parameters, but the app can ask for a 
different behavior depending on its needs by passing a pa_buffer_attr 
structure, with a -1 value meaning "highest possible latency". This is 
only possible btw with the regular API, apps using the simple API will 
not control this behavior and can't ask for a specific latency.

https://www.freedesktop.org/wiki/Software/PulseAudio/Documentation/Developer/Clients/LatencyControl/

>
>> Back in the Meego days the plan was to use very long
>> buffers and not rewinding did impact user experience. It's the same
>> issue on Windows and Android with deep buffers, source transitions,
>> volume changes can be painful when the committed samples cannot be
>> invalidated.
> Yes, but we're talking specifically about the Intel HD-audio, and it
> apparently works as is without rewind.  So an API change looks
> overcommitting (even though it's one bit flag addition).
>
> If the feature is really generic in wider hardware ranges, it's a
> convincing factor for changing the API.  Let's see.
>
>>>> 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.
>>> Sure, I agree with the golden "no regression" rule, too.  So I thought
>>> of some switch that is default off for the systems that may use
>>> rewind, while turning it on as default on the known systems like
>>> Android.  As PA is the sole user of rewind, here we won't see any
>>> regression, in theory.
>> The regression will depend on the depth of the buffer and whether apps
>> are ok to indicate the max latency when connecting with
>> PulseAudio. we've done crazy things with PulseAudio in the past and
>> things will go wrong.
> Yes, obviously we need testing.  But, in this case, the target
> hardware is *very* limited.  So covering this shouldn't be a big
> matter, I hope.  (And we'll keep a "big red button" to turn it off in
> emergency for some time.)
>
>
>>> And, such a system-level switch is preferable from the system
>>> management POV.  It's easier than a hard-coded API extension flag, and
>>> you can toggle it at any time for debugging if a problem happens.
>>>
>>> *IFF* the no-rewind feature is required by other multiple systems,
>>> adding some API would make sense.  But, currently, no-rewind is needed
>>> only for enabling some feature that is indirectly involved with the
>>> rewind on Intel chips.  To my eyes, it's too far away from the purpose
>>> of the PCM hw_params API.
>>>
>>> (Again, no-irq was in a different situation; it's a flag for a mode
>>>    that didn't exist (zero period) in the hw_params space.  But
>>>    no-rewind is irrelevant with the hw_params configuration itself.)
>> I hope we didn't give you the wrong impression that we are abusing the
>> API for nefarious or Intel-specific views.
>> In the initial patchset some 2+ years ago, there was this change that
>> disabled rewinds but also an additional functionality that let drivers
>> expose the granularity of the DMA bursts to userspace (I think
>> Jaroslav wanted it to be called in-flight-bytes), and possibly set
>> them depending on the application need. The latter part is of interest
>> to others, and I don't think removing the rewinds is useful only to
>> Intel, as soon as people use a second-level buffer rewinds are
>> problematic for everyone.
> I can think of some interface, too, but using hw_params flag is still
> questionable.  It's no worst choice, sure, but it doesn't look like
> the best fit, either, IMO -- especially because the rewind is no
> target of hw_params config space.
>
> We need to be extremely careful if it comes to API.  After all, API is
> the only fixed definition we can never ever change.  Once set, it must
> be rock-solid for the next decades...
>
>> So maybe a way to progress is to leave this flag set as a module
>> parameter as you suggested for now, but with the information known to
>> the core, which lets Intel enable functionality on Google OSes
>> (Android/Chrome), and in a second step (when we are done with SOF
>> upstream) suggest a more generic API enhancement related to DMA
>> handling which is not Intel specific.
>>
>> Would that be agreeable?
> Yes, that's a compromise.  I'd really love to have the support for
> this kind of feature, while I hesitate to add such a spontaneous bit
> flag in the global API level.  So let's begin with the driver-specific
> implementation, and extend to API level once when we see what are the
> real demands in wide range of hardware.

ok, makes sense.
Thanks for your feedback and enjoy your 4 day weekend.
I'll bug you next week with UCM stuff.
-Pierre