[PATCH 0/8] ASoC: SOF: power optimizations for HDaudio platforms

[PATCH 0/8] ASoC: SOF: power optimizations for HDaudio platforms

[Pierre-Louis Bossart]

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(-)

-- 
2.25.1

[PATCH 1/8] ASoC: SOF: Intel: Kconfig: clarify DMI L1 option description

[Pierre-Louis Bossart]

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.
-- 
2.25.1

[PATCH 2/8] ASoC: SOF: Intel: simplify logic for DMI_L1 handling

[Pierre-Louis Bossart]

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);
 
-- 
2.25.1

[PATCH 3/8] ASoC: SOF: pcm: add mechanisms to disable ALSA pause_push/release

[Pierre-Louis Bossart]

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);
-- 
2.25.1

[PATCH 4/8] ASoC: SOF: Intel: add kernel parameter to set DMI L1 configuration

[Pierre-Louis Bossart]

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;
-- 
2.25.1

[PATCH 5/8] ASoC: SOF: Intel: enable DMI L1 when pause is not supported

[Pierre-Louis Bossart]

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)
-- 
2.25.1

[PATCH 6/8] ALSA: pcm: conditionally avoid mmap of control data

[Pierre-Louis Bossart]

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-subhransu.s.prusty@intel.com/

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);
-- 
2.25.1

Re: [PATCH 6/8] ALSA: pcm: conditionally avoid mmap of control data

[Takashi Iwai]

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-subhransu.s.prusty@intel.com/
> 
> 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/memalloc


Takashi

Re: [PATCH 6/8] ALSA: pcm: conditionally avoid mmap of control data

[Pierre-Louis Bossart]

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-subhransu.s.prusty@intel.com/
>>
>> 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/memalloc

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?

Re: [PATCH 6/8] ALSA: pcm: conditionally avoid mmap of control data

[Takashi Iwai]

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-subhransu.s.prusty@intel.com/
> >>
> >> 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/memalloc
> 
> 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

Re: [PATCH 6/8] ALSA: pcm: conditionally avoid mmap of control data

[Pierre-Louis Bossart]

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-subhransu.s.prusty@intel.com/
>>>>
>>>> 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/memalloc
>>
>> 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.

[PATCH 7/8] ASOC: SOF: pcm: add .ack callback support

[Pierre-Louis Bossart]

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,
-- 
2.25.1

[PATCH 8/8] ASoC: SOF: Intel: add .ack support for HDaudio platforms

[Pierre-Louis Bossart]

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 */
-- 
2.25.1

Re: [PATCH 8/8] ASoC: SOF: Intel: add .ack support for HDaudio platforms

[Takashi Iwai]

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

Re: [PATCH 8/8] ASoC: SOF: Intel: add .ack support for HDaudio platforms

[Pierre-Louis Bossart]

>> +/* 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-subhransu.s.prusty@intel.com/

Re: [PATCH 0/8] ASoC: SOF: power optimizations for HDaudio platforms

[Takashi Iwai]

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

Re: [PATCH 0/8] ASoC: SOF: power optimizations for HDaudio platforms

[Jaroslav Kysela]

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

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH 0/8] ASoC: SOF: power optimizations for HDaudio platforms

[Pierre-Louis Bossart]

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).

Re: [PATCH 0/8] ASoC: SOF: power optimizations for HDaudio platforms

[Jaroslav Kysela]

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

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH 0/8] ASoC: SOF: power optimizations for HDaudio platforms

[Pierre-Louis Bossart]

> 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?

Re: [PATCH 0/8] ASoC: SOF: power optimizations for HDaudio platforms

[Takashi Iwai]

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