[PATCH 0/2] ASoC: SOF: Intel/ipc4: Do not reset BE DAI pipeline during stop/suspend

[PATCH 0/2] ASoC: SOF: Intel/ipc4: Do not reset BE DAI pipeline during stop/suspend

[Peter Ujfalusi]

Hi,

Do not reset pipelines during the stop/suspend triggers in the BE DAI
ops as the BE DAI pipeline needs to be left in the PAUSED state. It should
only be reset during hw_free. This simplification is already done for
the FE pipelines and the DAI trigger only toggles the states between
PAUSED and RUNNING.

Regards
Peter
---
Ranjani Sridharan (2):
  ASoC: SOF: Intel: hda-dai-ops: Split the get_hext_stream() op for IPC4
  ASoC: SOF: ipc4-pcm: reset all pipelines during FE DAI hw_free

 sound/soc/sof/intel/hda-dai-ops.c | 69 +++++++++++++++++++++++--------
 sound/soc/sof/ipc4-pcm.c          |  4 +-
 sound/soc/sof/ipc4-topology.c     |  1 -
 3 files changed, 53 insertions(+), 21 deletions(-)

-- 
2.40.1

[PATCH 1/2] ASoC: SOF: Intel: hda-dai-ops: Split the get_hext_stream() op for IPC4

[Peter Ujfalusi]

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

Introduce a separate op implementation for get_hext_stream() for IPC4.
This op will also be used to set the skip_during_fe_trigger flag for the
BE DAI pipeline. With this change, we can remove the flag setting in
sof_ipc4_dai_config() which will further simplify support for
DMIC/SSP/Soundwire in the LunarLake platform.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/sof/intel/hda-dai-ops.c | 22 +++++++++++++++++++++-
 sound/soc/sof/ipc4-topology.c     |  1 -
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sof/intel/hda-dai-ops.c b/sound/soc/sof/intel/hda-dai-ops.c
index 4b39cecacd68..4c04adff4c45 100644
--- a/sound/soc/sof/intel/hda-dai-ops.c
+++ b/sound/soc/sof/intel/hda-dai-ops.c
@@ -120,6 +120,26 @@ static struct hdac_ext_stream *hda_get_hext_stream(struct snd_sof_dev *sdev,
 	return snd_soc_dai_get_dma_data(cpu_dai, substream);
 }
 
+static struct hdac_ext_stream *hda_ipc4_get_hext_stream(struct snd_sof_dev *sdev,
+							struct snd_soc_dai *cpu_dai,
+							struct snd_pcm_substream *substream)
+{
+	struct snd_sof_widget *pipe_widget;
+	struct sof_ipc4_pipeline *pipeline;
+	struct snd_sof_widget *swidget;
+	struct snd_soc_dapm_widget *w;
+
+	w = snd_soc_dai_get_widget(cpu_dai, substream->stream);
+	swidget = w->dobj.private;
+	pipe_widget = swidget->spipe->pipe_widget;
+	pipeline = pipe_widget->private;
+
+	/* mark pipeline so that it can be skipped during FE trigger */
+	pipeline->skip_during_fe_trigger = true;
+
+	return snd_soc_dai_get_dma_data(cpu_dai, substream);
+}
+
 static struct hdac_ext_stream *hda_assign_hext_stream(struct snd_sof_dev *sdev,
 						      struct snd_soc_dai *cpu_dai,
 						      struct snd_pcm_substream *substream)
@@ -267,7 +287,7 @@ static int hda_ipc4_post_trigger(struct snd_sof_dev *sdev, struct snd_soc_dai *c
 }
 
 static const struct hda_dai_widget_dma_ops hda_ipc4_dma_ops = {
-	.get_hext_stream = hda_get_hext_stream,
+	.get_hext_stream = hda_ipc4_get_hext_stream,
 	.assign_hext_stream = hda_assign_hext_stream,
 	.release_hext_stream = hda_release_hext_stream,
 	.setup_hext_stream = hda_setup_hext_stream,
diff --git a/sound/soc/sof/ipc4-topology.c b/sound/soc/sof/ipc4-topology.c
index 059eebf0a687..91a89ac9cec3 100644
--- a/sound/soc/sof/ipc4-topology.c
+++ b/sound/soc/sof/ipc4-topology.c
@@ -2507,7 +2507,6 @@ static int sof_ipc4_dai_config(struct snd_sof_dev *sdev, struct snd_sof_widget *
 		}
 		gtw_attr = ipc4_copier->gtw_attr;
 		gtw_attr->lp_buffer_alloc = pipeline->lp_mode;
-		pipeline->skip_during_fe_trigger = true;
 		fallthrough;
 	case SOF_DAI_INTEL_ALH:
 		/*
-- 
2.40.1

[PATCH 2/2] ASoC: SOF: ipc4-pcm: reset all pipelines during FE DAI hw_free

[Peter Ujfalusi]

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

Do not reset pipelines during the stop/suspend triggers in the BE DAI
ops as the BE DAI pipeline needs to be left in the PAUSED state. It should
only be reset during hw_free. This simplification is already done for
the FE pipelines and the DAI trigger only toggles the states between
PAUSED and RUNNING.

But because the FE DAI hw_free is invoked first and all the pipelines are
freed during this op, we need to make sure that the BE DAI pipeline also
gets reset before it is freed. So do not skip the pipelines that have the
skip_during_fe_trigger flag set when resetting pipelines.

Also, because the pipeline state changes are split between the FE and BE
DAI ops now, protect the BE DAI pipeline state changes with the
pipeline_state_mutex as well.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/sof/intel/hda-dai-ops.c | 47 ++++++++++++++++++++-----------
 sound/soc/sof/ipc4-pcm.c          |  4 +--
 2 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/sound/soc/sof/intel/hda-dai-ops.c b/sound/soc/sof/intel/hda-dai-ops.c
index 4c04adff4c45..f121b9e31708 100644
--- a/sound/soc/sof/intel/hda-dai-ops.c
+++ b/sound/soc/sof/intel/hda-dai-ops.c
@@ -178,6 +178,7 @@ static void hda_reset_hext_stream(struct snd_sof_dev *sdev, struct hdac_ext_stre
 static int hda_ipc4_pre_trigger(struct snd_sof_dev *sdev, struct snd_soc_dai *cpu_dai,
 				struct snd_pcm_substream *substream, int cmd)
 {
+	struct sof_ipc4_fw_data *ipc4_data = sdev->private;
 	struct snd_sof_widget *pipe_widget;
 	struct sof_ipc4_pipeline *pipeline;
 	struct snd_sof_widget *swidget;
@@ -189,6 +190,8 @@ static int hda_ipc4_pre_trigger(struct snd_sof_dev *sdev, struct snd_soc_dai *cp
 	pipe_widget = swidget->spipe->pipe_widget;
 	pipeline = pipe_widget->private;
 
+	mutex_lock(&ipc4_data->pipeline_state_mutex);
+
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
@@ -199,7 +202,7 @@ static int hda_ipc4_pre_trigger(struct snd_sof_dev *sdev, struct snd_soc_dai *cp
 		ret = sof_ipc4_set_pipeline_state(sdev, pipe_widget->instance_id,
 						  SOF_IPC4_PIPE_PAUSED);
 		if (ret < 0)
-			return ret;
+			goto out;
 
 		pipeline->state = SOF_IPC4_PIPE_PAUSED;
 		break;
@@ -207,7 +210,8 @@ static int hda_ipc4_pre_trigger(struct snd_sof_dev *sdev, struct snd_soc_dai *cp
 		dev_err(sdev->dev, "unknown trigger command %d\n", cmd);
 		return -EINVAL;
 	}
-
+out:
+	mutex_unlock(&ipc4_data->pipeline_state_mutex);
 	return 0;
 }
 
@@ -237,53 +241,62 @@ static int hda_trigger(struct snd_sof_dev *sdev, struct snd_soc_dai *cpu_dai,
 static int hda_ipc4_post_trigger(struct snd_sof_dev *sdev, struct snd_soc_dai *cpu_dai,
 				 struct snd_pcm_substream *substream, int cmd)
 {
+	struct sof_ipc4_fw_data *ipc4_data = sdev->private;
 	struct snd_sof_widget *pipe_widget;
 	struct sof_ipc4_pipeline *pipeline;
 	struct snd_sof_widget *swidget;
 	struct snd_soc_dapm_widget *w;
-	int ret;
+	int ret = 0;
 
 	w = snd_soc_dai_get_widget(cpu_dai, substream->stream);
 	swidget = w->dobj.private;
 	pipe_widget = swidget->spipe->pipe_widget;
 	pipeline = pipe_widget->private;
 
+	mutex_lock(&ipc4_data->pipeline_state_mutex);
+
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
-	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		if (pipeline->state != SOF_IPC4_PIPE_PAUSED) {
 			ret = sof_ipc4_set_pipeline_state(sdev, pipe_widget->instance_id,
 							  SOF_IPC4_PIPE_PAUSED);
 			if (ret < 0)
-				return ret;
+				goto out;
 			pipeline->state = SOF_IPC4_PIPE_PAUSED;
 		}
 
 		ret = sof_ipc4_set_pipeline_state(sdev, pipe_widget->instance_id,
 						  SOF_IPC4_PIPE_RUNNING);
 		if (ret < 0)
-			return ret;
+			goto out;
 		pipeline->state = SOF_IPC4_PIPE_RUNNING;
+		swidget->spipe->started_count++;
 		break;
-	case SNDRV_PCM_TRIGGER_SUSPEND:
-	case SNDRV_PCM_TRIGGER_STOP:
-	{
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		ret = sof_ipc4_set_pipeline_state(sdev, pipe_widget->instance_id,
-						  SOF_IPC4_PIPE_RESET);
+						  SOF_IPC4_PIPE_RUNNING);
 		if (ret < 0)
-			return ret;
-
-		pipeline->state = SOF_IPC4_PIPE_RESET;
+			goto out;
+		pipeline->state = SOF_IPC4_PIPE_RUNNING;
+		break;
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_STOP:
+		/*
+		 * STOP/SUSPEND trigger is invoked only once when all users of this pipeline have
+		 * been stopped. So, clear the started_count so that the pipeline can be reset
+		 */
+		swidget->spipe->started_count = 0;
 		break;
-	}
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 		break;
 	default:
 		dev_err(sdev->dev, "unknown trigger command %d\n", cmd);
-		return -EINVAL;
+		ret = -EINVAL;
+		break;
 	}
-
-	return 0;
+out:
+	mutex_unlock(&ipc4_data->pipeline_state_mutex);
+	return ret;
 }
 
 static const struct hda_dai_widget_dma_ops hda_ipc4_dma_ops = {
diff --git a/sound/soc/sof/ipc4-pcm.c b/sound/soc/sof/ipc4-pcm.c
index 9e2b6c45080d..0c905bd0fab4 100644
--- a/sound/soc/sof/ipc4-pcm.c
+++ b/sound/soc/sof/ipc4-pcm.c
@@ -69,7 +69,7 @@ sof_ipc4_add_pipeline_to_trigger_list(struct snd_sof_dev *sdev, int state,
 	struct snd_sof_widget *pipe_widget = spipe->pipe_widget;
 	struct sof_ipc4_pipeline *pipeline = pipe_widget->private;
 
-	if (pipeline->skip_during_fe_trigger)
+	if (pipeline->skip_during_fe_trigger && state != SOF_IPC4_PIPE_RESET)
 		return;
 
 	switch (state) {
@@ -108,7 +108,7 @@ sof_ipc4_update_pipeline_state(struct snd_sof_dev *sdev, int state, int cmd,
 	struct sof_ipc4_pipeline *pipeline = pipe_widget->private;
 	int i;
 
-	if (pipeline->skip_during_fe_trigger)
+	if (pipeline->skip_during_fe_trigger && state != SOF_IPC4_PIPE_RESET)
 		return;
 
 	/* set state for pipeline if it was just triggered */
-- 
2.40.1

Re: [PATCH 0/2] ASoC: SOF: Intel/ipc4: Do not reset BE DAI pipeline during stop/suspend

[Mark Brown]

On Mon, 15 May 2023 14:20:20 +0300, Peter Ujfalusi wrote:
> Do not reset pipelines during the stop/suspend triggers in the BE DAI
> ops as the BE DAI pipeline needs to be left in the PAUSED state. It should
> only be reset during hw_free. This simplification is already done for
> the FE pipelines and the DAI trigger only toggles the states between
> PAUSED and RUNNING.
> 
> 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-dai-ops: Split the get_hext_stream() op for IPC4
      commit: 81a5d699217d1ae2853d6b022fc110aa95a2ff52
[2/2] ASoC: SOF: ipc4-pcm: reset all pipelines during FE DAI hw_free
      commit: 225f37b578a9f6462afd46c976e31977f765c38b

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