[PATCH v3 0/4] ASoC: SOF: Intel: power optimizations with HDaudio SPIB register

[PATCH v3 0/4] ASoC: SOF: Intel: power optimizations with HDaudio SPIB register

[Pierre-Louis Bossart]

This patchset was initially provided in a larger series that was split
in two [1]. This part only provides support for the SPIB register
support, added on Intel platforms since Skylake (2015).

The use of the SPIB register 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 via an opt-in kernel
parameter. In the previous reviews, there was consensus that a Kconfig
option was too complicated for distributions to set, and we are
missing a TBD API to expose such capabilities to user-space.

The debate on whether or not to use rewinds, and the impact of
disabling rewinds, will likely be closed when Intel releases the
'deep-buffer' support, currently under development [2][3]. With this
solution, rewinds will not be needed, ever. When an application deals
with content that is not latency-sensitive (e.g. music playback), it
will be able to reduce power consumption by selecting a different PCM
device with increased buffering capabilities.  Low-latency streams
will be handled by the 'regular' path. In other words, the impossible
compromise between power and latency will be handled with different
PCM devices/profiles for the same endpoint, and we can push the design
of capability negotiation to a later time when all the building blocks
(firmware topology, kernel, userspace) are ready - we still have
firmware xruns, DPCM race conditions to solve, and a need to describe
these alternate PCM devices with UCM using 'modifiers'.

[1] https://lore.kernel.org/alsa-devel/20210610205326.1176400-1-pierre-louis.bossart@linux.intel.com/
[2] https://github.com/thesofproject/linux/pull/3146
[3] https://github.com/thesofproject/sof/pull/4611

Changes since v2:
Added check on appl_ptr (Takashi)
Moved rewind detection to deal with SYNC_PTR (Takashi)

Pierre-Louis Bossart (2):
  ALSA: pcm: unconditionally check if appl_ptr is in 0..boundary range
  ALSA: pcm: introduce INFO_NO_REWINDS flag

Ranjani Sridharan (2):
  ASOC: SOF: pcm: add .ack callback support
  ASoC: SOF: Intel: add .ack support for HDaudio platforms

 include/uapi/sound/asound.h      |  2 +-
 sound/core/pcm_lib.c             | 17 +++++++++++++
 sound/soc/sof/intel/apl.c        |  1 +
 sound/soc/sof/intel/cnl.c        |  1 +
 sound/soc/sof/intel/hda-pcm.c    | 41 ++++++++++++++++++++++++++++++--
 sound/soc/sof/intel/hda-stream.c |  2 ++
 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              |  9 +++++++
 sound/soc/sof/sof-priv.h         |  3 +++
 11 files changed, 85 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH v3 1/4] ALSA: pcm: unconditionally check if appl_ptr is in 0..boundary range

[Pierre-Louis Bossart]

In some cases, the appl_ptr passed by userspace is not checked before
being used. This patch adds an unconditional check and returns an
error code should the appl_ptr exceed the ALSA 'boundary'.

Suggested-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@intel.com>
---
 sound/core/pcm_lib.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index a144a3f68e9e..ec53a3e7cf35 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -2132,6 +2132,9 @@ int pcm_lib_apply_appl_ptr(struct snd_pcm_substream *substream,
 	if (old_appl_ptr == appl_ptr)
 		return 0;
 
+	if (appl_ptr >= runtime->boundary)
+		return -EINVAL;
+
 	runtime->control->appl_ptr = appl_ptr;
 	if (substream->ops->ack) {
 		ret = substream->ops->ack(substream);
-- 
2.25.1

[PATCH v3 2/4] ALSA: pcm: introduce INFO_NO_REWINDS flag

[Pierre-Louis Bossart]

When the hardware can only deal with a monotonically increasing
appl_ptr, this flag can be set.

In case the application requests a rewind, be it with a
snd_pcm_rewind() or with a direct change of a mmap'ed pointer followed
by a SNDRV_PCM_IOCTL_SYNC_PTR, this patch checks if a rewind
occurred and returns an error.

Credits to Takashi Iwai for identifying the path with SYNC_PTR and
suggesting the pointer checks.

Suggested-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@intel.com>
---
 include/uapi/sound/asound.h |  2 +-
 sound/core/pcm_lib.c        | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 5859ca0a1439..59fcf39f022e 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -300,7 +300,7 @@ typedef int __bitwise snd_pcm_subformat_t;
 #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_EXPLICIT_SYNC	0x10000000	/* needs explicit sync of pointers and data */
-
+#define SNDRV_PCM_INFO_NO_REWINDS	0x20000000	/* hardware can only support monotonic changes of appl_ptr */
 #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_lib.c b/sound/core/pcm_lib.c
index ec53a3e7cf35..1d3927aa3dd8 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -2127,6 +2127,7 @@ int pcm_lib_apply_appl_ptr(struct snd_pcm_substream *substream,
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	snd_pcm_uframes_t old_appl_ptr = runtime->control->appl_ptr;
+	snd_pcm_sframes_t diff;
 	int ret;
 
 	if (old_appl_ptr == appl_ptr)
@@ -2134,6 +2135,19 @@ int pcm_lib_apply_appl_ptr(struct snd_pcm_substream *substream,
 
 	if (appl_ptr >= runtime->boundary)
 		return -EINVAL;
+	/*
+	 * check if a rewind is requested by the application
+	 */
+	if (substream->runtime->info & SNDRV_PCM_INFO_NO_REWINDS) {
+		diff = appl_ptr - old_appl_ptr;
+		if (diff >= 0) {
+			if (diff > runtime->buffer_size)
+				return -EINVAL;
+		} else {
+			if (runtime->boundary + diff > runtime->buffer_size)
+				return -EINVAL;
+		}
+	}
 
 	runtime->control->appl_ptr = appl_ptr;
 	if (substream->ops->ack) {
-- 
2.25.1

[PATCH v3 3/4] 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: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
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 09bf38fdfb8a..8c58741176a0 100644
--- a/sound/soc/sof/ops.h
+++ b/sound/soc/sof/ops.h
@@ -454,6 +454,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 fa0bfcd2474e..cde3f65c4378 100644
--- a/sound/soc/sof/pcm.c
+++ b/sound/soc/sof/pcm.c
@@ -869,6 +869,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;
@@ -887,6 +895,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_DEBUG_PROBES)
 	pd->compress_ops = &sof_probe_compressed_ops;
diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index efaec2989a27..11f26ef6f447 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -206,6 +206,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 v3 4/4] 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. For simplicity, we set the
INFO_NO_REWINDS flag in the .open callback when we already need to
program the SNDRV_PCM_INFO_EXPLICIT_SYNC flag.

Rewinds are not used by many applications. One notable application
using rewinds is PulseAudio. Practical experiments with
Ubuntu/PulseAudio default settings did not show any audible issues,
but the user may hear volume changes and notification with a delay,
depending on the size of the ring buffer and latency constraints.

The choice of disabling rewinds is exposed as a kernel parameter and
not a Kconfig option to avoid any undesirable side-effects.

Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
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>
---
 sound/soc/sof/intel/apl.c        |  1 +
 sound/soc/sof/intel/cnl.c        |  1 +
 sound/soc/sof/intel/hda-pcm.c    | 41 ++++++++++++++++++++++++++++++--
 sound/soc/sof/intel/hda-stream.c |  2 ++
 sound/soc/sof/intel/hda.h        |  1 +
 sound/soc/sof/intel/tgl.c        |  1 +
 6 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sof/intel/apl.c b/sound/soc/sof/intel/apl.c
index 917f78cf6daf..689679014ade 100644
--- a/sound/soc/sof/intel/apl.c
+++ b/sound/soc/sof/intel/apl.c
@@ -78,6 +78,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 3957e2b3db32..d128c08ba726 100644
--- a/sound/soc/sof/intel/cnl.c
+++ b/sound/soc/sof/intel/cnl.c
@@ -283,6 +283,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 cc8ddef37f37..875350283eac 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,6 +147,29 @@ 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;
+
+	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)
 {
@@ -234,6 +264,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_REWINDS | SNDRV_PCM_INFO_EXPLICIT_SYNC;
+
 	/*
 	 * All playback streams are DMI L1 capable, capture streams 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 1d845c2cbc33..b6f037815344 100644
--- a/sound/soc/sof/intel/hda-stream.c
+++ b/sound/soc/sof/intel/hda-stream.c
@@ -655,6 +655,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 1195018a1f4f..6829d36fbfe9 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -533,6 +533,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 48da8e7a67bc..2a29058e0c20 100644
--- a/sound/soc/sof/intel/tgl.c
+++ b/sound/soc/sof/intel/tgl.c
@@ -73,6 +73,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 v3 4/4] ASoC: SOF: Intel: add .ack support for HDaudio platforms

[Takashi Iwai]

On Fri, 15 Oct 2021 21:59:32 +0200,
Pierre-Louis Bossart wrote:
> 
> 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. For simplicity, we set the
> INFO_NO_REWINDS flag in the .open callback when we already need to
> program the SNDRV_PCM_INFO_EXPLICIT_SYNC flag.

Using this flag itself isn't wrong, but if we need to check only
appl_ptr updates, a more appropriate flag is
SNDRV_PCM_INFO_SYNC_APPLPTR.  This will still allow the mmap of status
(i.e. hwptr update) while the mmap of control is disabled for
appl_ptr.  SNDRV_PCM_INFO_EXPLICIT_SYNC flag disables both, instead.


thanks,

Takashi

> Rewinds are not used by many applications. One notable application
> using rewinds is PulseAudio. Practical experiments with
> Ubuntu/PulseAudio default settings did not show any audible issues,
> but the user may hear volume changes and notification with a delay,
> depending on the size of the ring buffer and latency constraints.
> 
> The choice of disabling rewinds is exposed as a kernel parameter and
> not a Kconfig option to avoid any undesirable side-effects.
> 
> Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> 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>
> ---
>  sound/soc/sof/intel/apl.c        |  1 +
>  sound/soc/sof/intel/cnl.c        |  1 +
>  sound/soc/sof/intel/hda-pcm.c    | 41 ++++++++++++++++++++++++++++++--
>  sound/soc/sof/intel/hda-stream.c |  2 ++
>  sound/soc/sof/intel/hda.h        |  1 +
>  sound/soc/sof/intel/tgl.c        |  1 +
>  6 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/sof/intel/apl.c b/sound/soc/sof/intel/apl.c
> index 917f78cf6daf..689679014ade 100644
> --- a/sound/soc/sof/intel/apl.c
> +++ b/sound/soc/sof/intel/apl.c
> @@ -78,6 +78,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 3957e2b3db32..d128c08ba726 100644
> --- a/sound/soc/sof/intel/cnl.c
> +++ b/sound/soc/sof/intel/cnl.c
> @@ -283,6 +283,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 cc8ddef37f37..875350283eac 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,6 +147,29 @@ 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;
> +
> +	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)
>  {
> @@ -234,6 +264,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_REWINDS | SNDRV_PCM_INFO_EXPLICIT_SYNC;
> +
>  	/*
>  	 * All playback streams are DMI L1 capable, capture streams 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 1d845c2cbc33..b6f037815344 100644
> --- a/sound/soc/sof/intel/hda-stream.c
> +++ b/sound/soc/sof/intel/hda-stream.c
> @@ -655,6 +655,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 1195018a1f4f..6829d36fbfe9 100644
> --- a/sound/soc/sof/intel/hda.h
> +++ b/sound/soc/sof/intel/hda.h
> @@ -533,6 +533,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 48da8e7a67bc..2a29058e0c20 100644
> --- a/sound/soc/sof/intel/tgl.c
> +++ b/sound/soc/sof/intel/tgl.c
> @@ -73,6 +73,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 v3 1/4] ALSA: pcm: unconditionally check if appl_ptr is in 0..boundary range

[Takashi Iwai]

On Fri, 15 Oct 2021 21:59:29 +0200,
Pierre-Louis Bossart wrote:
> 
> In some cases, the appl_ptr passed by userspace is not checked before
> being used. This patch adds an unconditional check and returns an
> error code should the appl_ptr exceed the ALSA 'boundary'.
> 
> Suggested-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> Reviewed-by: Kai Vehmanen <kai.vehmanen@intel.com>

Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

> ---
>  sound/core/pcm_lib.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index a144a3f68e9e..ec53a3e7cf35 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -2132,6 +2132,9 @@ int pcm_lib_apply_appl_ptr(struct snd_pcm_substream *substream,
>  	if (old_appl_ptr == appl_ptr)
>  		return 0;
>  
> +	if (appl_ptr >= runtime->boundary)
> +		return -EINVAL;
> +
>  	runtime->control->appl_ptr = appl_ptr;
>  	if (substream->ops->ack) {
>  		ret = substream->ops->ack(substream);
> -- 
> 2.25.1
> 

Re: [PATCH v3 2/4] ALSA: pcm: introduce INFO_NO_REWINDS flag

[Takashi Iwai]

On Fri, 15 Oct 2021 21:59:30 +0200,
Pierre-Louis Bossart wrote:
> 
> When the hardware can only deal with a monotonically increasing
> appl_ptr, this flag can be set.
> 
> In case the application requests a rewind, be it with a
> snd_pcm_rewind() or with a direct change of a mmap'ed pointer followed
> by a SNDRV_PCM_IOCTL_SYNC_PTR, this patch checks if a rewind
> occurred and returns an error.
> 
> Credits to Takashi Iwai for identifying the path with SYNC_PTR and
> suggesting the pointer checks.
> 
> Suggested-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> Reviewed-by: Kai Vehmanen <kai.vehmanen@intel.com>

Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

> ---
>  include/uapi/sound/asound.h |  2 +-
>  sound/core/pcm_lib.c        | 14 ++++++++++++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 5859ca0a1439..59fcf39f022e 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -300,7 +300,7 @@ typedef int __bitwise snd_pcm_subformat_t;
>  #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_EXPLICIT_SYNC	0x10000000	/* needs explicit sync of pointers and data */
> -
> +#define SNDRV_PCM_INFO_NO_REWINDS	0x20000000	/* hardware can only support monotonic changes of appl_ptr */
>  #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_lib.c b/sound/core/pcm_lib.c
> index ec53a3e7cf35..1d3927aa3dd8 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -2127,6 +2127,7 @@ int pcm_lib_apply_appl_ptr(struct snd_pcm_substream *substream,
>  {
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  	snd_pcm_uframes_t old_appl_ptr = runtime->control->appl_ptr;
> +	snd_pcm_sframes_t diff;
>  	int ret;
>  
>  	if (old_appl_ptr == appl_ptr)
> @@ -2134,6 +2135,19 @@ int pcm_lib_apply_appl_ptr(struct snd_pcm_substream *substream,
>  
>  	if (appl_ptr >= runtime->boundary)
>  		return -EINVAL;
> +	/*
> +	 * check if a rewind is requested by the application
> +	 */
> +	if (substream->runtime->info & SNDRV_PCM_INFO_NO_REWINDS) {
> +		diff = appl_ptr - old_appl_ptr;
> +		if (diff >= 0) {
> +			if (diff > runtime->buffer_size)
> +				return -EINVAL;
> +		} else {
> +			if (runtime->boundary + diff > runtime->buffer_size)
> +				return -EINVAL;
> +		}
> +	}
>  
>  	runtime->control->appl_ptr = appl_ptr;
>  	if (substream->ops->ack) {
> -- 
> 2.25.1
> 

Re: [PATCH v3 3/4] ASOC: SOF: pcm: add .ack callback support

[Takashi Iwai]

On Fri, 15 Oct 2021 21:59:31 +0200,
Pierre-Louis Bossart wrote:
> 
> 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: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
> 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>

Just to point out that the subject prefix should be ASoC, not ASOC :)


thanks,

Takashi

Re: [PATCH v3 4/4] 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. For simplicity, we set the
>> INFO_NO_REWINDS flag in the .open callback when we already need to
>> program the SNDRV_PCM_INFO_EXPLICIT_SYNC flag.
> 
> Using this flag itself isn't wrong, but if we need to check only
> appl_ptr updates, a more appropriate flag is
> SNDRV_PCM_INFO_SYNC_APPLPTR.  This will still allow the mmap of status
> (i.e. hwptr update) while the mmap of control is disabled for
> appl_ptr.  SNDRV_PCM_INFO_EXPLICIT_SYNC flag disables both, instead.

I don't mind, but now we're officially out of INFO flags :-)

NO_REWINDS took the last available bit...

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

[Takashi Iwai]

On Mon, 18 Oct 2021 17:05:13 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> 
> >> 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. For simplicity, we set the
> >> INFO_NO_REWINDS flag in the .open callback when we already need to
> >> program the SNDRV_PCM_INFO_EXPLICIT_SYNC flag.
> > 
> > Using this flag itself isn't wrong, but if we need to check only
> > appl_ptr updates, a more appropriate flag is
> > SNDRV_PCM_INFO_SYNC_APPLPTR.  This will still allow the mmap of status
> > (i.e. hwptr update) while the mmap of control is disabled for
> > appl_ptr.  SNDRV_PCM_INFO_EXPLICIT_SYNC flag disables both, instead.
> 
> I don't mind, but now we're officially out of INFO flags :-)
> 
> NO_REWINDS took the last available bit...

I mean only about the use EXPLICIT_SYNC flag.  There has been already
an info flag SYNC_APPLPTR, and this should suffice for your purpose.
In a nutshell:

EXPLICIT_SYNC = disable both control and status mmaps
SYNC_APPLPTR = disable only control mmap


Takashi

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

[Pierre-Louis Bossart]

> I mean only about the use EXPLICIT_SYNC flag.  There has been already
> an info flag SYNC_APPLPTR, and this should suffice for your purpose.
> In a nutshell:
> 
> EXPLICIT_SYNC = disable both control and status mmaps
> SYNC_APPLPTR = disable only control mmap

Humm, are you sure Takashi? it's been a long time since we discussed
this and your initial direction was to disable both?

To quote your own words from
https://lore.kernel.org/alsa-devel/s5hfug51g0x.wl-tiwai@suse.de/

"In mmap mode, we transfer data on the mmap
buffer, and update appl_ptr via mmap control.  Both are done without
notification to the driver (which is intentional for avoiding the
context switching).

So we want to disable this optimization and always notify to the
driver.  Disabling mmap status/control is the straight hack as it
falls back to ioctl and then the driver can know the change."

I really don't mind changing, I don't have enough background on this,
just wanted to make sure that disabling the control mmap is sufficient
on paper before we re-run tests. Thanks!

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

[Takashi Iwai]

On Mon, 18 Oct 2021 18:57:12 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> 
> 
> > I mean only about the use EXPLICIT_SYNC flag.  There has been already
> > an info flag SYNC_APPLPTR, and this should suffice for your purpose.
> > In a nutshell:
> > 
> > EXPLICIT_SYNC = disable both control and status mmaps
> > SYNC_APPLPTR = disable only control mmap
> 
> Humm, are you sure Takashi? it's been a long time since we discussed
> this and your initial direction was to disable both?
> 
> To quote your own words from
> https://lore.kernel.org/alsa-devel/s5hfug51g0x.wl-tiwai@suse.de/
> 
> "In mmap mode, we transfer data on the mmap
> buffer, and update appl_ptr via mmap control.  Both are done without
> notification to the driver (which is intentional for avoiding the
> context switching).
> 
> So we want to disable this optimization and always notify to the
> driver.  Disabling mmap status/control is the straight hack as it
> falls back to ioctl and then the driver can know the change."

The above text was correct, per se.  There was no *_SYNC_APPLPTR flag
at that time, hence the only option was to disable both mmaps (that
was done for 32bit PCM compat streams).

But now we have two ways to disable, as mentioned in the previous
mail, that allows selectively disabling only the control mmap, which
is required in most cases.

> I really don't mind changing, I don't have enough background on this,
> just wanted to make sure that disabling the control mmap is sufficient
> on paper before we re-run tests. Thanks!

Yes, please verify.


thanks,

Takashi