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: Wed, 28 Mar 2018 16:51:46 -0500 Message-ID: <1714954c-0703-d397-ae08-24f3359f6af4@linux.intel.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by alsa0.perex.cz (Postfix) with ESMTP id AEE8E267063 for ; Wed, 28 Mar 2018 23:51:49 +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/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 >>>>>>>>>> >>>>>>>>>> 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 >>>>>>>>>> Signed-off-by: Ramesh Babu >>>>>>>>>> Signed-off-by: Subhransu S. Prusty >>>>>>>>>> Signed-off-by: Sriram Periyasamy >>>>>>>>> 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.