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

[Takashi Iwai <tiwai@suse.de>]

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@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@linux.intel.com>
> >>>> Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> >>>> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> >>>> Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@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.


thanks,

Takashi