[alsa-devel] [RESEND][PATCH v4 0/3] Add SPIB Support for Intel Skylake platforms
Skylake audio controller supports SPIB (Software Position in buffer) capability, which can be used to inform position of application pointer to host DMA controller. When SPIB mode is enabled, driver could write the application pointer position in SPIB register. Host DMA will make sure it won't read/write beyond bytes specified in SPIB register.
SPIB mode will be useful in low power use cases, where DSP could pre-fetch large buffers to avoid frequent wakes caused due to interrupts.
To support SPIB in the driver, save the spib values in stream context which can be restored during resume from S3. Add new hw_params flag to explicitly tell driver that rewinds will never be used.
Please find the references for the previous discussions at [1][2]
[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-January/131329. html
[2] https://patchwork.kernel.org/patch/9795233/
and the patches of previous versions at [3][4][5]
[3] http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/121967.html
[4] http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/121683.html
[5] http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120676.html
Pierre-Louis Bossart (1): ALSA: core: let low-level driver or userspace disable rewinds
Ramesh Babu (2): ALSA: hda: ext: add spib to stream context ASoC: Intel: Skylake: Add support for spib mode
include/sound/hdaudio_ext.h | 1 + include/sound/pcm.h | 1 + include/uapi/sound/asound.h | 1 + sound/core/pcm_native.c | 8 ++++++++ sound/hda/ext/hdac_ext_stream.c | 2 ++ sound/soc/intel/skylake/skl-pcm.c | 43 ++++++++++++++++++++++++++++++++++++++- 6 files changed, 55 insertions(+), 1 deletion(-)
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 --- include/sound/pcm.h | 1 + include/uapi/sound/asound.h | 1 + sound/core/pcm_native.c | 8 ++++++++ 3 files changed, 10 insertions(+)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index e054c583d3b3..f60397eb4aab 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -379,6 +379,7 @@ struct snd_pcm_runtime { unsigned int rate_num; unsigned int rate_den; unsigned int no_period_wakeup: 1; + unsigned int no_rewinds:1;
/* -- SW params -- */ int tstamp_mode; /* mmap timestamp is updated */ diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index ed0a120d4f08..ff57e4c89de4 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -377,6 +377,7 @@ typedef int snd_pcm_hw_param_t; #define SNDRV_PCM_HW_PARAMS_NORESAMPLE (1<<0) /* avoid rate resampling */ #define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER (1<<1) /* export buffer */ #define SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP (1<<2) /* disable period wakeups */ +#define SNDRV_PCM_HW_PARAMS_NO_REWINDS (1<<3) /* disable rewinds */
struct snd_interval { unsigned int min, max; diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 77ba50ddcf9e..4616420cdbf7 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -692,6 +692,8 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, runtime->no_period_wakeup = (params->info & SNDRV_PCM_INFO_NO_PERIOD_WAKEUP) && (params->flags & SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP); + runtime->no_rewinds = + (params->flags & SNDRV_PCM_HW_PARAMS_NO_REWINDS) ? 1 : 0;
bits = snd_pcm_format_physical_width(runtime->format); runtime->sample_bits = bits; @@ -2614,6 +2616,9 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst if (frames == 0) return 0;
+ if (runtime->no_rewinds) + return -ENODEV; + snd_pcm_stream_lock_irq(substream); ret = do_pcm_hwsync(substream); if (!ret) @@ -2632,6 +2637,9 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr if (frames == 0) return 0;
+ if (runtime->no_rewinds) + return -ENODEV; + snd_pcm_stream_lock_irq(substream); ret = do_pcm_hwsync(substream); if (!ret)
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 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.
thanks,
Takashi
include/sound/pcm.h | 1 + include/uapi/sound/asound.h | 1 + sound/core/pcm_native.c | 8 ++++++++ 3 files changed, 10 insertions(+)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index e054c583d3b3..f60397eb4aab 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -379,6 +379,7 @@ struct snd_pcm_runtime { unsigned int rate_num; unsigned int rate_den; unsigned int no_period_wakeup: 1;
unsigned int no_rewinds:1;
/* -- SW params -- */ int tstamp_mode; /* mmap timestamp is updated */
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index ed0a120d4f08..ff57e4c89de4 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -377,6 +377,7 @@ typedef int snd_pcm_hw_param_t; #define SNDRV_PCM_HW_PARAMS_NORESAMPLE (1<<0) /* avoid rate resampling */ #define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER (1<<1) /* export buffer */ #define SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP (1<<2) /* disable period wakeups */ +#define SNDRV_PCM_HW_PARAMS_NO_REWINDS (1<<3) /* disable rewinds */
struct snd_interval { unsigned int min, max; diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 77ba50ddcf9e..4616420cdbf7 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -692,6 +692,8 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream, runtime->no_period_wakeup = (params->info & SNDRV_PCM_INFO_NO_PERIOD_WAKEUP) && (params->flags & SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP);
runtime->no_rewinds =
(params->flags & SNDRV_PCM_HW_PARAMS_NO_REWINDS) ? 1 : 0;bits = snd_pcm_format_physical_width(runtime->format); runtime->sample_bits = bits;
@@ -2614,6 +2616,9 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst if (frames == 0) return 0;
- if (runtime->no_rewinds)
return -ENODEV;- snd_pcm_stream_lock_irq(substream); ret = do_pcm_hwsync(substream); if (!ret)
@@ -2632,6 +2637,9 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr if (frames == 0) return 0;
- if (runtime->no_rewinds)
return -ENODEV;- snd_pcm_stream_lock_irq(substream); ret = do_pcm_hwsync(substream); if (!ret)
-- 2.7.4
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.
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."
Thanks, Sriram.
thanks,
Takashi
-- 2.7.4
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
--
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?
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.
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.
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.
thanks,
Takashi
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.
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.
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.
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.
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...
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.
thanks,
Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
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
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 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.
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 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
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 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.
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
- better DMA handling with opportunistic/bursty transfers
- 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?
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?
(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.)
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?
thanks,
Takashi
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 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.
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
- better DMA handling with opportunistic/bursty transfers
- 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. If we restrict the rewinds and then the application calls rewind, we would have a run-time issue and stop playback/capture.
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.
(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.
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 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.
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
- better DMA handling with opportunistic/bursty transfers
- 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. OTOH, if you do introduce a single switch, it's immediately effective, even with the old user-space without compilation.
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 :)
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?
thanks,
Takashi
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 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 questionis 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
- better DMA handling with opportunistic/bursty transfers
- 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.
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.)
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?
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.
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.)
thanks,
Takashi
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. 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.
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.
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.
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?
Thanks -Pierre
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?
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.
thanks,
Takashi
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...
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
From: Ramesh Babu ramesh.babu@intel.com
Platforms like skylake support SPIB (software position index in Buffer) capability, through which application pointer can be programmed in DMA. This helps DMA stop rendering stale data.
This patch saves spib values in stream context which can be restored during resume from S3.
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 --- include/sound/hdaudio_ext.h | 1 + sound/hda/ext/hdac_ext_stream.c | 2 ++ 2 files changed, 3 insertions(+)
diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h index 9c14e21dda85..34c41496fbc7 100644 --- a/include/sound/hdaudio_ext.h +++ b/include/sound/hdaudio_ext.h @@ -89,6 +89,7 @@ struct hdac_ext_stream {
u32 dpib; u32 lpib; + u32 spib; bool decoupled:1; bool link_locked:1; bool link_prepared; diff --git a/sound/hda/ext/hdac_ext_stream.c b/sound/hda/ext/hdac_ext_stream.c index c96d7a7a36af..c5212709bdb7 100644 --- a/sound/hda/ext/hdac_ext_stream.c +++ b/sound/hda/ext/hdac_ext_stream.c @@ -461,6 +461,8 @@ int snd_hdac_ext_stream_set_spib(struct hdac_ext_bus *ebus, }
writel(value, stream->spib_addr); + /* save the spib value in stream context */ + stream->spib = value;
return 0; }
From: Ramesh Babu ramesh.babu@intel.com
Skylake audio controller supports SPIB (Software Position in buffer) capability, which can be used to inform position of application pointer to host DMA controller. When SPIB mode is enabled, driver could write the application pointer position in SPIB register. Host DMA will make sure it won't read/write beyond bytes specified in SPIB register.
SPIB mode will be useful in low power use cases, where DSP could pre-fetch large buffers to avoid frequent wakes caused due to interrupts.
Skylake driver makes use of no_rewind flag and appl_ptr_update callback to enable and update SPIB register respectively.
Signed-off-by: Ramesh Babu ramesh.babu@intel.com Signed-off-by: Subhransu S. Prusty subhransu.s.prusty@intel.com Signed-off-by: Sanyog Kale sanyog.r.kale@intel.com Signed-off-by: Sriram Periyasamy sriramx.periyasamy@intel.com --- sound/soc/intel/skylake/skl-pcm.c | 43 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c index 15cb8ac3e374..01780a50a322 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -43,7 +43,8 @@ static const struct snd_pcm_hardware azx_pcm_hw = { SNDRV_PCM_INFO_SYNC_START | SNDRV_PCM_INFO_HAS_WALL_CLOCK | /* legacy */ SNDRV_PCM_INFO_HAS_LINK_ATIME | - SNDRV_PCM_INFO_NO_PERIOD_WAKEUP), + SNDRV_PCM_INFO_NO_PERIOD_WAKEUP | + SNDRV_PCM_INFO_SYNC_APPLPTR), .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S24_LE, @@ -145,6 +146,7 @@ int skl_pcm_host_dma_prepare(struct device *dev, struct skl_pipe_params *params) unsigned int format_val; struct hdac_stream *hstream; struct hdac_ext_stream *stream; + struct snd_pcm_runtime *runtime; int err;
hstream = snd_hdac_get_stream(bus, params->stream, @@ -170,6 +172,11 @@ int skl_pcm_host_dma_prepare(struct device *dev, struct skl_pipe_params *params) if (err < 0) return err;
+ /* Enable SPIB if no_rewinds flag is set */ + runtime = hdac_stream(stream)->substream->runtime; + if (runtime->no_rewinds) + snd_hdac_ext_stream_spbcap_enable(ebus, true, hstream->index); + hdac_stream(stream)->prepared = 1;
return 0; @@ -366,9 +373,14 @@ static int skl_pcm_hw_free(struct snd_pcm_substream *substream, { struct hdac_ext_bus *ebus = dev_get_drvdata(dai->dev); struct hdac_ext_stream *stream = get_hdac_ext_stream(substream); + struct snd_pcm_runtime *runtime = substream->runtime; + struct hdac_stream *hstream = hdac_stream(stream);
dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
+ if (runtime->no_rewinds) + snd_hdac_ext_stream_spbcap_enable(ebus, false, hstream->index); + snd_hdac_stream_cleanup(hdac_stream(stream)); hdac_stream(stream)->prepared = 0;
@@ -444,6 +456,7 @@ static int skl_pcm_trigger(struct snd_pcm_substream *substream, int cmd, struct skl_module_cfg *mconfig; struct hdac_ext_bus *ebus = get_bus_ctx(substream); struct hdac_ext_stream *stream = get_hdac_ext_stream(substream); + struct snd_pcm_runtime *runtime = substream->runtime; struct snd_soc_dapm_widget *w; int ret;
@@ -469,6 +482,10 @@ static int skl_pcm_trigger(struct snd_pcm_substream *substream, int cmd, snd_hdac_ext_stream_set_dpibr(ebus, stream, stream->lpib); snd_hdac_ext_stream_set_lpib(stream, stream->lpib); + + if (runtime->no_rewinds) + snd_hdac_ext_stream_set_spib(ebus, + stream, stream->spib); }
case SNDRV_PCM_TRIGGER_START: @@ -1095,6 +1112,29 @@ static int skl_platform_pcm_trigger(struct snd_pcm_substream *substream, return 0; }
+/* Update SPIB register with application position */ +static int skl_platform_ack(struct snd_pcm_substream *substream) +{ + struct hdac_ext_stream *estream = get_hdac_ext_stream(substream); + struct snd_pcm_runtime *runtime = substream->runtime; + struct hdac_ext_bus *ebus = get_bus_ctx(substream); + ssize_t appl_pos, buf_size; + u32 spib; + + /* Use spib mode only if no_rewind mode is set */ + if (!runtime->no_rewinds) + return 0; + + appl_pos = frames_to_bytes(runtime, runtime->control->appl_ptr); + buf_size = frames_to_bytes(runtime, runtime->buffer_size); + + spib = appl_pos % buf_size; + + /* Allowable value for SPIB is 1 byte to max buffer size */ + spib = (spib == 0) ? buf_size : spib; + return snd_hdac_ext_stream_set_spib(ebus, estream, spib); +} + static snd_pcm_uframes_t skl_platform_pcm_pointer (struct snd_pcm_substream *substream) { @@ -1202,6 +1242,7 @@ static const struct snd_pcm_ops skl_platform_ops = { .get_time_info = skl_get_time_info, .mmap = snd_pcm_lib_default_mmap, .page = snd_pcm_sgbuf_ops_page, + .ack = skl_platform_ack, };
static void skl_pcm_free(struct snd_pcm *pcm)
On Tue, Mar 20, 2018 at 09:31:05PM +0530, Sriram Periyasamy wrote:
Skylake audio controller supports SPIB (Software Position in buffer) capability, which can be used to inform position of application pointer
You probably want to cut down on the number of tags you're adding in your subject lines - what I see when I look at this mail in my inbox is:
-> 1061 T 03/20 Sriram Periyasa (1.7K) [alsa-devel][RESEND][PATCH v4 0/3] Add
so I can't see *anything* about what the patch is about! The [alsa-devel] bit is probably the main thing that can go, I know the mailing list software adds it which is unfortuate.
participants (4)
-
Mark Brown -
Pierre-Louis Bossart -
Sriram Periyasamy -
Takashi Iwai