[PATCH 0/8] ASoC: SOF: power optimizations for HDaudio platforms
This patchset provides two optimizations that result in significant power savings on Intel HDAudio platforms using SOF (Sound Open Firmware).
a) We previously prevented the Intel DSP from enabling the DMI_L1 capability to work-around issues with pause on capture streams. It turns out that this also prevented the platform from entering high C states in full-duplex usages such as videoconferencing - a rather basic use case since the start of the pandemic. The support for pause_push/release was already a bit controversial for Intel platforms, in theory platforms should only enable PAUSE if they can resume on the same sample, which is not the case on any Intel platform. Since we didn't want to disable a capability that could impact existing users, the suggestion is to optionally disable pause_push/release at build time or via a kernel parameter, in which case DMI_L1 is enabled. In practice very few applications make use of pause_push/release so there should be a limited impact when disabling this ALSA capability.
b) The use of the SPIB register also helps reduce power consumption, though to a smaller degree than DMI_L1. This hardware capability is however incompatible with userspace-initiated rewinds typically used by PulseAudio. In the past (2015..2017) Intel suggested an API extension to let applications disable rewinds. At the time the feedback was that such a capability was too Intel-specific and SPIB remained unused except for loading DSP code. We now see devices with smaller batteries being released, and it's time to revisit Linux support for SPIB to extend battery life. In this update the rewinds are disabled either at build-time or via a kernel parameter. As suggested by Takashi, a new SDNDRV_PCM_INFO flag is needed though to make sure the appl_ptr value is provided to the driver through the .ack callback. Distributions using PipeWire (Fedora34) and CRAS (ChromeOS/Chromium) can safely enable this option. Distributions using PulseAudio should probably avoid enabling it, although nothing is really fundamentally broken if they do. While in theory volume updates and mixing of notifications could be delayed, in practice distributions use small ring buffers that make such delays difficult to notice.
Again both of these updates are opt-in to avoid any impact on existing solutions or users: someone updating their kernel source but using 'make olddefconfig' will see the same results. Distributions that care neither about pause_push/release or rewinds should enable both options, in case of issues users will still be able to override these build-time choices with a simple module parameter.
Pierre-Louis Bossart (6): ASoC: SOF: Intel: Kconfig: clarify DMI L1 option description ASoC: SOF: Intel: simplify logic for DMI_L1 handling ASoC: SOF: pcm: add mechanisms to disable ALSA pause_push/release ASoC: SOF: Intel: add kernel parameter to set DMI L1 configuration ASoC: SOF: Intel: enable DMI L1 when pause is not supported ALSA: pcm: conditionally avoid mmap of control data
Ranjani Sridharan (2): ASOC: SOF: pcm: add .ack callback support ASoC: SOF: Intel: add .ack support for HDaudio platforms
include/sound/hdaudio_ext.h | 5 ++- include/uapi/sound/asound.h | 1 + sound/core/pcm_native.c | 17 ++++++++ sound/soc/sof/Kconfig | 9 ++++ sound/soc/sof/intel/Kconfig | 14 +++--- sound/soc/sof/intel/apl.c | 1 + sound/soc/sof/intel/cnl.c | 1 + sound/soc/sof/intel/hda-pcm.c | 74 ++++++++++++++++++++++++++++++-- sound/soc/sof/intel/hda-stream.c | 13 +++--- sound/soc/sof/intel/hda.h | 1 + sound/soc/sof/intel/tgl.c | 1 + sound/soc/sof/ops.h | 10 +++++ sound/soc/sof/pcm.c | 16 +++++++ sound/soc/sof/sof-priv.h | 3 ++ 14 files changed, 148 insertions(+), 18 deletions(-)
This option is only valid for HDaudio platforms. This was described in the help but not explicit in the option description.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/sof/intel/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig index 4bce89b5ea40..d9108b12740e 100644 --- a/sound/soc/sof/intel/Kconfig +++ b/sound/soc/sof/intel/Kconfig @@ -250,7 +250,7 @@ config SND_SOC_SOF_HDA_PROBES If unsure, select "N".
config SND_SOC_SOF_HDA_ALWAYS_ENABLE_DMI_L1 - bool "SOF enable DMI Link L1" + bool "SOF Intel-HDA enable DMI Link L1" help This option enables DMI L1 for both playback and capture and disables known workarounds for specific HDAudio platforms.
We don't need to test in multiple places if the kconfig SND_SOC_SOF_HDA_ALWAYS_ENABLE_DMI_L1 is enabled or not, we might as well set the existing DMI_L1_COMPATIBLE flag.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/sof/intel/hda-pcm.c | 3 ++- sound/soc/sof/intel/hda-stream.c | 11 +++++------ 2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/sound/soc/sof/intel/hda-pcm.c b/sound/soc/sof/intel/hda-pcm.c index df00db8369c7..59220fa1def1 100644 --- a/sound/soc/sof/intel/hda-pcm.c +++ b/sound/soc/sof/intel/hda-pcm.c @@ -229,7 +229,8 @@ int hda_dsp_pcm_open(struct snd_sof_dev *sdev, }
/* All playback and D0i3 compatible streams are DMI L1 capable */ - if (direction == SNDRV_PCM_STREAM_PLAYBACK || + if (IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_ALWAYS_ENABLE_DMI_L1) || + direction == SNDRV_PCM_STREAM_PLAYBACK || spcm->stream[substream->stream].d0i3_compatible) flags |= SOF_HDA_STREAM_DMI_L1_COMPATIBLE;
diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c index 40a3993ae2cb..4e49b7b16b4c 100644 --- a/sound/soc/sof/intel/hda-stream.c +++ b/sound/soc/sof/intel/hda-stream.c @@ -197,11 +197,10 @@ hda_dsp_stream_get(struct snd_sof_dev *sdev, int direction, u32 flags) * Workaround to address a known issue with host DMA that results * in xruns during pause/release in capture scenarios. */ - if (!IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_ALWAYS_ENABLE_DMI_L1)) - if (stream && !(flags & SOF_HDA_STREAM_DMI_L1_COMPATIBLE)) - snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, - HDA_VS_INTEL_EM2, - HDA_VS_INTEL_EM2_L1SEN, 0); + if (stream && !(flags & SOF_HDA_STREAM_DMI_L1_COMPATIBLE)) + snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, + HDA_VS_INTEL_EM2, + HDA_VS_INTEL_EM2_L1SEN, 0);
return stream; } @@ -240,7 +239,7 @@ int hda_dsp_stream_put(struct snd_sof_dev *sdev, int direction, int stream_tag) spin_unlock_irq(&bus->reg_lock);
/* Enable DMI L1 if permitted */ - if (!IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_ALWAYS_ENABLE_DMI_L1) && dmi_l1_enable) + if (dmi_l1_enable) snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, HDA_VS_INTEL_EM2, HDA_VS_INTEL_EM2_L1SEN, HDA_VS_INTEL_EM2_L1SEN);
PulseAudio, PipeWire and CRAS do not use pause push/release, which means that we can optionally disable this capability without any impact on most users.
In addition, on some platforms, e.g. based on HDaudio DMAs, support for pause_push/release prevents the system from entering low-power states.
This patch suggests an opt-in selection via kconfig or kernel parameter to disable pause.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/sof/Kconfig | 9 +++++++++ sound/soc/sof/pcm.c | 7 +++++++ 2 files changed, 16 insertions(+)
diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig index cd659493b5df..81b834558a2d 100644 --- a/sound/soc/sof/Kconfig +++ b/sound/soc/sof/Kconfig @@ -55,6 +55,15 @@ config SND_SOC_SOF_DEBUG_PROBES Say Y if you want to enable probes. If unsure, select "N".
+config SND_SOC_SOF_PCM_DISABLE_PAUSE + bool "SOF disable pause push/release" + help + This option disables ALSA pause push/release capabilities for + SOF drivers. These capabilities are not used by typical + sound servers such as PulseAudio, PipeWire and CRAS. + Say Y if you want to disable pause push/release + If unsure, select "N". + config SND_SOC_SOF_DEVELOPER_SUPPORT bool "SOF developer options support" depends on EXPERT diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c index 9893b182da43..bab837ed8c7f 100644 --- a/sound/soc/sof/pcm.c +++ b/sound/soc/sof/pcm.c @@ -10,6 +10,7 @@ // PCM Layer, interface between ALSA and IPC. //
+#include <linux/moduleparam.h> #include <linux/pm_runtime.h> #include <sound/pcm_params.h> #include <sound/sof.h> @@ -20,6 +21,10 @@ #include "compress.h" #endif
+static bool pcm_disable_pause = IS_ENABLED(CONFIG_SND_SOC_SOF_PCM_DISABLE_PAUSE); +module_param_named(disable_pause, pcm_disable_pause, bool, 0444); +MODULE_PARM_DESC(disable_pause, "SOF HDA disable pause"); + /* Create DMA buffer page table for DSP */ static int create_page_table(struct snd_soc_component *component, struct snd_pcm_substream *substream, @@ -480,6 +485,8 @@ static int sof_pcm_open(struct snd_soc_component *component,
/* set runtime config */ runtime->hw.info = ops->hw_info; /* platform-specific */ + if (pcm_disable_pause) + runtime->hw.info &= ~SNDRV_PCM_INFO_PAUSE;
/* set any runtime constraints based on topology */ runtime->hw.formats = le64_to_cpu(caps->formats);
Exposing the DMI L1 configuration to users was in hindsight a bad idea. It led to several errors reported by distributions which selected it by mistake.
The Kconfig is now replaced with a kernel parameter that should only be used by expert users for tests. In a follow-up patch, the DMI L1 configuration is enabled automatically to conform to hardware programming sequences.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/sof/intel/Kconfig | 10 ---------- sound/soc/sof/intel/hda-pcm.c | 7 ++++++- 2 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig index d9108b12740e..219cf0bf9633 100644 --- a/sound/soc/sof/intel/Kconfig +++ b/sound/soc/sof/intel/Kconfig @@ -249,16 +249,6 @@ config SND_SOC_SOF_HDA_PROBES Say Y if you want to enable probes. If unsure, select "N".
-config SND_SOC_SOF_HDA_ALWAYS_ENABLE_DMI_L1 - bool "SOF Intel-HDA enable DMI Link L1" - help - This option enables DMI L1 for both playback and capture - and disables known workarounds for specific HDAudio platforms. - Only use to look into power optimizations on platforms not - affected by DMI L1 issues. This option is not recommended. - Say Y if you want to enable DMI Link L1. - If unsure, select "N". - endif ## SND_SOC_SOF_HDA_COMMON
config SND_SOC_SOF_HDA_LINK_BASELINE diff --git a/sound/soc/sof/intel/hda-pcm.c b/sound/soc/sof/intel/hda-pcm.c index 59220fa1def1..47ff2c757d0a 100644 --- a/sound/soc/sof/intel/hda-pcm.c +++ b/sound/soc/sof/intel/hda-pcm.c @@ -15,6 +15,7 @@ * Hardware interface for generic Intel audio DSP HDA IP */
+#include <linux/moduleparam.h> #include <sound/hda_register.h> #include <sound/pcm_params.h> #include "../sof-audio.h" @@ -27,6 +28,10 @@ #define SDnFMT_BITS(x) ((x) << 4) #define SDnFMT_CHAN(x) ((x) << 0)
+static bool hda_always_enable_dmi_l1; +module_param_named(always_enable_dmi_l1, hda_always_enable_dmi_l1, bool, 0444); +MODULE_PARM_DESC(always_enable_dmi_l1, "SOF HDA always enable DMI l1"); + u32 hda_dsp_get_mult_div(struct snd_sof_dev *sdev, int rate) { switch (rate) { @@ -229,7 +234,7 @@ int hda_dsp_pcm_open(struct snd_sof_dev *sdev, }
/* All playback and D0i3 compatible streams are DMI L1 capable */ - if (IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_ALWAYS_ENABLE_DMI_L1) || + if (hda_always_enable_dmi_l1 || direction == SNDRV_PCM_STREAM_PLAYBACK || spcm->stream[substream->stream].d0i3_compatible) flags |= SOF_HDA_STREAM_DMI_L1_COMPATIBLE;
DMI L1 entry is incompatible with pause on a capture stream, so if pause is not supported we can enable DMI L1 unconditionally.
Experimental results show an increased residency in higher C states and a significant decrease of system power consumption for "work from home" usages such as VoIP calls.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- sound/soc/sof/intel/hda-pcm.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/sound/soc/sof/intel/hda-pcm.c b/sound/soc/sof/intel/hda-pcm.c index 47ff2c757d0a..aaa7686c00ee 100644 --- a/sound/soc/sof/intel/hda-pcm.c +++ b/sound/soc/sof/intel/hda-pcm.c @@ -221,6 +221,7 @@ int hda_dsp_pcm_open(struct snd_sof_dev *sdev, struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + struct snd_pcm_runtime *runtime = substream->runtime; struct snd_soc_component *scomp = sdev->component; struct hdac_ext_stream *dsp_stream; struct snd_sof_pcm *spcm; @@ -233,7 +234,13 @@ int hda_dsp_pcm_open(struct snd_sof_dev *sdev, return -EINVAL; }
- /* All playback and D0i3 compatible streams are DMI L1 capable */ + /* + * All playback and D0i3 compatible streams are DMI L1 capable, others need + * pause push/release to be disabled + */ + if (!(runtime->hw.info & SNDRV_PCM_INFO_PAUSE)) + hda_always_enable_dmi_l1 = true; + if (hda_always_enable_dmi_l1 || direction == SNDRV_PCM_STREAM_PLAYBACK || spcm->stream[substream->stream].d0i3_compatible)
In case of mmap, by default alsa-lib mmaps both control and status data.
If driver subscribes for application pointer update, driver needs to get notification whenever appl ptr changes. With the above case driver won't get appl ptr notifications.
This patch check on a hw info flag and returns error when user land asks for mmaping control & status data, thus forcing user to issue IOCTL_SYNC_PTR.
This patch was originally submitted in 2017, c.f. https://lore.kernel.org/alsa-devel/1494896518-23399-4-git-send-email-subhran...
Suggested-by: Takashi Iwai tiwai@suse.de Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- include/uapi/sound/asound.h | 1 + sound/core/pcm_native.c | 17 +++++++++++++++++ 2 files changed, 18 insertions(+)
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 535a7229e1d9..e7566bfc106c 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -299,6 +299,7 @@ typedef int __bitwise snd_pcm_subformat_t; #define SNDRV_PCM_INFO_HAS_LINK_ABSOLUTE_ATIME 0x02000000 /* report absolute hardware link audio time, not reset on startup */ #define SNDRV_PCM_INFO_HAS_LINK_ESTIMATED_ATIME 0x04000000 /* report estimated link audio time */ #define SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME 0x08000000 /* report synchronized audio/system time */ +#define SNDRV_PCM_INFO_NO_STATUS_MMAP 0x10000000 /* status and control mmap not supported */
#define SNDRV_PCM_INFO_DRAIN_TRIGGER 0x40000000 /* internal kernel flag - trigger in drain */ #define SNDRV_PCM_INFO_FIFO_IN_FRAMES 0x80000000 /* internal kernel flag - FIFO size is in frames */ diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 8dbe86cf2e4f..01f755ce54a8 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -3810,11 +3810,13 @@ static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area) struct snd_pcm_file * pcm_file; struct snd_pcm_substream *substream; unsigned long offset; + unsigned int info; pcm_file = file->private_data; substream = pcm_file->substream; if (PCM_RUNTIME_CHECK(substream)) return -ENXIO; + info = substream->runtime->hw.info;
offset = area->vm_pgoff << PAGE_SHIFT; switch (offset) { @@ -3825,6 +3827,13 @@ static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area) case SNDRV_PCM_MMAP_OFFSET_STATUS_NEW: if (!pcm_status_mmap_allowed(pcm_file)) return -ENXIO; + /* + * force fallback to ioctl if driver doesn't support status + * and control mmap. + */ + if (info & SNDRV_PCM_INFO_NO_STATUS_MMAP) + return -ENXIO; + return snd_pcm_mmap_status(substream, file, area); case SNDRV_PCM_MMAP_OFFSET_CONTROL_OLD: if (pcm_file->no_compat_mmap || !IS_ENABLED(CONFIG_64BIT)) @@ -3833,6 +3842,14 @@ static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area) case SNDRV_PCM_MMAP_OFFSET_CONTROL_NEW: if (!pcm_control_mmap_allowed(pcm_file)) return -ENXIO; + + /* + * force fallback to ioctl if driver doesn't support status + * and control mmap. + */ + if (info & SNDRV_PCM_INFO_NO_STATUS_MMAP) + return -ENXIO; + return snd_pcm_mmap_control(substream, file, area); default: return snd_pcm_mmap_data(substream, file, area);
On Thu, 10 Jun 2021 22:53:24 +0200, Pierre-Louis Bossart wrote:
In case of mmap, by default alsa-lib mmaps both control and status data.
If driver subscribes for application pointer update, driver needs to get notification whenever appl ptr changes. With the above case driver won't get appl ptr notifications.
This patch check on a hw info flag and returns error when user land asks for mmaping control & status data, thus forcing user to issue IOCTL_SYNC_PTR.
This patch was originally submitted in 2017, c.f. https://lore.kernel.org/alsa-devel/1494896518-23399-4-git-send-email-subhran...
Suggested-by: Takashi Iwai tiwai@suse.de Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com
This kind of flag itself was what I also introduced for another purpose, too. There is a WIP patch that allows the use of non-coherent non-contiguous buffer pages, and this flag would fit for that. FWIW, the patch is found at https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/log/?h=topic...
Takashi
On 6/13/21 2:28 AM, Takashi Iwai wrote:
On Thu, 10 Jun 2021 22:53:24 +0200, Pierre-Louis Bossart wrote:
In case of mmap, by default alsa-lib mmaps both control and status data.
If driver subscribes for application pointer update, driver needs to get notification whenever appl ptr changes. With the above case driver won't get appl ptr notifications.
This patch check on a hw info flag and returns error when user land asks for mmaping control & status data, thus forcing user to issue IOCTL_SYNC_PTR.
This patch was originally submitted in 2017, c.f. https://lore.kernel.org/alsa-devel/1494896518-23399-4-git-send-email-subhran...
Suggested-by: Takashi Iwai tiwai@suse.de Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com
This kind of flag itself was what I also introduced for another purpose, too. There is a WIP patch that allows the use of non-coherent non-contiguous buffer pages, and this flag would fit for that. FWIW, the patch is found at https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/log/?h=topic...
Sorry Takashi, I missed your feedback on this patch.
Are you saying I should use the definition in that patch?
+#define SNDRV_DMA_TYPE_NONCONTIG 8 /* non-coherent SG buffer */
I am not quite sure if how this is related to the application using mmap or not?
On Mon, 12 Jul 2021 22:56:07 +0200, Pierre-Louis Bossart wrote:
On 6/13/21 2:28 AM, Takashi Iwai wrote:
On Thu, 10 Jun 2021 22:53:24 +0200, Pierre-Louis Bossart wrote:
In case of mmap, by default alsa-lib mmaps both control and status data.
If driver subscribes for application pointer update, driver needs to get notification whenever appl ptr changes. With the above case driver won't get appl ptr notifications.
This patch check on a hw info flag and returns error when user land asks for mmaping control & status data, thus forcing user to issue IOCTL_SYNC_PTR.
This patch was originally submitted in 2017, c.f. https://lore.kernel.org/alsa-devel/1494896518-23399-4-git-send-email-subhran...
Suggested-by: Takashi Iwai tiwai@suse.de Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com
This kind of flag itself was what I also introduced for another purpose, too. There is a WIP patch that allows the use of non-coherent non-contiguous buffer pages, and this flag would fit for that. FWIW, the patch is found at https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/log/?h=topic...
Sorry Takashi, I missed your feedback on this patch.
Are you saying I should use the definition in that patch?
+#define SNDRV_DMA_TYPE_NONCONTIG 8 /* non-coherent SG buffer */
I am not quite sure if how this is related to the application using mmap or not?
Not about that, but rather meant that some flag for disabling the mmap of PCM control record would be needed for other purposes like the above, too. That is, this patch could be out of series and applied beforehand in my side.
thanks,
Takashi
On 7/13/21 1:17 AM, Takashi Iwai wrote:
On Mon, 12 Jul 2021 22:56:07 +0200, Pierre-Louis Bossart wrote:
On 6/13/21 2:28 AM, Takashi Iwai wrote:
On Thu, 10 Jun 2021 22:53:24 +0200, Pierre-Louis Bossart wrote:
In case of mmap, by default alsa-lib mmaps both control and status data.
If driver subscribes for application pointer update, driver needs to get notification whenever appl ptr changes. With the above case driver won't get appl ptr notifications.
This patch check on a hw info flag and returns error when user land asks for mmaping control & status data, thus forcing user to issue IOCTL_SYNC_PTR.
This patch was originally submitted in 2017, c.f. https://lore.kernel.org/alsa-devel/1494896518-23399-4-git-send-email-subhran...
Suggested-by: Takashi Iwai tiwai@suse.de Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com
This kind of flag itself was what I also introduced for another purpose, too. There is a WIP patch that allows the use of non-coherent non-contiguous buffer pages, and this flag would fit for that. FWIW, the patch is found at https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/log/?h=topic...
Sorry Takashi, I missed your feedback on this patch.
Are you saying I should use the definition in that patch?
+#define SNDRV_DMA_TYPE_NONCONTIG 8 /* non-coherent SG buffer */
I am not quite sure if how this is related to the application using mmap or not?
Not about that, but rather meant that some flag for disabling the mmap of PCM control record would be needed for other purposes like the above, too. That is, this patch could be out of series and applied beforehand in my side.
Thanks Takashi, I will resubmit this separately. I may also break the series in two, the two parts (pause/L1EN support and rewinds) are not really connected, they help reduce power but at different levels.
From: Ranjani Sridharan ranjani.sridharan@linux.intel.com
Add the indirections required at the core level for platform-specific operations on ack.
Note that on errors in the .ack the ALSA core will restore the previous appl_ptr.
Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/sof/ops.h | 10 ++++++++++ sound/soc/sof/pcm.c | 9 +++++++++ sound/soc/sof/sof-priv.h | 3 +++ 3 files changed, 22 insertions(+)
diff --git a/sound/soc/sof/ops.h b/sound/soc/sof/ops.h index 4a5d6e497f05..fc9142fe3421 100644 --- a/sound/soc/sof/ops.h +++ b/sound/soc/sof/ops.h @@ -428,6 +428,16 @@ snd_sof_pcm_platform_pointer(struct snd_sof_dev *sdev, return 0; }
+/* pcm ack */ +static inline int snd_sof_pcm_platform_ack(struct snd_sof_dev *sdev, + struct snd_pcm_substream *substream) +{ + if (sof_ops(sdev) && sof_ops(sdev)->pcm_ack) + return sof_ops(sdev)->pcm_ack(sdev, substream); + + return 0; +} + #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES) static inline int snd_sof_probe_compr_assign(struct snd_sof_dev *sdev, diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c index bab837ed8c7f..8c47687d0d3a 100644 --- a/sound/soc/sof/pcm.c +++ b/sound/soc/sof/pcm.c @@ -817,6 +817,14 @@ static void sof_pcm_remove(struct snd_soc_component *component) snd_soc_tplg_component_remove(component); }
+static int sof_pcm_ack(struct snd_soc_component *component, + struct snd_pcm_substream *substream) +{ + struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component); + + return snd_sof_pcm_platform_ack(sdev, substream); +} + void snd_sof_new_platform_drv(struct snd_sof_dev *sdev) { struct snd_soc_component_driver *pd = &sdev->plat_drv; @@ -835,6 +843,7 @@ void snd_sof_new_platform_drv(struct snd_sof_dev *sdev) pd->hw_free = sof_pcm_hw_free; pd->trigger = sof_pcm_trigger; pd->pointer = sof_pcm_pointer; + pd->ack = sof_pcm_ack;
#if IS_ENABLED(CONFIG_SND_SOC_SOF_COMPRESS) pd->compress_ops = &sof_compressed_ops; diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h index fd8423172d8f..8640ffed6cb5 100644 --- a/sound/soc/sof/sof-priv.h +++ b/sound/soc/sof/sof-priv.h @@ -178,6 +178,9 @@ struct snd_sof_dsp_ops { snd_pcm_uframes_t (*pcm_pointer)(struct snd_sof_dev *sdev, struct snd_pcm_substream *substream); /* optional */
+ /* pcm ack */ + int (*pcm_ack)(struct snd_sof_dev *sdev, struct snd_pcm_substream *substream); /* optional */ + #if IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_PROBES) /* Except for probe_pointer, all probe ops are mandatory */ int (*probe_assign)(struct snd_sof_dev *sdev,
From: Ranjani Sridharan ranjani.sridharan@linux.intel.com
When we disable rewinds, then the .ack can be used to program SPIB with the application pointer, which allows the HDaudio DMA to save power by opportunistically bursting data transfers when the path to memory is enabled (and conversely to shut it down when there are no transfer requests).
The SPIB register can only be programmed with incremental values with wrap-around after the DMA RUN bits are set. The hdac stream structure is extended to keep track of the previous appl_ptr, and compared with the suggested value. When a rewind is detected, a negative error code is returned and the ALSA core will restore the old value in pcm_lib_apply_appl_ptr().
Rewinds are only used by PulseAudio. If rewinds are disabled by mistake in a distribution where PulseAudio is used, the user may hear volume changes and notification with a delay, depending on the size of the ring buffer and latency constraints. Practical experiments with Ubuntu default settings did not show any audible issues.
Co-developed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- include/sound/hdaudio_ext.h | 5 ++- sound/soc/sof/intel/Kconfig | 10 ++++++ sound/soc/sof/intel/apl.c | 1 + sound/soc/sof/intel/cnl.c | 1 + sound/soc/sof/intel/hda-pcm.c | 57 ++++++++++++++++++++++++++++++-- sound/soc/sof/intel/hda-stream.c | 2 ++ sound/soc/sof/intel/hda.h | 1 + sound/soc/sof/intel/tgl.c | 1 + 8 files changed, 75 insertions(+), 3 deletions(-)
diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h index a125e3814b58..33e1aa61d088 100644 --- a/include/sound/hdaudio_ext.h +++ b/include/sound/hdaudio_ext.h @@ -51,7 +51,8 @@ enum hdac_ext_stream_type { * @decoupled: stream host and link is decoupled * @link_locked: link is locked * @link_prepared: link is prepared - * link_substream: link substream + * @link_substream: link substream + * @old_appl_ptr: last appl_ptr to double-check rewinds when SPIB is used. */ struct hdac_ext_stream { struct hdac_stream hstream; @@ -71,6 +72,8 @@ struct hdac_ext_stream { bool link_prepared;
struct snd_pcm_substream *link_substream; + + snd_pcm_uframes_t old_appl_ptr; };
#define hdac_stream(s) (&(s)->hstream) diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig index 219cf0bf9633..904eabd958d9 100644 --- a/sound/soc/sof/intel/Kconfig +++ b/sound/soc/sof/intel/Kconfig @@ -249,6 +249,16 @@ config SND_SOC_SOF_HDA_PROBES Say Y if you want to enable probes. If unsure, select "N".
+config SND_SOC_SOF_HDA_DISABLE_REWINDS + bool "SOF Intel-HDA disable rewinds" + help + This option disables ALSA rewinds for HDaudio platforms, which + will help enable power savings capabilities. + ALSA rewinds are only used by PulseAudio, so can be disabled + for all distributions relying on PipeWire, JACK or CRAS. + Say Y if you want to disable rewinds. + If unsure, select "N". + endif ## SND_SOC_SOF_HDA_COMMON
config SND_SOC_SOF_HDA_LINK_BASELINE diff --git a/sound/soc/sof/intel/apl.c b/sound/soc/sof/intel/apl.c index c7ed2b3d6abc..854b90a9a511 100644 --- a/sound/soc/sof/intel/apl.c +++ b/sound/soc/sof/intel/apl.c @@ -73,6 +73,7 @@ const struct snd_sof_dsp_ops sof_apl_ops = { .pcm_hw_free = hda_dsp_stream_hw_free, .pcm_trigger = hda_dsp_pcm_trigger, .pcm_pointer = hda_dsp_pcm_pointer, + .pcm_ack = hda_dsp_pcm_ack,
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_PROBES) /* probe callbacks */ diff --git a/sound/soc/sof/intel/cnl.c b/sound/soc/sof/intel/cnl.c index 821f25fbcf08..93e075ccb8b8 100644 --- a/sound/soc/sof/intel/cnl.c +++ b/sound/soc/sof/intel/cnl.c @@ -278,6 +278,7 @@ const struct snd_sof_dsp_ops sof_cnl_ops = { .pcm_hw_free = hda_dsp_stream_hw_free, .pcm_trigger = hda_dsp_pcm_trigger, .pcm_pointer = hda_dsp_pcm_pointer, + .pcm_ack = hda_dsp_pcm_ack,
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_PROBES) /* probe callbacks */ diff --git a/sound/soc/sof/intel/hda-pcm.c b/sound/soc/sof/intel/hda-pcm.c index aaa7686c00ee..b53194c63057 100644 --- a/sound/soc/sof/intel/hda-pcm.c +++ b/sound/soc/sof/intel/hda-pcm.c @@ -32,6 +32,10 @@ static bool hda_always_enable_dmi_l1; module_param_named(always_enable_dmi_l1, hda_always_enable_dmi_l1, bool, 0444); MODULE_PARM_DESC(always_enable_dmi_l1, "SOF HDA always enable DMI l1");
+static bool hda_disable_rewinds = IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_DISABLE_REWINDS); +module_param_named(disable_rewinds, hda_disable_rewinds, bool, 0444); +MODULE_PARM_DESC(disable_rewinds, "SOF HDA disable rewinds"); + u32 hda_dsp_get_mult_div(struct snd_sof_dev *sdev, int rate) { switch (rate) { @@ -120,8 +124,11 @@ int hda_dsp_pcm_hw_params(struct snd_sof_dev *sdev, return ret; }
- /* disable SPIB, to enable buffer wrap for stream */ - hda_dsp_stream_spib_config(sdev, stream, HDA_DSP_SPIB_DISABLE, 0); + /* enable SPIB when rewinds are disabled */ + if (hda_disable_rewinds) + hda_dsp_stream_spib_config(sdev, stream, HDA_DSP_SPIB_ENABLE, 0); + else + hda_dsp_stream_spib_config(sdev, stream, HDA_DSP_SPIB_DISABLE, 0);
/* update no_stream_position flag for ipc params */ if (hda && hda->no_ipc_position) { @@ -140,12 +147,51 @@ int hda_dsp_pcm_hw_params(struct snd_sof_dev *sdev, return 0; }
+/* update SPIB register with appl position */ +int hda_dsp_pcm_ack(struct snd_sof_dev *sdev, struct snd_pcm_substream *substream) +{ + struct hdac_stream *hstream = substream->runtime->private_data; + struct hdac_ext_stream *stream = stream_to_hdac_ext_stream(hstream); + struct snd_pcm_runtime *runtime = substream->runtime; + ssize_t appl_pos, buf_size; + u32 spib; + + if (!hda_disable_rewinds) + return 0; + + /* + * paranoia check: if a rewind request took place after the RUN bits were programmed, + * deny it since hardware only supports monotonic (modulo) increments for SPIB. + */ + if (hstream->running) { + if (runtime->control->appl_ptr < stream->old_appl_ptr) + return -EINVAL; + stream->old_appl_ptr = runtime->control->appl_ptr; + } + + 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 */ + if (!spib) + spib = buf_size; + + sof_io_write(sdev, stream->spib_addr, spib); + + return 0; +} + int hda_dsp_pcm_trigger(struct snd_sof_dev *sdev, struct snd_pcm_substream *substream, int cmd) { struct hdac_stream *hstream = substream->runtime->private_data; + struct snd_pcm_runtime *runtime = substream->runtime; struct hdac_ext_stream *stream = stream_to_hdac_ext_stream(hstream);
+ stream->old_appl_ptr = runtime->control->appl_ptr; + return hda_dsp_stream_trigger(sdev, stream, cmd); }
@@ -234,6 +280,13 @@ int hda_dsp_pcm_open(struct snd_sof_dev *sdev, return -EINVAL; }
+ /* + * if we want the .ack to work, we need to prevent the status and + * control from being mapped + */ + if (hda_disable_rewinds) + runtime->hw.info |= SNDRV_PCM_INFO_NO_STATUS_MMAP; + /* * All playback and D0i3 compatible streams are DMI L1 capable, others need * pause push/release to be disabled diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c index 4e49b7b16b4c..96b892dbf7b8 100644 --- a/sound/soc/sof/intel/hda-stream.c +++ b/sound/soc/sof/intel/hda-stream.c @@ -621,6 +621,8 @@ int hda_dsp_stream_hw_free(struct snd_sof_dev *sdev, SOF_HDA_REG_PP_PPCTL, mask, 0); spin_unlock_irq(&bus->reg_lock);
+ hda_dsp_stream_spib_config(sdev, link_dev, HDA_DSP_SPIB_DISABLE, 0); + stream->substream = NULL;
return 0; diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h index 4d44f8910393..a3a04890ca4f 100644 --- a/sound/soc/sof/intel/hda.h +++ b/sound/soc/sof/intel/hda.h @@ -543,6 +543,7 @@ int hda_dsp_pcm_trigger(struct snd_sof_dev *sdev, struct snd_pcm_substream *substream, int cmd); snd_pcm_uframes_t hda_dsp_pcm_pointer(struct snd_sof_dev *sdev, struct snd_pcm_substream *substream); +int hda_dsp_pcm_ack(struct snd_sof_dev *sdev, struct snd_pcm_substream *substream);
/* * DSP Stream Operations. diff --git a/sound/soc/sof/intel/tgl.c b/sound/soc/sof/intel/tgl.c index 2ed788304414..79b6e8a7c905 100644 --- a/sound/soc/sof/intel/tgl.c +++ b/sound/soc/sof/intel/tgl.c @@ -68,6 +68,7 @@ const struct snd_sof_dsp_ops sof_tgl_ops = { .pcm_hw_free = hda_dsp_stream_hw_free, .pcm_trigger = hda_dsp_pcm_trigger, .pcm_pointer = hda_dsp_pcm_pointer, + .pcm_ack = hda_dsp_pcm_ack,
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_PROBES) /* probe callbacks */
On Thu, 10 Jun 2021 22:53:26 +0200, Pierre-Louis Bossart wrote:
+/* update SPIB register with appl position */ +int hda_dsp_pcm_ack(struct snd_sof_dev *sdev, struct snd_pcm_substream *substream) +{
- struct hdac_stream *hstream = substream->runtime->private_data;
- struct hdac_ext_stream *stream = stream_to_hdac_ext_stream(hstream);
- struct snd_pcm_runtime *runtime = substream->runtime;
- ssize_t appl_pos, buf_size;
- u32 spib;
- if (!hda_disable_rewinds)
return 0;- /*
* paranoia check: if a rewind request took place after the RUN bits were programmed,* deny it since hardware only supports monotonic (modulo) increments for SPIB.*/- if (hstream->running) {
if (runtime->control->appl_ptr < stream->old_appl_ptr)return -EINVAL;
This condition won't be enough when the appl_ptr overlap the buffer boundary. It's still possible on 32bit architecture.
thanks,
Takashi
+/* update SPIB register with appl position */ +int hda_dsp_pcm_ack(struct snd_sof_dev *sdev, struct snd_pcm_substream *substream) +{
- struct hdac_stream *hstream = substream->runtime->private_data;
- struct hdac_ext_stream *stream = stream_to_hdac_ext_stream(hstream);
- struct snd_pcm_runtime *runtime = substream->runtime;
- ssize_t appl_pos, buf_size;
- u32 spib;
- if (!hda_disable_rewinds)
return 0;- /*
* paranoia check: if a rewind request took place after the RUN bits were programmed,* deny it since hardware only supports monotonic (modulo) increments for SPIB.*/- if (hstream->running) {
if (runtime->control->appl_ptr < stream->old_appl_ptr)return -EINVAL;This condition won't be enough when the appl_ptr overlap the buffer boundary. It's still possible on 32bit architecture.
And I missed this feedback as well...I only replied at the comments on module parameters/KConfig/controls/new API.
Takashi, is this saying that on 32-bit architectures there's no way to make the difference in the .ack implementation between - regular rewind and forward after the buffer max boundary - regular forward and rewind before the buffer zero boundary
If that was the case, the proposal made in this patch to validate the rewind with the .ack wouldn't work, we would have to go back to a filter in snd_pcm_rewind similar to what was initially suggested in [1]
[1] https://lore.kernel.org/alsa-devel/1494896518-23399-2-git-send-email-subhran...
On Thu, 10 Jun 2021 22:53:18 +0200, Pierre-Louis Bossart wrote:
This patchset provides two optimizations that result in significant power savings on Intel HDAudio platforms using SOF (Sound Open Firmware).
a) We previously prevented the Intel DSP from enabling the DMI_L1 capability to work-around issues with pause on capture streams. It turns out that this also prevented the platform from entering high C states in full-duplex usages such as videoconferencing - a rather basic use case since the start of the pandemic. The support for pause_push/release was already a bit controversial for Intel platforms, in theory platforms should only enable PAUSE if they can resume on the same sample, which is not the case on any Intel platform. Since we didn't want to disable a capability that could impact existing users, the suggestion is to optionally disable pause_push/release at build time or via a kernel parameter, in which case DMI_L1 is enabled. In practice very few applications make use of pause_push/release so there should be a limited impact when disabling this ALSA capability.
b) The use of the SPIB register also helps reduce power consumption, though to a smaller degree than DMI_L1. This hardware capability is however incompatible with userspace-initiated rewinds typically used by PulseAudio. In the past (2015..2017) Intel suggested an API extension to let applications disable rewinds. At the time the feedback was that such a capability was too Intel-specific and SPIB remained unused except for loading DSP code. We now see devices with smaller batteries being released, and it's time to revisit Linux support for SPIB to extend battery life. In this update the rewinds are disabled either at build-time or via a kernel parameter. As suggested by Takashi, a new SDNDRV_PCM_INFO flag is needed though to make sure the appl_ptr value is provided to the driver through the .ack callback. Distributions using PipeWire (Fedora34) and CRAS (ChromeOS/Chromium) can safely enable this option. Distributions using PulseAudio should probably avoid enabling it, although nothing is really fundamentally broken if they do. While in theory volume updates and mixing of notifications could be delayed, in practice distributions use small ring buffers that make such delays difficult to notice.
Again both of these updates are opt-in to avoid any impact on existing solutions or users: someone updating their kernel source but using 'make olddefconfig' will see the same results. Distributions that care neither about pause_push/release or rewinds should enable both options, in case of issues users will still be able to override these build-time choices with a simple module parameter.
Hmm, in general it's not easy for distros to decide which kconfig to take because most of distros ship both PulseAuadio and pipweire. It's rather the runtime choice, even different for each user at starting a different DE session, hence a kconfig or a module config doesn't fit well.
That said, it comes to the question about the severity of the change: how badly would behave if we disable the rewind? If the influence is limited, distros can take it as a cost of the better power-saving (which is often preferred). If PA's behavior change is significant, the option is way too dangerous, and it's hard to set as default.
thanks,
Takashi
On 11. 06. 21 9:58, Takashi Iwai wrote:
On Thu, 10 Jun 2021 22:53:18 +0200, Pierre-Louis Bossart wrote:
This patchset provides two optimizations that result in significant power savings on Intel HDAudio platforms using SOF (Sound Open Firmware).
a) We previously prevented the Intel DSP from enabling the DMI_L1 capability to work-around issues with pause on capture streams. It turns out that this also prevented the platform from entering high C states in full-duplex usages such as videoconferencing - a rather basic use case since the start of the pandemic. The support for pause_push/release was already a bit controversial for Intel platforms, in theory platforms should only enable PAUSE if they can resume on the same sample, which is not the case on any Intel platform. Since we didn't want to disable a capability that could impact existing users, the suggestion is to optionally disable pause_push/release at build time or via a kernel parameter, in which case DMI_L1 is enabled. In practice very few applications make use of pause_push/release so there should be a limited impact when disabling this ALSA capability.
b) The use of the SPIB register also helps reduce power consumption, though to a smaller degree than DMI_L1. This hardware capability is however incompatible with userspace-initiated rewinds typically used by PulseAudio. In the past (2015..2017) Intel suggested an API extension to let applications disable rewinds. At the time the feedback was that such a capability was too Intel-specific and SPIB remained unused except for loading DSP code. We now see devices with smaller batteries being released, and it's time to revisit Linux support for SPIB to extend battery life. In this update the rewinds are disabled either at build-time or via a kernel parameter. As suggested by Takashi, a new SDNDRV_PCM_INFO flag is needed though to make sure the appl_ptr value is provided to the driver through the .ack callback. Distributions using PipeWire (Fedora34) and CRAS (ChromeOS/Chromium) can safely enable this option. Distributions using PulseAudio should probably avoid enabling it, although nothing is really fundamentally broken if they do. While in theory volume updates and mixing of notifications could be delayed, in practice distributions use small ring buffers that make such delays difficult to notice.
Again both of these updates are opt-in to avoid any impact on existing solutions or users: someone updating their kernel source but using 'make olddefconfig' will see the same results. Distributions that care neither about pause_push/release or rewinds should enable both options, in case of issues users will still be able to override these build-time choices with a simple module parameter.
Hmm, in general it's not easy for distros to decide which kconfig to take because most of distros ship both PulseAuadio and pipweire. It's rather the runtime choice, even different for each user at starting a different DE session, hence a kconfig or a module config doesn't fit well.
That said, it comes to the question about the severity of the change: how badly would behave if we disable the rewind? If the influence is limited, distros can take it as a cost of the better power-saving (which is often preferred). If PA's behavior change is significant, the option is way too dangerous, and it's hard to set as default.
I would prefer to add a new API which will tell that the rewind support consumes more energy (is costly) and let apps to disable this feature when the user agreed. We should create an universal API without any sound server / application assumptions. We don't know beforehand, if users want the ultra low latencies for a purpose or they want to save the battery power.
The same objection is for the pcm mmap control suppression / pause trigger suppression.
The pulseaudio / pipewire code can be extended and it's better if both sides (driver / apps) agree on the protocol.
Jaroslav
Thanks Takashi and Jaroslav for your feedback
Hmm, in general it's not easy for distros to decide which kconfig to take because most of distros ship both PulseAuadio and pipweire. It's rather the runtime choice, even different for each user at starting a different DE session, hence a kconfig or a module config doesn't fit well.
That said, it comes to the question about the severity of the change: how badly would behave if we disable the rewind? If the influence is limited, distros can take it as a cost of the better power-saving (which is often preferred). If PA's behavior change is significant, the option is way too dangerous, and it's hard to set as default.
I've personally tried mucking with PulseAudio and didn't see any side effects. We do know that by design the effects of rewinds become significant if the HDAudio ring buffer becomes large (e.g 0.5..2s), but most distros keep the default size.
I would prefer to add a new API which will tell that the rewind support consumes more energy (is costly) and let apps to disable this feature when the user agreed. We should create an universal API without any sound server / application assumptions. We don't know beforehand, if users want the ultra low latencies for a purpose or they want to save the battery power.
The same objection is for the pcm mmap control suppression / pause trigger suppression.
The pulseaudio / pipewire code can be extended and it's better if both sides (driver / apps) agree on the protocol.
When I suggested an API extension (initial code in 2015) precisely to establish a 'contract' between userspace and driver, the answer from Takashi was this:
https://lore.kernel.org/alsa-devel/s5ha7uq7icw.wl-tiwai@suse.de/
"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."
What I am suggesting here is precisely the driver-specific implementation.
If both of you now prefer an API extension that's fine with me, that's what I preferred all along :-)
It's no big deal to bolt a userspace choice on top of those changes, but maybe we can do this as a second step?
I can remove the kconfig changes and only add kernel parameters in the mean time so that only early adopters make that selection. In a second step, these kernel parameters can be removed when applications make use of the new API extension.
Would this work for you?
I just want to stress that both of these changes result in significant power savings on Intel platforms. The world has changed since 2015 and the push to smaller batteries/longer battery life makes both of these changes very desirable. It's no longer an architecture-driven effort to enable new features, it's backed by real-world measurements on customer devices (which I can only disclose under NDA and not on a public mailing list obviously).
On 11. 06. 21 16:32, Pierre-Louis Bossart wrote:
Thanks Takashi and Jaroslav for your feedback
Hmm, in general it's not easy for distros to decide which kconfig to take because most of distros ship both PulseAuadio and pipweire. It's rather the runtime choice, even different for each user at starting a different DE session, hence a kconfig or a module config doesn't fit well.
That said, it comes to the question about the severity of the change: how badly would behave if we disable the rewind? If the influence is limited, distros can take it as a cost of the better power-saving (which is often preferred). If PA's behavior change is significant, the option is way too dangerous, and it's hard to set as default.
I've personally tried mucking with PulseAudio and didn't see any side effects. We do know that by design the effects of rewinds become significant if the HDAudio ring buffer becomes large (e.g 0.5..2s), but most distros keep the default size.
I would prefer to add a new API which will tell that the rewind support consumes more energy (is costly) and let apps to disable this feature when the user agreed. We should create an universal API without any sound server / application assumptions. We don't know beforehand, if users want the ultra low latencies for a purpose or they want to save the battery power.
The same objection is for the pcm mmap control suppression / pause trigger suppression.
The pulseaudio / pipewire code can be extended and it's better if both sides (driver / apps) agree on the protocol.
When I suggested an API extension (initial code in 2015) precisely to establish a 'contract' between userspace and driver, the answer from Takashi was this:
https://lore.kernel.org/alsa-devel/s5ha7uq7icw.wl-tiwai@suse.de/
"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."
What I am suggesting here is precisely the driver-specific implementation.
If both of you now prefer an API extension that's fine with me, that's what I preferred all along :-)
It's no big deal to bolt a userspace choice on top of those changes, but maybe we can do this as a second step?
I can remove the kconfig changes and only add kernel parameters in the mean time so that only early adopters make that selection. In a second step, these kernel parameters can be removed when applications make use of the new API extension.
Would this work for you?
Thinking more, my main objection is that we cannot change the runtime behaviour after the drivers are loaded in an easy way. I think that the current default settings should be kept without any Kconfig extensions. The module options are always good for the debugging, so they're fine in my eyes.
I see the Takashi's arguments, too.
Perhaps, it may be acceptable to add a global control enum (to the control API) for the ALSA card which may modify the driver behaviour / settings at runtime (normal operation, battery saving operation etc.). This control can be set in the UCM config. In this way, we don't need to touch the PCM API for the user space at all.
Jaroslav
Perhaps, it may be acceptable to add a global control enum (to the control API) for the ALSA card which may modify the driver behaviour / settings at runtime (normal operation, battery saving operation etc.). This control can be set in the UCM config. In this way, we don't need to touch the PCM API for the user space at all.
If there was a mechanism based on ALSA controls for an application to query capabilities and set what it want to disable that would be fine. hwdep would be fine as well.
I don't get though how this could be 'set in the UCM config', different apps might have different needs. UCM files don't currently make assumptions on which application uses them, do they?
On Fri, 11 Jun 2021 18:30:53 +0200, Pierre-Louis Bossart wrote:
Perhaps, it may be acceptable to add a global control enum (to the control API) for the ALSA card which may modify the driver behaviour / settings at runtime (normal operation, battery saving operation etc.). This control can be set in the UCM config. In this way, we don't need to touch the PCM API for the user space at all.
If there was a mechanism based on ALSA controls for an application to query capabilities and set what it want to disable that would be fine. hwdep would be fine as well.
I don't get though how this could be 'set in the UCM config', different apps might have different needs. UCM files don't currently make assumptions on which application uses them, do they?
I also came to the idea of kcontrol instead of module parameter, and hit to the differentiating via UCM, too. Maybe we may provide two different profiles and let apps choose. But, this reaches to the question: how to tell applications what is for. It's also the question if we extend the API.
Namely, the driver may inform user-space or the user-space may inform the driver whether to allow (or to use) the rewind. But, it doesn't explain what cost it would need. And that's a difficult task to generalize it. I can think of some API providing a preset per scenario, e.g. power-saving, large buffer, etc. But in this case, if rewind is allowed, what would it mean practically?
Then, this also led me a question: what happens if you disable tsched on PA? It'll be essentially equivalent behavior like pipewire, and this would be better? Note that the latest kernel already dropped the buffer pre-allocation for HD-audio, hence PA will take a large buffer (for one second) per default. I suppose that the influence of no rewind becomes noticeable, or would it still be OK with tsched?
If tsched=0 mode works reasonably well in case of no rewind, it can be done simply by setting SNDRV_PCM_INFO_BATCH together. Even with this change, we can keep the module parameter as a kill switch in case significant regression is found.
thanks,
Takashi
participants (3)
-
Jaroslav Kysela -
Pierre-Louis Bossart -
Takashi Iwai