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@xxxxxxxxxxxxxxx> > >>>> > >>>> 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@xxxxxxxxxxxxxxx> > >>>> Signed-off-by: Ramesh Babu <ramesh.babu@xxxxxxxxx> > >>>> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@xxxxxxxxx> > >>>> Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@xxxxxxxxx> > >>> > >>> 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. thanks, Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
