ipc: don't propagate commands across pipelines for IPC4 #8285

iuliana-prodan · 2023-10-04T16:22:37Z

When IPC4_MOD_ID was introduced it returned a non-zero module ID under IPC4 and 0 under IPC3.
After commit "45ca3d430 (include: ipc4: module: fix component ID macros)", the IPC4_MOD_ID, under IPC3, is not 0 anymore.
Therefore, in order to not propagate the commands across pipelines for IPC4, add them under CONFIG_IPC_MAJOR_4.

This fixes playback with mixer.
Without this patch, with IPC3, we get:
src/audio/component.c:130 ERROR comp_set_state(): wrong state = 1, COMP_TRIGGER_PRE_START ../pipeline-stream.c:436 ERROR pipeline_trigger_run(): ret = -22, host->comp.id = 12, cmd = 7 src/ipc/ipc3/handler.c:540 ERROR ipc: comp 12 trigger 0x40000 failed -22

That's because, at some point, the trigger command is not propagated across pipeline and the component state remains unmodified.

kv2019i

Thanks for rootcausing! It would be really good to have some (non-Intel) IPC3 system in upstream CI to catch these.

One alternative fix proposal inline, see comment.

kv2019i · 2023-10-04T17:19:39Z

src/audio/pipeline/pipeline-graph.c

@@ -331,13 +331,15 @@ static int pipeline_comp_reset(struct comp_dev *current,
 	pipe_dbg(p_current, "pipeline_comp_reset(), current->comp.id = %u, dir = %u",
 		 dev_comp_id(current), dir);

+#if CONFIG_IPC_MAJOR_4


Ouch, this is pretty bad. :(

How about if we'd conditionally define IPC4_MOD_ID in src/include/ipc4/module.h so that it would return 0 when building for IPC3. Then we wouldn't need the #ifdefs in pipeline code...?

I agree, is not pretty.
I saw we have a lot #if CONFIG_IPC_MAJOR_4 and #if CONFIG_IPC_MAJOR_3 in code and I thought we'll stick with this until we'll all use IPC4.
But your proposal is better, I'll update the PR.
Thanks.

@iuliana-prodan Actually with "pretty bad" I was more thinking about the regression :) But yes, even better if we can minimize the ifdefs in code. We do have some of them in code, but we should strive to keep the count down.

The commit message doesn't seem to be quite correct - commit 45ca3d4 is only touching IPC4 files, so it cannot change IPC3 behaviour as the commit message seems to suggest. Could you clarify?

Ouch, this is pretty bad. :(

How about if we'd conditionally define IPC4_MOD_ID in src/include/ipc4/module.h so that it would return 0 when building for IPC3. Then we wouldn't need the #ifdefs in pipeline code...?

@kv2019i wait, are you saying, that that file is included in IPC3 builds??

btw, we also can use IS_ENABLED(CONFIG_IPC_MAJOR_4) inside ifs instead of using IPC4_MOD_ID() as an IPC version check

I've added a new version using IS_ENABLED(CONFIG_IPC_MAJOR_4) when defining IPC4_MOD_ID()

@lyakh yes, pipeline-stream.c includes ipc4/module.h unconditionally. otherwise this code would not work at all.

But you raise a good point that "if (IPC4_MOD_ID(current->ipc_config.id))" check is only used to check for IPC4 builds, so we could clean this up further by doing:

-» if (!is_single_ppl && (!is_same_sched || IPC4_MOD_ID(current->ipc_config.id))) { +» if (!is_single_ppl && (!is_same_sched || IS_ENABLED(CONFIG_IPC_MAJOR_4))) { i.e. in IPC4 builds, IPC4_MOD_ID() check is always true. I thought we had some case this would not be the case, but alas it seems really not. Not sure why @lyakh and @RanderWang you did not use this originally. Relying on IPC4_MOD_ID() to be zero in IPC3 builds is really not very readable. So I think this is one case where ifdef is better. So @iuliana-prodan if you can do one more update?

@kv2019i This

-» if (!is_single_ppl && (!is_same_sched || IPC4_MOD_ID(current->ipc_config.id))) { +» if (!is_single_ppl && (!is_same_sched || IS_ENABLED(CONFIG_IPC_MAJOR_4))) {

is redundant with the way I define IPC4_MOD_ID now:
#define IPC4_MOD_ID(x) (IS_ENABLED(CONFIG_IPC_MAJOR_4) ? ((x) & 0xffff) : 0)

When IPC4_MOD_ID was introduced it returned a non-zero module ID under IPC4 and 0 under IPC3. After commit "45ca3d430 (include: ipc4: module: fix component ID macros)", the IPC4_MOD_ID, under IPC3, is not 0 anymore. Therefore, in order to not propagate the commands across pipelines for IPC4, define IPC4_MOD_ID always to 0 for IPC3. This fixes playback with mixer. Without this patch, with IPC3, we get: src/audio/component.c:130 ERROR comp_set_state(): wrong state = 1, COMP_TRIGGER_PRE_START ../pipeline-stream.c:436 ERROR pipeline_trigger_run(): ret = -22, host->comp.id = 12, cmd = 7 src/ipc/ipc3/handler.c:540 ERROR ipc: comp 12 trigger 0x40000 failed -22 That's because, at some point, the trigger command is not propagated across pipeline and the component state remains unmodified Signed-off-by: Iuliana Prodan <[email protected]>

lgirdwood · 2023-10-05T14:57:31Z

@wszypelt @lrudyX not expecting a change for IPC4 tests here, false positive ?

lyakh

in fact it's weird that IPC3 code includes IPC4 headers, I don't think this should happen at all. So I'd expect when IPC3 is used, the ipc3/module.h header would be included and that one would just unconditionally #define IPC4_MOD_IS(x) 0

If we want to go deeper ipc3/module.h should even know about IPC4. Perhaps some generic wrappers that are implemented in both ipc3/ipc4.

but I think we are fine for now.

kv2019i · 2023-10-06T10:03:15Z

Ack @dbaluta @iuliana-prodan @lyakh , let's go ahead to unblock IPC3 builds.

But my point remains (and this is really Intel folks to address):

if (!is_single_ppl && (!is_same_sched || IPC4_MOD_ID(current->ipc_config.id))) {

This is convoluted way to express different behaviour for IPC4. To understand the logic, you have to know IPC4_MOD_ID() is non-zero always for IPC4. I got mislead by this as well. With " IS_ENABLED(CONFIG_IPC_MAJOR_4)", it's immediately clear to code reader that the behaviour is different for this IPC version. Am I @lyakh missing something?

Oh well, as @lyakh pointed out, maybe we clean it up differenly and address the module.h inclusion.

iuliana-prodan · 2023-10-06T11:57:53Z

Ack @dbaluta @iuliana-prodan @lyakh , let's go ahead to unblock IPC3 builds.

But my point remains (and this is really Intel folks to address):
if (!is_single_ppl && (!is_same_sched || IPC4_MOD_ID(current->ipc_config.id))) {
This is convoluted way to express different behaviour for IPC4. To understand the logic, you have to know IPC4_MOD_ID() is non-zero always for IPC4. I got mislead by this as well. With " IS_ENABLED(CONFIG_IPC_MAJOR_4)", it's immediately clear to code reader that the behaviour is different for this IPC version. Am I @lyakh missing something?

Oh well, as @lyakh pointed out, maybe we clean it up differenly and address the module.h inclusion.

@kv2019i @lyakh there is no module.h for IPC3. At least, not now.
I was thinking of something like:

diff --git a/src/include/ipc/header.h b/src/include/ipc/header.h
index 04b97bb80..94795e52a 100644
--- a/src/include/ipc/header.h
+++ b/src/include/ipc/header.h
@@ -24,8 +24,10 @@
  */
 #if CONFIG_IPC_MAJOR_3
 #include <ipc3/header.h>
+#define IPC_MOD_ID(x)	IPC3_MOD_ID(x)
 #elif CONFIG_IPC_MAJOR_4
 #include <ipc4/header.h>
+#define IPC_MOD_ID(x)	IPC4_MOD_ID(x)
 #endif
 
 /**
diff --git a/src/include/ipc3/header.h b/src/include/ipc3/header.h
index 0c3b5bba7..3b4ff2b29 100644
--- a/src/include/ipc3/header.h
+++ b/src/include/ipc3/header.h
@@ -28,6 +28,8 @@
 
 #define ipc_from_hdr(x) ((struct sof_ipc_cmd_hdr *)x)
 
+#define IPC3_MOD_ID(x)	0
+
 /**
  * \brief Generic message header. IPC MAJOR 3 version.
  * All IPC3 messages use this header as abstraction

And use IPC_MOD_ID(x) everywhere in pipelines.

Would that work for Intel?

iuliana-prodan requested review from lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners October 4, 2023 16:22

iuliana-prodan requested review from lyakh and btian1 October 4, 2023 16:23

kv2019i requested a review from aiChaoSONG October 4, 2023 17:18

kv2019i reviewed Oct 4, 2023

View reviewed changes

iuliana-prodan force-pushed the fix-mixer-playback branch from 2d49cec to a0f8a19 Compare October 5, 2023 10:59

iuliana-prodan requested review from RanderWang, marcinszkudlinski and pblaszko as code owners October 5, 2023 10:59

lgirdwood approved these changes Oct 5, 2023

View reviewed changes

dbaluta approved these changes Oct 5, 2023

View reviewed changes

lyakh approved these changes Oct 6, 2023

View reviewed changes

dnikodem approved these changes Oct 6, 2023

View reviewed changes

dbaluta merged commit a44ddbe into thesofproject:main Oct 6, 2023
37 of 38 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ipc: don't propagate commands across pipelines for IPC4 #8285

ipc: don't propagate commands across pipelines for IPC4 #8285

iuliana-prodan commented Oct 4, 2023

kv2019i left a comment

kv2019i Oct 4, 2023

iuliana-prodan Oct 4, 2023

kv2019i Oct 5, 2023

lyakh Oct 5, 2023

lyakh Oct 5, 2023

lyakh Oct 5, 2023

iuliana-prodan Oct 5, 2023 •

edited

Loading

kv2019i Oct 5, 2023

iuliana-prodan Oct 6, 2023

lgirdwood commented Oct 5, 2023

lyakh left a comment •

edited by dbaluta

Loading

kv2019i commented Oct 6, 2023

iuliana-prodan commented Oct 6, 2023

ipc: don't propagate commands across pipelines for IPC4 #8285

ipc: don't propagate commands across pipelines for IPC4 #8285

Conversation

iuliana-prodan commented Oct 4, 2023

kv2019i 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

iuliana-prodan Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood commented Oct 5, 2023

lyakh left a comment • edited by dbaluta Loading

Choose a reason for hiding this comment

kv2019i commented Oct 6, 2023

iuliana-prodan commented Oct 6, 2023

iuliana-prodan Oct 5, 2023 •

edited

Loading

lyakh left a comment •

edited by dbaluta

Loading