ipc4: handler: Split the set_pipeline_state() #8084

ujfalusi · 2023-08-23T13:06:28Z

Split the pipeline state setting and the pipeline triggering into separate loops to avoid cases when a component at the junction would try to access information on the not yet triggered/configured pipeline side.

Needs cleanup!

ranj063 · 2023-08-23T16:04:33Z

src/ipc/ipc4/handler.c

-int set_pipeline_state(struct ipc_comp_dev *ppl_icd, uint32_t cmd,
-		       bool *delayed)
+
+static int do_set_pipeline_state(struct ipc_comp_dev *ppl_icd, uint32_t cmd,


@ujfalusi what issue are we trying to fix here?

Also, the name do_set_pipeline_state() is so confusing, you dont just set state but also do the the trigger here isnt it?

@ranj063 ， this PR are trying to fix Seppo's SRC CI error（#7547）, params are not correctly propagated to SRC source buffer, I tried other ways, but not working。

@ranj063, what I have seen with @singalsu's SRC support is
The capture looks like this SSP2 -> SRC -> host which is in topology represented with the following two pipelines:
pipeline0: ssp.copier
pipeline1: src -> host.copier

We have 2 pipelines and we batch start them, the call path is something like this:
ipc4_set_pipeline_state()
set_pipeline_state()
ipc4_pcm_params()
ipc4_pipeline_params()
ipc4_comp_params()
pipeline_prepare()

the ipc4_set_pipeline_state() calls the set_pipeline_state() in a for loop for each pipelines, starting from the sink side.
The ipc4_pipeline_params() is walking through the pipeline and sets the params, it does it by using struct sof_ipc_pcm_params hw_params = {{ 0 }}; as initial params and this actually get set to the first buffer.
0x40001 : host.copier
0x80000 : SRC
0x40000 : ssp.copier
Just the relevant prints to no loose them. Modified audio_stream_set_params() to print the source and sink components

This is what happens:

[ 3.334896] <wrn> ipc: ipc4_pcm_params: comp(1):1 0x40001 ipc4_pcm_params(): ENTER [ 3.334900] <wrn> ipc: ipc4_pipeline_params: comp(1):1 0x40001 ipc4_pipeline_params(): ENTER [ 3.334906] <wrn> ipc: audio_stream_set_params: comp(0):1 0x80000 audio_stream_set_params(): the SOURCE [ 3.334910] <wrn> ipc: audio_stream_set_params: comp(1):1 0x40001 audio_stream_set_params(): the SINK [Heippa] audio_stream_set_params(): channels: 0 -> 0 [ 3.334941] <wrn> ipc: audio_stream_set_params: comp(2):0 0x40000 audio_stream_set_params(): the SOURCE [ 3.334945] <wrn> ipc: audio_stream_set_params: comp(0):1 0x80000 audio_stream_set_params(): the SINK [Heippa] audio_stream_set_params(): channels: 0 -> 2 [ 3.334965] <wrn> ipc: ipc4_pipeline_params: comp(1):1 0x40001 ipc4_pipeline_params(): LEAVE [ 3.334968] <wrn> ipc: ipc4_pcm_params: comp(1):1 0x40001 ipc4_pcm_params(): After params

This was the ipc4_pipeline_params() run, notice that the buffer between host.copier and SRC is is receiving params = { 0 }

[ 3.334985] <wrn> ipc: audio_stream_set_channels: comp(0):1 0x80000 audio_stream_set_channels(): the SOURCE [ 3.334990] <wrn> ipc: audio_stream_set_channels: comp(1):1 0x40001 audio_stream_set_channels(): the SINK [Heippa] audio_stream_set_channels(): channels: 0 -> 2 [Heippa] audio_stream_set_params(): channels: 0 -> 2 [ 3.335031] <inf> copier: copier_prepare: comp(1):1 0x40001 copier_prepare() [ 3.335041] <inf> src: src_prepare: comp(0):1 0x80000 src_prepare() [ 3.335046] <inf> src: src_params_general: comp(0):1 0x80000 src_params() [ 3.335051] <wrn> ipc: audio_stream_set_params: comp(0):1 0x80000 audio_stream_set_params(): the SOURCE [ 3.335055] <wrn> ipc: audio_stream_set_params: comp(1):1 0x40001 audio_stream_set_params(): the SINK [Heippa] audio_stream_set_params(): channels: 2 -> 2 [ 3.335068] <wrn> ipc: audio_stream_set_params: comp(2):0 0x40000 audio_stream_set_params(): the SOURCE [ 3.335073] <wrn> ipc: audio_stream_set_params: comp(0):1 0x80000 audio_stream_set_params(): the SINK [Heippa] audio_stream_set_params(): channels: 2 -> 2 [ 3.335085] <inf> src: src_params_general: comp(0):1 0x80000 src_params(), source_rate = 48000, sink_rate = 8000 [ 3.335113] <wrn> ipc: ipc4_pcm_params: comp(1):1 0x40001 ipc4_pcm_params(): LEAVE

In the prepare part of the function (pipeline_prepare()) the host.copier config got corrected.
At this point the SRC is activated and would start processing, the pipeline0 however is yet to be configured:

[ 3.335155] <wrn> ipc: ipc4_pcm_params: comp(2):0 0x40000 ipc4_pcm_params(): ENTER [ 3.335158] <wrn> ipc: ipc4_pipeline_params: comp(2):0 0x40000 ipc4_pipeline_params(): ENTER [ 3.335165] <wrn> ipc: audio_stream_set_params: comp(2):0 0x40000 audio_stream_set_params(): the SOURCE [ 3.335168] <wrn> ipc: audio_stream_set_params: comp(0):1 0x80000 audio_stream_set_params(): the SINK [Heippa] audio_stream_set_params(): channels: 2 -> 0 [ 3.335188] <wrn> ipc: ipc4_pipeline_params: comp(2):0 0x40000 ipc4_pipeline_params(): LEAVE ... [ 3.335265] <wrn> src: src_get_copy_limits: comp(2):0 0x40000 src_get_copy_limits(): the SOURCE [ 3.335270] <wrn> src: src_get_copy_limits: comp(0):1 0x80000 src_get_copy_limits(): the SINK [crash check] src_get_copy_limits(): channels: 0 ^^^ divide by zero crash ^^^ ... [ 3.335300] <wrn> ipc: ipc4_pcm_params: comp(2):0 0x40000 ipc4_pcm_params(): After params [ 3.335316] <wrn> ipc: audio_stream_set_channels: comp(2):0 0x40000 audio_stream_set_channels(): the SOURCE [ 3.335321] <wrn> ipc: audio_stream_set_channels: comp(0):1 0x80000 audio_stream_set_channels(): the SINK [Heippa] audio_stream_set_channels(): channels: 0 -> 2 [Heippa] audio_stream_set_params(): channels: 0 -> 2 ... [ 3.335430] <inf> copier: copier_prepare: comp(2):0 0x40000 copier_prepare() [ 3.335460] <wrn> ipc: ipc4_pcm_params: comp(2):0 0x40000 ipc4_pcm_params(): LEAVE

At the first hunk the ipc4_pipeline_params() resets the buffer paams to 0 between dai.copier and SRC as this is the first (and only one) in the pipeline. It does it by clearing the already set config - as down in this path we have comp_verify_params() which always forces the params to be set and to me it only does that, not verifying anything.

Then the src's process is called (it is already active) and it will encounter a buffer on it's input with all params to be 0, causing a panic.

If I handle this then the firmware run further and the the prepare path will configure the params finally and consequent src prepare will not have issue.

I have suggested to @singalsu to silently ignore a buffer params setting in case all parameters are to be set to 0 as that is by itself is not valid.
Probably that is the correct fix, but we do have a race in this case and it is matter of timing that the component at the start of the pipeline will cause a crash or not.

@ujfalusi good stuff, when doing cleanup can you add more inline comments which can include warnings to help other understand what can/can't be done in each phase and why.

@ujfalusi thanks for teh detailed explanation. But isnt a simpler fix just to modify the params before this call to copier_update_params()

sof/src/audio/copier/copier.c

Line 621 in b81f130

copier_update_params(cd, dev, params);

to read the params from the basecfg so that we dont overwrite the buffer with 0 params?

Isn't it one of the cases that should be fixed by all pipeline actions carried out in the pipeline task context? That was the mechanism that protected us from half-configured pipelines being triggered?

replying to myself: not quite, with IPC3 we also processed pipeline parameters in the IPC task context, the difference was only, that there setting pipeline parameters were a separate IPC message.

@lyakh, the pipeline is configured (pipeline 1 in the example) and it is triggered, but the configuration is not yet finished on pipeline 0 and pipeline 0 is not yet triggered (because it should be configured first).

In a simplified view, we have two pipelines connected: pipeline0, pipeline1 and we do atm:
configure pipeline1
trigger pipeline1
configure pipeline0
trigger pipeline0

with this PR:
configure pipeline1
configure pipeline0
trigger pipeline1
trigger pipeline0

There is one thing which might need to be considered however, I think: if we have a really long pipeline then the trigger round will be 'delayed' to the point when all pipelines have been configured already while currently the first pipeline is triggered right after the first configuration.
The last trigger will happen at the same time, but the time distribution will differ.

If this is a problem than probably have some sort of one step later trigger

for (i = 0; i < num_pipelines; i++) { configure_pipeline(i); if (i) trigger_pipeline(i - 1); } trigger_pipeline(i - 1); /* trigger_pipeline(num_pipelines - 1); */

so we run the trigger with a delay after the configure -> connected pipes are configured

If this is a problem than probably have some sort of one step later trigger

I really hope we won't need this!

ujfalusi · 2023-08-23T16:15:00Z

looks like creating internal CI failures and they are around triggers..

singalsu · 2023-08-24T07:49:46Z

Thanks Peter! With this patch the topology from #7547 works correctly in a SRC playback & capture stress test. Without, the divide by zero exception usually happened in first or second iteration.

ujfalusi · 2023-08-24T12:11:54Z

Changes since v1:

code is cleaned up and updated
Helper pipeline_get_host_dev() added to simplify the code and reduce duplication
debug prints cleaned up
the functionality is now clearly split (trigger only in do_pipeline_trigger()
do_pipeline_state_change() does not need the delay parameter anymore

kv2019i

Thanks @ujfalusi , looks good!

kv2019i · 2023-08-25T15:15:22Z

Hmm, a lot of fails in https://sof-ci.01.org/sofpr/PR8084/build12026/devicetest/index.html

lgirdwood · 2023-08-25T16:04:48Z

SOFCI TEST

lgirdwood · 2023-08-25T16:05:27Z

Hmm, a lot of fails in https://sof-ci.01.org/sofpr/PR8084/build12026/devicetest/index.html

Rerunning to make sure not a fluke, seems to be concentrated around SDW, no impact on CAVS though.

ujfalusi · 2023-08-28T05:34:59Z

It looks better now, there must have been some regression happened and got fixed right away. All FAIL were happening right after SET_DX when powering off the DSP.

lyakh · 2023-08-28T08:14:03Z

src/ipc/ipc4/handler.c

+
+		/* Do state change only on current core.
+		 * Otherwise the IPC will be passed to target core in trigger
+		 * phase


isn't that too late? What if the first pipeline runs on core 0, so it is configured here. The next pipeline runs on core 1, so it isn't configured yet. Then we continue below to trigger pipeline 0, while pipeline 1 is still unconfigured?

right, we don't have a means to separately configure and trigger a pipeline on remote core, afaik.
If we would do the IDC_MSG_PPL_STATE here in the config phase, it will trigger non core 0 pipelines right away.
If we add another loop in between config and trigger loop then it is almost good except if we have two non core 0 pipelines directly connected
If we do it after the trigger loop, then we have similar issue.

I'm not sure how that could be solved other than modifying the IDC_MSG_PPL_STATE message and add support for decoupling of config and trigger phases.

lgirdwood · 2023-08-28T09:57:25Z

@marcinszkudlinski @abonislawski any comments ?

Separate the set_pipeline_state() function into two distinct phase: ipc4_pipeline_prepare() and ipc4_pipeline_trigger() The two phase will be used later directly to avoid race conditions when a pipeline is already set to run while the connected pipeline is not yet prepared. With the split also clean up the debug prints to be unified and clear. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

In preparation to share the extension with an action, first introduce a mask so that the idc code is prepared for a shared use of the extension. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

Add definitions for pipeline state change phases to be executed when a PPL_STATE message is handled by a remote core. The phase can be a combination of: PREPARE and TRIGGER. PREPARE will run a configuration (state change preparations) phase on the pipeline TRIGGER will run a a trigger on the pipeline Set ONESHOT in ipc4_set_pipeline_state() to have both phases to be run in order to avoid regression. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

Make the ipc4_pipeline_prepare and ipc4_pipeline_trigger available outside of handler.c to be used to implement staged pipeline state phases. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

Execute the requested phases of the pipeline state change specified by the phase parameter. If no phase has been specified then run both phases as oneshot. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

Currently the pipeline state change flow handles the phases at the same time for each pipeline (for two pipelines): pipeline1 prepare Pipeline1 trigger pipeline0 prepare Pipeline0 trigger This can open a race condition when pipeline1 is already triggered and is in active state while pipeline0 is not yet prepared, configured. The process function of a component from pipeline1 might query information from pipeline0 at this point, which can lead to errors. With the separate loops the flow will be changed to: pipeline1 prepare pipeline0 prepare Pipeline1 trigger Pipeline0 trigger It will make sure that all pipelines and components have been prepared before the triggering is going to happen. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com> # Conflicts: # src/include/sof/ipc/topology.h # src/ipc/ipc4/handler.c

ujfalusi · 2023-08-28T12:54:12Z

Changes since v2:

new patches have been added to allow the split pipeline state change via IDC messaging on different core
Renaming of functions to ipc4_pipeline_prepare()/_trigger()
The IDC_MSG_PPL_STATE message's extension have been re-defined to carry the pipeline id and a phase of a state change

lyakh · 2023-08-29T07:47:10Z

src/ipc/ipc4/handler.c

-	       cmd, status);
-
-	/* source & sink components are set when pipeline is set to COMP_STATE_INIT */
-	if (status != COMP_STATE_INIT) {


If I understand correctly removing this block of code is a change of behaviour. At the very least this should be mentioned in the commit message. Ideally we could split this commit into two - one just splitting the function with no functional changes, and one with this change. I wouldn't insist on that, but at least let's add a comment to make sure everybody's aware of this change.

lyakh · 2023-08-29T07:59:36Z

src/ipc/ipc4/handler.c

+			return 0;
+		case COMP_STATE_READY:
+		case COMP_STATE_PREPARE:
+			cmd = COMP_TRIGGER_PRE_START;


it seems to me this does way too much. Can we just pass cmd by reference to prepare() and keep the current top part of set_pipeline_state() 1-to-1 with only replacing cmd with *cmd and then use that cmd for the trigger part. I think that way the diff should become much smaller and a lot clearer and the chance of an accidental change will be much lower.

I'd still rather do it somewhat differently, resulting in a smaller diff and final code, but it shouldn't be critical

ujfalusi requested review from ranj063 and singalsu August 23, 2023 13:06

ranj063 reviewed Aug 23, 2023

View reviewed changes

singalsu mentioned this pull request Aug 24, 2023

Audio: SRC: Avoid divide by zero when source frame bytes is not set #8064

Closed

ujfalusi force-pushed the peter/pr/ipc_split_set_pipeline_state_01 branch from 1f73fa0 to 80ef9f6 Compare August 24, 2023 12:09

ujfalusi marked this pull request as ready for review August 24, 2023 12:09

ujfalusi requested review from dbaluta, lbetlej, mmaka1 and plbossart as code owners August 24, 2023 12:09

lgirdwood approved these changes Aug 24, 2023

View reviewed changes

kv2019i approved these changes Aug 25, 2023

View reviewed changes

ranj063 approved these changes Aug 25, 2023

View reviewed changes

lyakh reviewed Aug 28, 2023

View reviewed changes

ujfalusi added 6 commits August 28, 2023 15:44

idc: Add pipeline ID mask definition to IDC_MSG_PPL_STATE extension

5475e9f

In preparation to share the extension with an action, first introduce a mask so that the idc code is prepared for a shared use of the extension. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ipc4: handler: Export ipc4_pipeline_prepare and ipc4_pipeline_trigger

b20d503

Make the ipc4_pipeline_prepare and ipc4_pipeline_trigger available outside of handler.c to be used to implement staged pipeline state phases. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ipc4: idc: Handle the phase parameter in idc_ppl_state()

f79b371

Execute the requested phases of the pipeline state change specified by the phase parameter. If no phase has been specified then run both phases as oneshot. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ujfalusi force-pushed the peter/pr/ipc_split_set_pipeline_state_01 branch from 80ef9f6 to 9e26d98 Compare August 28, 2023 12:51

ujfalusi requested a review from iuliana-prodan as a code owner August 28, 2023 12:51

ujfalusi requested a review from abonislawski as a code owner August 28, 2023 12:51

lyakh previously requested changes Aug 29, 2023

View reviewed changes

kv2019i merged commit 068f143 into thesofproject:main Aug 29, 2023

kv2019i mentioned this pull request Aug 29, 2023

ASoC: SOF: sof-audio: Modify the order of widget set up thesofproject/linux#4550

Closed

abonislawski mentioned this pull request Nov 17, 2023

[BUG] DMIC capture failed to start #8481

Closed

ipc4: handler: Split the set_pipeline_state() #8084

ipc4: handler: Split the set_pipeline_state() #8084

Uh oh!

Conversation

ujfalusi commented Aug 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ujfalusi commented Aug 23, 2023

Uh oh!

singalsu commented Aug 24, 2023

Uh oh!

ujfalusi commented Aug 24, 2023

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i commented Aug 25, 2023

Uh oh!

lgirdwood commented Aug 25, 2023

Uh oh!

lgirdwood commented Aug 25, 2023

Uh oh!

ujfalusi commented Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Aug 28, 2023

Uh oh!

ujfalusi commented Aug 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ujfalusi commented Aug 28, 2023 •

edited

Loading