[PATCH 0/2] ASoC: SOF: pcm/Intel: Pause-resume improvements for IPC4

[PATCH 0/2] ASoC: SOF: pcm/Intel: Pause-resume improvements for IPC4

[Peter Ujfalusi]

Hi,

last minute patch for correct the pasue/resume operation with IPC4.

The issues are hardto reproduce and needs extended stress testing to be hit, in
which case the audio breaks due to DMA errors.

Regards,
Peter
---
Ranjani Sridharan (2):
  ASoC: SOF: Intel: hda: Do not stop/start DMA during pause/release
  ASoC: SOF: pcm: Add an option to skip platform trigger during stop

 sound/soc/sof/intel/hda-stream.c | 11 ++++++++++-
 sound/soc/sof/ipc4-pcm.c         |  3 ++-
 sound/soc/sof/pcm.c              | 23 +++++++++++++++--------
 sound/soc/sof/sof-audio.c        | 22 ++++++++++++++--------
 sound/soc/sof/sof-audio.h        |  6 ++++++
 5 files changed, 47 insertions(+), 18 deletions(-)

-- 
2.40.0

[PATCH 1/2] ASoC: SOF: Intel: hda: Do not stop/start DMA during pause/release

[Peter Ujfalusi]

From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

The FW does not pause/stop the host DMA during pause and stopping the
host DMA from the driver could result in an unknown behaviour. So, skip
triggering the HD-Audio host DMA during pause/release.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/sof/intel/hda-stream.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/sound/soc/sof/intel/hda-stream.c b/sound/soc/sof/intel/hda-stream.c
index 79d818e6a0fa..8de422604ad5 100644
--- a/sound/soc/sof/intel/hda-stream.c
+++ b/sound/soc/sof/intel/hda-stream.c
@@ -337,7 +337,13 @@ int hda_dsp_stream_trigger(struct snd_sof_dev *sdev,
 	/* cmd must be for audio stream */
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		if (!sdev->dspless_mode_selected)
+			break;
+		fallthrough;
 	case SNDRV_PCM_TRIGGER_START:
+		if (hstream->running)
+			break;
+
 		snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, SOF_HDA_INTCTL,
 					1 << hstream->index,
 					1 << hstream->index);
@@ -360,8 +366,11 @@ int hda_dsp_stream_trigger(struct snd_sof_dev *sdev,
 			hstream->running = true;
 
 		break;
-	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		if (!sdev->dspless_mode_selected)
+			break;
+		fallthrough;
+	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_STOP:
 		snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
 					sd_offset,
-- 
2.40.0

[PATCH 2/2] ASoC: SOF: pcm: Add an option to skip platform trigger during stop

[Peter Ujfalusi]

From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

In the case of IPC4, a pipeline is only paused during STOP/PAUSE/SUSPEND
triggers and the FW keeps the host DMA running when a pipeline is
paused. The start/stop tests iterate through STOP/START triggers without
involving a hw_free. This means that the pipeline state will only toggle
between PAUSED (during the STOP trigger) and RUNNING (during the START
trigger). So this test should be treated in the same way as a
PAUSE_PUSH/PAUSE_RELEASE test and the DMA should be kept running when
toggling the pipeline states between PAUSED and RUNNING.

Since there is no way to tell if a STOP trigger will be followed by hw_free
or not, this patch proposes to always skip DMA stop during the STOP trigger
and handle it later during hw_free. Introduce a new flag in struct
sof_ipc_pcm_ops, delayed_platform_trigger, that will be used to ensure that
the host DMA will not be stopped during the STOP/PAUSE/RELEASE triggers
and set it for IPC4. The platform_trigger call to stop the DMA will be
invoked during PCM hw_free instead when the pipeline is reset.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/sof/ipc4-pcm.c  |  3 ++-
 sound/soc/sof/pcm.c       | 23 +++++++++++++++--------
 sound/soc/sof/sof-audio.c | 22 ++++++++++++++--------
 sound/soc/sof/sof-audio.h |  6 ++++++
 4 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/sound/soc/sof/ipc4-pcm.c b/sound/soc/sof/ipc4-pcm.c
index a2cd21256e44..ea19bd0330e2 100644
--- a/sound/soc/sof/ipc4-pcm.c
+++ b/sound/soc/sof/ipc4-pcm.c
@@ -836,5 +836,6 @@ const struct sof_ipc_pcm_ops ipc4_pcm_ops = {
 	.pcm_setup = sof_ipc4_pcm_setup,
 	.pcm_free = sof_ipc4_pcm_free,
 	.delay = sof_ipc4_pcm_delay,
-	.ipc_first_on_start = true
+	.ipc_first_on_start = true,
+	.platform_stop_during_hw_free = true,
 };
diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c
index 127b68caf9e1..567db32173a8 100644
--- a/sound/soc/sof/pcm.c
+++ b/sound/soc/sof/pcm.c
@@ -211,16 +211,22 @@ static int sof_pcm_hw_free(struct snd_soc_component *component,
 	dev_dbg(component->dev, "pcm: free stream %d dir %d\n",
 		spcm->pcm.pcm_id, substream->stream);
 
-	/* free PCM in the DSP */
-	if (pcm_ops && pcm_ops->hw_free && spcm->prepared[substream->stream]) {
-		ret = pcm_ops->hw_free(component, substream);
-		if (ret < 0)
-			err = ret;
+	if (spcm->prepared[substream->stream]) {
+		/* stop DMA first if needed */
+		if (pcm_ops && pcm_ops->platform_stop_during_hw_free)
+			snd_sof_pcm_platform_trigger(sdev, substream, SNDRV_PCM_TRIGGER_STOP);
+
+		/* free PCM in the DSP */
+		if (pcm_ops && pcm_ops->hw_free) {
+			ret = pcm_ops->hw_free(component, substream);
+			if (ret < 0)
+				err = ret;
+		}
 
 		spcm->prepared[substream->stream] = false;
 	}
 
-	/* stop DMA */
+	/* reset DMA */
 	ret = snd_sof_pcm_platform_hw_free(sdev, substream);
 	if (ret < 0) {
 		dev_err(component->dev, "error: platform hw free failed\n");
@@ -362,8 +368,9 @@ static int sof_pcm_trigger(struct snd_soc_component *component,
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 	case SNDRV_PCM_TRIGGER_STOP:
-		/* invoke platform trigger to stop DMA even if pcm_ops failed */
-		snd_sof_pcm_platform_trigger(sdev, substream, cmd);
+		/* invoke platform trigger to stop DMA even if pcm_ops isn't set or if it failed */
+		if (!pcm_ops || (pcm_ops && !pcm_ops->platform_stop_during_hw_free))
+			snd_sof_pcm_platform_trigger(sdev, substream, cmd);
 		break;
 	default:
 		break;
diff --git a/sound/soc/sof/sof-audio.c b/sound/soc/sof/sof-audio.c
index 7651644fcd62..1cbda595c518 100644
--- a/sound/soc/sof/sof-audio.c
+++ b/sound/soc/sof/sof-audio.c
@@ -805,16 +805,22 @@ int sof_pcm_stream_free(struct snd_sof_dev *sdev, struct snd_pcm_substream *subs
 	const struct sof_ipc_pcm_ops *pcm_ops = sof_ipc_get_ops(sdev, pcm);
 	int ret;
 
-	/* Send PCM_FREE IPC to reset pipeline */
-	if (pcm_ops && pcm_ops->hw_free && spcm->prepared[substream->stream]) {
-		ret = pcm_ops->hw_free(sdev->component, substream);
-		if (ret < 0)
-			return ret;
-	}
+	if (spcm->prepared[substream->stream]) {
+		/* stop DMA first if needed */
+		if (pcm_ops && pcm_ops->platform_stop_during_hw_free)
+			snd_sof_pcm_platform_trigger(sdev, substream, SNDRV_PCM_TRIGGER_STOP);
+
+		/* Send PCM_FREE IPC to reset pipeline */
+		if (pcm_ops && pcm_ops->hw_free) {
+			ret = pcm_ops->hw_free(sdev->component, substream);
+			if (ret < 0)
+				return ret;
+		}
 
-	spcm->prepared[substream->stream] = false;
+		spcm->prepared[substream->stream] = false;
+	}
 
-	/* stop the DMA */
+	/* reset the DMA */
 	ret = snd_sof_pcm_platform_hw_free(sdev, substream);
 	if (ret < 0)
 		return ret;
diff --git a/sound/soc/sof/sof-audio.h b/sound/soc/sof/sof-audio.h
index 6c64376858b3..a090a9eb4828 100644
--- a/sound/soc/sof/sof-audio.h
+++ b/sound/soc/sof/sof-audio.h
@@ -108,6 +108,11 @@ struct snd_sof_dai_config_data {
  *				 STOP pcm trigger
  * @ipc_first_on_start: Send IPC before invoking platform trigger during
  *				START/PAUSE_RELEASE triggers
+ * @platform_stop_during_hw_free: Invoke the platform trigger during hw_free. This is needed for
+ *				  IPC4 where a pipeline is only paused during stop/pause/suspend
+ *				  triggers. The FW keeps the host DMA running in this case and
+ *				  therefore the host must do the same and should stop the DMA during
+ *				  hw_free.
  */
 struct sof_ipc_pcm_ops {
 	int (*hw_params)(struct snd_soc_component *component, struct snd_pcm_substream *substream,
@@ -123,6 +128,7 @@ struct sof_ipc_pcm_ops {
 				   struct snd_pcm_substream *substream);
 	bool reset_hw_params_during_stop;
 	bool ipc_first_on_start;
+	bool platform_stop_during_hw_free;
 };
 
 /**
-- 
2.40.0

Re: [PATCH 0/2] ASoC: SOF: pcm/Intel: Pause-resume improvements for IPC4

[Mark Brown]

On Thu, 20 Apr 2023 14:41:35 +0300, Peter Ujfalusi wrote:
> last minute patch for correct the pasue/resume operation with IPC4.
> 
> The issues are hardto reproduce and needs extended stress testing to be hit, in
> which case the audio breaks due to DMA errors.
> 
> Regards,
> Peter
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/2] ASoC: SOF: Intel: hda: Do not stop/start DMA during pause/release
      commit: 3e94369729ea8a825cf8bf304bfb1749de62ebf4
[2/2] ASoC: SOF: pcm: Add an option to skip platform trigger during stop
      commit: 6d0a21dd95c349bbe3663a4870ff7e70ddc6c9b6

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark