Refactor pipeline management #5262

ranj063 · 2024-12-04T04:18:37Z

This PR refactors the pipeline management to do the following:

Make sure that BE pipeline widgets/routes are set up and freed during BE DAI ops. A BE pipeline is the one that contains the DAI widget
Pipeline trigger is already split between FE and BE DAI ops except for pipeline reset. This PR also modifies pipeline set/up free to the respective DAI ops i.e BE pipeline in the BE DAI hw_free and the rest of the pipelines in the FE DAI hw_free.
The widget's ipc_prepare op is intentionally left to the handled during the FE DAI hw_params because it requires propagating pipeline_params all the way from the source to the sink widget.

So the overall flow for a normal start/stop would be:

Pipeline widgets are prepared during FE DAI hw_params. This includes all widgets belonging to all pipelines including the BE pipeline as well.
All widgets and routes belonging to BE pipelines are set up during BE DAI prepare.
All widgets and routes belonging to the remaining pipelines are set up during FE DAI prepare.
All widgets and routes belonging to BE pipelines are freed and unprepared during BE DAI hw_free
All widgets and routes belonging to the remaining pipelines are freed and unprepared during FE DAI hw_free

The above flow ensures that the pipelines are unprepared only during hw_free. Since the hw_params do not change because of xruns/suspend, there is no need to unprepare and re-prepare the widgets in these cases. The only critical change would be to make sure that the host DMA and the link DMA are reconfigured before restarting.

In the case of an xrun, the change would be that the BE pipeline would be freed, the link DMA will be reconfigured and the pipelines will be set up again in the prepare following the xrun stop trigger. And the remaining pipelines would be freed, host DMA reconfigured and the pipelines will be set up again in the prepare ioctl following the xrun stop trigger.

The case of a suspend/resume during audio is slightly different from the xrun case: The BE pipeline will be freed during the suspend trigger. The link DMA will be reconfigured and the BE pipeline will be set up again during the BE DAI prepare after resuming. Similarly, for the remaining pipelines, they will be freed during suspend and the host DMA will be reconfigured and pipelines set up during the prepare after resume.

ranj063 · 2024-12-04T19:41:14Z

SOFCI TEST

bardliao

Can we call setup in hw_params and prepare in prepare?

bardliao · 2024-12-06T03:25:05Z

sound/soc/sof/pcm.c

+	list = spcm->stream[dir].list;
+	params = &spcm->params[substream->stream];
+	platform_params = &spcm->platform_params[substream->stream];
+	ret = sof_widget_list_setup(sdev, spcm, params, platform_params, dir);


As setup and prepare are split, why don't we do setup in sof_pcm_hw_params?

we dont do setup in hw_params @bardliao. See my earlier comment. Its widget_prepare in hw_params and widget setup in prepare

bardliao · 2024-12-06T03:35:25Z

sound/soc/sof/intel/hda-dai.c

 	int stream = substream->stream;
+	int ret;
+
+	ret = hda_dai_hw_params(substream, &rtd->dpcm[stream].hw_params, dai);


Is there any specific reason that we always call hw_params in .prepare?

because sometimes we dont have a hw_free like in the case of xruns and we'll need to do everything again.

prepare() should be fast. We should call this out in comments - we need to do X,Y out of cycle because Z

drivers/soundwire/intel_ace2x.c

ranj063 · 2024-12-06T04:38:00Z

Can we call setup in hw_params and prepare in prepare?

No. A widget needs to be prepared before it can be set up. So ipc_prepare needs to be done in hw_params before we do set up in prepare

ranj063 · 2024-12-07T02:15:53Z

SOFCI TEST

ranj063 · 2024-12-08T01:59:55Z

SOFCI TEST

sound/soc/sof/intel/hda-dai-ops.c

bardliao · 2024-12-09T06:43:51Z

SOFCI TEST

ujfalusi · 2024-12-09T11:53:39Z

@ranj063, this sequence still fails (HDA machine):
terminal 1:
aplay -Dhw:0,0
CTRL+z

terminal 2:
aplay -Dhw:0,3

terminal 1:
fg

[11038.698037] snd_soc_core:dpcm_be_dai_trigger:  Analog Playback and Capture: ASoC: trigger BE Analog Playback and Capture cmd 1
[11038.698057] snd_sof_intel_hda_common:hda_dai_trigger: sof-audio-pci-intel-tgl 0000:00:1f.3: cmd=1 dai Analog CPU DAI direction 0
[11038.698071] snd_sof:sof_ipc4_set_pipeline_state: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc4 set pipeline instance 0 state 3
[11038.698078] snd_sof:sof_ipc4_log_header: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx      : 0x13000003|0x0: GLB_SET_PIPELINE_STATE
[11038.698434] snd_sof:sof_ipc4_log_header: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx reply: 0x33000000|0x0: GLB_SET_PIPELINE_STATE
[11038.698455] snd_sof:sof_ipc4_log_header: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx done : 0x13000003|0x0: GLB_SET_PIPELINE_STATE
[11038.698460] snd_sof:sof_ipc4_set_pipeline_state: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc4 set pipeline instance 0 state 4
[11038.698464] snd_sof:sof_ipc4_log_header: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx      : 0x13000004|0x0: GLB_SET_PIPELINE_STATE
[11038.703027] snd_sof:sof_ipc4_log_header: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx reply: 0x33000007|0x0: GLB_SET_PIPELINE_STATE
[11038.703031] sof-audio-pci-intel-tgl 0000:00:1f.3: FW reported error: 7 - Unsupported operation requested
[11038.703058] sof-audio-pci-intel-tgl 0000:00:1f.3: ipc error for msg 0x13000004|0x0
[11038.703062] sof-audio-pci-intel-tgl 0000:00:1f.3: ASoC: error at soc_dai_trigger on Analog CPU DAI: -22
[11038.703067]  HDA Analog: ASoC: trigger FE cmd: 1 failed: -22

lgirdwood

@ranj063 we you able to see any IP programming delta with debug in the hw_read(), hw_write() ?

lgirdwood · 2024-12-09T16:13:47Z

sound/soc/sof/intel/hda-dai.c

 	int stream = substream->stream;
+	int ret;
+
+	ret = hda_dai_hw_params(substream, &rtd->dpcm[stream].hw_params, dai);


prepare() should be fast. We should call this out in comments - we need to do X,Y out of cycle because Z

lgirdwood · 2024-12-16T16:12:18Z

@ujfalusi can you review whilst the HDA issue is being resolved. Thanks !

ranj063 · 2024-12-16T16:20:36Z

@ranj063 we you able to see any IP programming delta with debug in the hw_read(), hw_write() ?

@lgirdwood no there would no IP programming delta at all. The only thing that would change is the timing of the pipeline setup/free and its a clean split now with the BE pipeline handled in the BE DAI ops.

ujfalusi · 2024-12-18T13:12:20Z

@ranj063, with a quick test this appears to work fine (no regression), but needs to be rebased because of the spcm print PR and also this:

diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c
index 915a1109b2e3..c7625b73d713 100644
--- a/sound/soc/sof/intel/hda-dai.c
+++ b/sound/soc/sof/intel/hda-dai.c
@@ -745,7 +745,7 @@ int sdw_hda_dai_prepare(struct snd_pcm_substream *substream, struct snd_pcm_hw_p
 
 	return ret;
 }
-EXPORT_SYMBOL_NS(sdw_hda_dai_prepare, SND_SOC_SOF_INTEL_HDA_COMMON);
+EXPORT_SYMBOL_NS(sdw_hda_dai_prepare, "SND_SOC_SOF_INTEL_HDA_COMMON");
 
 static int hda_dai_suspend(struct hdac_bus *bus)
 {

ujfalusi · 2024-12-18T15:05:45Z

Suspend while pause still have problems, but it is the same without the PR.

lgirdwood · 2024-12-19T13:20:45Z

Suspend while pause still have problems, but it is the same without the PR.

We should not be pausing anymore when topology is being used to indicate pause support or not.

ujfalusi · 2024-12-19T13:25:32Z

Suspend while pause still have problems, but it is the same without the PR.

We should not be pausing anymore when topology is being used to indicate pause support or not.

True, but if the topology indicates that then we should handle it.

lgirdwood · 2024-12-19T13:51:14Z

Suspend while pause still have problems, but it is the same without the PR.

We should not be pausing anymore when topology is being used to indicate pause support or not.

True, but if the topology indicates that then we should handle it.

There should be no pipelines/topologies supporting pause today.

ujfalusi

@ranj063, I get the overall idea now.
What I'm a bit puzzled is that all vendors will need to implement the BE pipe management (which is mostly generic pipe/widget mangling code) on their own.

I'm not sure if we can do better? Some how have the BE code also up in the core and call in to platform if needed?

sound/soc/sof/ipc4-topology.c

sound/soc/sof/sof-audio.h

ujfalusi · 2024-12-19T13:46:47Z

sound/soc/sof/pcm.c

@@ -106,9 +106,9 @@ sof_pcm_setup_connected_widgets(struct snd_sof_dev *sdev, struct snd_soc_pcm_run

 		spcm->stream[dir].list = list;

-		ret = sof_widget_list_setup(sdev, spcm, params, platform_params, dir);
+		ret = sof_widget_list_prepare(sdev, spcm, params, platform_params, dir);


it is really confusing that in setup we prepare and later in prepare we setup

I'm not sure which has precedence... Do we need to prepare things to be able to setup or we need to setup to be able to prepare them.

@ujfalusi there's a bit of confusion wrt naming between the pcm ioctl and widgets. But what we do is widget_prepare during hw_params and widget_setup during prepare because prepare succeeds hw_params

Right, but we will sof_widget_list_prepare() in sof_pcm_setup_connected_widgets()
iow, we prepare from setup
and
sof_widget_list_setup() in sof_pcm_prepare()
iow we setup from prepare

I know it is all fine if you know the background on what these are, but for an untrained eye this is confusing.

sound/soc/sof/intel/hda-dai-ops.c

ujfalusi · 2024-12-19T15:05:13Z

sound/soc/sof/intel/hda-dai-ops.c

+	return 0;
+}
+
+static int hda_ipc4_set_up_be_pipeline(struct snd_soc_dapm_widget *w, int dir)


there is not much IPC4 specific code in these and what they call are not IPC4 specific anymore.
I'm not sure if I follow it.

@ujfalusi there's isnt anything IPC4 specific here but I dont want to modify anything for IPC3 anymore. So we just let IPC3 work as before without moving the pipeline management to the BE DAI ops.

sound/soc/sof/sof-audio.c

sound/soc/sof/ipc4-pcm.c

Add a new host_config op in struct sof_ipc_tplg_ops and define it for IPC4. This will be used to configure the host widget during prepare after a suspend/resume or after an xrun. Signed-off-by: Ranjani Sridharan <[email protected]>

In preparation for refacting pipeline management, split the widget prepare and set up between the hw_params and prepare ioctls. This is required to ensure that the BE pipeline widgets can be set up during the BE DAI prepare and the remaining widgets will be set up during the FE DAI prepare. The widget's ipc_prepare op for all widgets in both the BE and FE pipelines are handled during the FE DAI hw_params to make sure that the pipeline params can be propagated all the way from the source widget to the sink widget. Signed-off-by: Ranjani Sridharan <[email protected]>

This will be used in the BE DAI ops in the following patches. Signed-off-by: Ranjani Sridharan <[email protected]>

Define 3 new ops in struct hda_dai_widget_dma_ops for setting up/freeing/unpreparing the BE pipeline and set them for IPC4 version of the ops. These ops will be used when the pipeline management is refactored to set up/free widgets, routes and pipelines during BE DAI prepare or hw_free. Signed-off-by: Ranjani Sridharan <[email protected]>

Add and define the prepare op for Intel ACE2.x platforms. For the moment, the prepare_stream op is exactly the same as the prepare_stream op. But it will be modified in the following patch when the pipeline management is refactored. Signed-off-by: Ranjani Sridharan <[email protected]>

Invoke the set_up_be_pipeline and free_be_pipeline ops in the BE DAI prepare and hw_free to ensure that all the widgets and routes belonging to the BE pipeline are handled in the BE DAI ops. Add a new field in struct snd_sof_pipeline to identify BE pipelines i.e. those that contain the DAI widget. Modify the FE ops to make sure that the widgets/routes belonging to these pipelines are skipped during setup/free/unprepare. Signed-off-by: Ranjani Sridharan <[email protected]>

Now that all widgets/routes/pipelines are setup, triggered and freed in the BE DAI ops, we can reuse the be_pipeline flag in struct snd_sof_pipeline to skip triggering the BE pipelines in the PCM trigger. So remove the skip_during_fe_trigger flag from struct sof_ipc4_pipeline. Signed-off-by: Ranjani Sridharan <[email protected]>

ujfalusi

@ranj063, finally I think I managed to get the idea and I really like it!

However I think there could be a better and probably more intuitive way to achieve this.

I would add the hda_ipc4_set_up_be_pipeline()/hda_ipc4_free_be_pipeline()/hda_dai_unprepare_widgets_in_pipeline() and their helpers to generic SOF code (to sof-audio.c perhaps)
as snd_sof_set_up_be_pipeline()/snd_sof_free_be_pipeline()/snd_sof_unprepare_widgets_in_pipeline() and export them.

In Intel/HDA code instead of callbacks I would have a flag to not that these platforms will manage the BE pipeline themselves (IPC4)

If this flag is set then you would set the swidget->spipe->be_managed_pipeline which will cause core to skip the BE pipe and in hda code you would either use this swidget->spipe-be_managed_pipeline flag or the one which is local to intel code and call the appropriate generic helper to do the pipeline handling.

I do think that this would make it more intuitive at the end while keeping the core idea intact.

ujfalusi · 2025-01-15T14:00:36Z

sound/soc/sof/pcm.c

@@ -106,9 +106,9 @@ sof_pcm_setup_connected_widgets(struct snd_sof_dev *sdev, struct snd_soc_pcm_run

 		spcm->stream[dir].list = list;

-		ret = sof_widget_list_setup(sdev, spcm, params, platform_params, dir);
+		ret = sof_widget_list_prepare(sdev, spcm, params, platform_params, dir);


Right, but we will sof_widget_list_prepare() in sof_pcm_setup_connected_widgets()
iow, we prepare from setup
and
sof_widget_list_setup() in sof_pcm_prepare()
iow we setup from prepare

I know it is all fine if you know the background on what these are, but for an untrained eye this is confusing.

ujfalusi · 2025-01-15T15:30:51Z

drivers/soundwire/intel_ace2x.c

@@ -286,6 +286,27 @@ static int intel_params_stream(struct sdw_intel *sdw,
 	return -EIO;
 }

+static int intel_prepare_stream(struct sdw_intel *sdw,


Commit message:
For the moment, the prepare_stream op is exactly the same as the prepare_stream op.
I think one of them should be different, in this form it sound obvious that A is exactly the same as A.

also, this applies to ACE2+ platforms, I guess.

ujfalusi · 2025-01-16T06:49:51Z

sound/soc/sof/intel/hda-dai.c

+
+			if (ops->free_be_pipeline) {
+				ret = ops->free_be_pipeline(w,
+							    hdac_stream(hext_stream)->direction);


hdac_stream(hext_stream) == s, no?

ujfalusi · 2025-01-16T07:37:16Z

sound/soc/sof/intel/hda-dai.c

+
+	/* if this is a prepare without a hw_free or a suspend, free the BE pipeline */
+	if (swidget->spipe->complete && ops->free_be_pipeline) {
+		ret = ops->free_be_pipeline(w, substream->stream);


you have substream->stream saved as local variable.

ujfalusi · 2025-01-16T07:48:05Z

sound/soc/sof/intel/hda-dai.c

+	if (ops && ops->set_up_be_pipeline)
+		return ops->set_up_be_pipeline(w, substream->stream);
+
+	return ret;


you can return 0 from here to be more explicit (ret must be 0 if you reached this point)

ujfalusi · 2025-01-16T07:48:38Z

sound/soc/sof/intel/hda-dai.c


-	return non_hda_dai_hw_params(substream, &rtd->dpcm[stream].hw_params, cpu_dai);
+	return ret;


Here also, just return 0

ujfalusi · 2025-01-16T07:49:10Z

sound/soc/sof/intel/hda-dai.c

+	if (ops->set_up_be_pipeline)
+		return ops->set_up_be_pipeline(w, stream);
+
+	return ret;


ujfalusi · 2025-01-16T08:08:03Z

sound/soc/sof/intel/hda-dai-ops.c

+	int ret;
+
+	/* set the be_pipeline flag true for the pipeline */
+	swidget->spipe->be_pipeline = true;


This will be done each time a stream starts?

ujfalusi · 2025-01-16T08:09:43Z

sound/soc/sof/intel/hda-dai-ops.c

+		dev_err(sdev->dev, "failed to set up routes in the BE pipeline with DAI: %s\n",
+			w->name);
+
+	swidget->spipe->complete = 1;


even if it fails?

ujfalusi · 2025-01-16T08:15:03Z

sound/soc/sof/sof-audio.c

+{
+	struct snd_sof_widget *swidget = w->dobj.private;
+
+	if (swidget->spipe->be_pipeline)


I would rather use be_managed_pipeline. This flag is not set for example for IPC3 BE pipelines which does not make them non BE pipelines.

ranj063 force-pushed the fix/pipeline_mgt branch 2 times, most recently from 50da688 to 9883c9b Compare December 4, 2024 18:52

ranj063 force-pushed the fix/pipeline_mgt branch 6 times, most recently from f2569fa to 8818a53 Compare December 6, 2024 01:59

bardliao reviewed Dec 6, 2024

View reviewed changes

ranj063 force-pushed the fix/pipeline_mgt branch 2 times, most recently from 0a0cbf9 to dae9464 Compare December 6, 2024 17:31

ranj063 force-pushed the fix/pipeline_mgt branch 4 times, most recently from 43f86ee to 32588f6 Compare December 7, 2024 23:50

ranj063 marked this pull request as ready for review December 8, 2024 02:00

ranj063 requested review from lgirdwood, plbossart, kv2019i, dbaluta and ujfalusi as code owners December 8, 2024 02:00

bardliao reviewed Dec 9, 2024

View reviewed changes

sound/soc/sof/intel/hda-dai-ops.c Show resolved Hide resolved

lgirdwood reviewed Dec 9, 2024

View reviewed changes

ujfalusi reviewed Dec 19, 2024

View reviewed changes

ranj063 added 7 commits January 14, 2025 08:54

ASoC: SOF: sof-audio: expose sof_route_setup()

736d252

This will be used in the BE DAI ops in the following patches. Signed-off-by: Ranjani Sridharan <[email protected]>

ranj063 force-pushed the fix/pipeline_mgt branch from f48cd1e to f417fc7 Compare January 14, 2025 16:55

ranj063 requested review from ujfalusi, bardliao and lgirdwood January 14, 2025 16:55

bardliao approved these changes Jan 15, 2025

View reviewed changes

ujfalusi reviewed Jan 16, 2025

View reviewed changes


		return non_hda_dai_hw_params(substream, &rtd->dpcm[stream].hw_params, cpu_dai);
		return ret;

Refactor pipeline management #5262

Are you sure you want to change the base?

Refactor pipeline management #5262

Conversation

ranj063 commented Dec 4, 2024 • edited Loading

ranj063 commented Dec 4, 2024

bardliao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranj063 commented Dec 6, 2024

ranj063 commented Dec 7, 2024

ranj063 commented Dec 8, 2024

bardliao commented Dec 9, 2024

ujfalusi commented Dec 9, 2024

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood commented Dec 16, 2024

ranj063 commented Dec 16, 2024

ujfalusi commented Dec 18, 2024 • edited Loading

ujfalusi commented Dec 18, 2024

lgirdwood commented Dec 19, 2024

ujfalusi commented Dec 19, 2024

lgirdwood commented Dec 19, 2024

ujfalusi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ujfalusi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranj063 commented Dec 4, 2024 •

edited

Loading

ujfalusi commented Dec 18, 2024 •

edited

Loading