From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pierre-Louis Bossart Subject: Re: [RESEND][PATCH v4 1/3] ALSA: core: let low-level driver or userspace disable rewinds Date: Thu, 29 Mar 2018 16:40:43 -0500 Message-ID: References: <1521561668-28613-1-git-send-email-sriramx.periyasamy@intel.com> <1521561668-28613-2-git-send-email-sriramx.periyasamy@intel.com> <20180325104643.GA13721@intel.com> <53f5df7e-0162-9a4a-ea2d-a8beb4c14b89@linux.intel.com> <1714954c-0703-d397-ae08-24f3359f6af4@linux.intel.com> <6d209396-bfad-b210-cb83-73b4be4aa360@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by alsa0.perex.cz (Postfix) with ESMTP id F0A69267140 for ; Thu, 29 Mar 2018 23:40:48 +0200 (CEST) In-Reply-To: Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Takashi Iwai Cc: ALSA ML , Vinod Koul , Sriram Periyasamy , Ramesh Babu , Takashi Sakamoto , Liam Girdwood , Patches Audio , Mark Brown , "Subhransu S . Prusty" List-Id: alsa-devel@alsa-project.org 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