src: audio: module_adapter: Do the params config right after init #10094

ranj063 · 2025-07-03T20:15:43Z

The module_adapter_params() functions set the stream params based on the base config for each module which is available right after module init. So configure the params for IPC4 during module_new() and remove the call to ipc4_pipeline_params(). This should help with reducing the time to trigger pipelines during start.

ranj063 · 2025-07-03T23:40:13Z

SOFCI TEST

kv2019i

A nice improvement, although I'd elaborate what is happening and why more in the code comment. After all, the documentation for "params()" op is:

»        * Sets component audio stream parameters.                                                                                                                                           
»        * @param dev Component device.                                                                                                                                                      
»        * @param params Audio (PCM) stream parameters to be set.

What module_adapter does here when built for IPC4, is quite a different thing.

kv2019i · 2025-07-04T12:49:23Z

src/audio/module_adapter/module_adapter.c

+#if CONFIG_IPC_MAJOR_4
+	struct sof_ipc_stream_params params = {{ 0 }};
+
+	/* set the stream params based on the module base cfg */


This is quite whacky looking code to pass a NULL params struct. I would add even more explanation to explain that in IPC3, params is used, but in IPC4 is not used, but instead it will be filled from basecfg.

@kv2019i @ranj063 well, not sure if it's important or intentional, but it isn't the same as what is done now. Now these parameters are initialised to 0 only once for the entire pipeline. So when traversing the pipeline in ipc4_comp_params() if the first component sets some values in it, the next component will get those values. Let me "request changes" for this PR for now to make sure we think this through

@lyakh I thought about this as well but even if it is initialized only once, we should be overwriting the params based on the current module's base_cfg. So in theory this should be do the same thing as before. But of course some of the alsabat tests as failing so I am not entirely sure if we're missing something

but looking more closely at module_adapter_params(), what I may be missing is the fact that the buffer parameters are updated in comp_verify_params() and this will not happen with my PR because the components may not be connected at this point. I'll look into it

@lyakh have a look at the updated PR now. I think Ive fixed the issue with missing comp_verify_params(). I've got almost all platforms passing except for LNL HDA(which doesnt seem like an issue with my PR)

of course the QB tests as failing. @lgirdwood who can help me with that? I cant even access the link

lyakh · 2025-07-07T08:23:37Z

src/audio/module_adapter/module_adapter.c

+#if CONFIG_IPC_MAJOR_4
+	struct sof_ipc_stream_params params = {{ 0 }};
+
+	/* set the stream params based on the module base cfg */


@kv2019i @ranj063 well, not sure if it's important or intentional, but it isn't the same as what is done now. Now these parameters are initialised to 0 only once for the entire pipeline. So when traversing the pipeline in ipc4_comp_params() if the first component sets some values in it, the next component will get those values. Let me "request changes" for this PR for now to make sure we think this through

ranj063 · 2025-07-07T22:16:41Z

SOFCI TEST

src/audio/module_adapter/module_adapter.c

question clarified

lyakh · 2025-07-14T06:17:47Z

src/audio/kpb.c


+	kpb_set_params(dev, &params);
+
+	ret = kbp_verify_params(dev, &params);


the naming of both of these functions isn't very... intuitive. AFAICS kpb_set_params() actually doesn't set parameters but rather gets them instead. I.e. it retrieves current KPB parameters into a user-provided structure. Then it isn't very clear why kbp_verify_params() is called - if you've just obtained parameters from a device, they should be valid, right? But also instead of just verifying parameters, which sounds like a read-only operation, kbp_verify_params() and the underlying comp_verify_params() can update parameters and also use them to configure audio buffers... Maybe we need at least some comments here and eventually also in the implementation.

lyakh · 2025-07-14T06:20:13Z

src/audio/module_adapter/module_adapter.c

+	struct sof_ipc_stream_params params;
+
+	/*
+	 * set the stream params based on the module base cfg. There's no need to initialize


I think usually when we say, that a function "sets parameters," we mean, that we pass parameters to a function which are then used internally by that function for some configuration. While here as well as in kpb_set_params() above we retrieve parameters.

lyakh · 2025-07-14T06:23:14Z

src/audio/kpb.c

+
+	ret = kbp_verify_params(dev, &params);
+	if (ret < 0) {
+		comp_err(dev, "kpb_prepare: pcm params verification failed");


I'd propose not to use function names at least in new logging prints - even if they're otherwise used throughout the code. But in fact see comp_cl_err() call 6 lines down - it has no function name in it. Zephyr adds function names by itself. I'd also not use function names in added prints below - in module_adapter.c etc., even if so far they're used there.

lyakh · 2025-07-14T06:25:04Z

src/audio/module_adapter/module_adapter.c


 	comp_dbg(dev, "module_adapter_prepare() start");
+#if CONFIG_IPC_MAJOR_4
+	/* verify params */


I think comments should add information, not obvious from the code, and a comment like this above a call to comp_verify_params() doesn't seem to add a lot of information. Instead maybe clarify that that function not only verifies parameters, but does more as discussed above

src/audio/module_adapter/module_adapter.c

src/samples/audio/detect_test.c

lyakh · 2025-07-14T06:29:31Z

@ranj063 also, if you update this PR - the subject "src: audio:..." confused me at first - I thought it was related to the SRC component :-) I think in SOF we don't quote the full path to the affected sources in the patch subject line, at least I don't think we usually put the "src" part there. Beginning with "audio:..." should be enough?

ranj063 · 2025-07-15T15:25:09Z

SOFCI TEST

ranj063 · 2025-07-16T00:53:00Z

@lyakh Ive addressed your comments now. Let me know how it looks. I do not like the naming of the function comp_veryify_params() or kpb_verify_params. I am going to do a follow up PR to remove this function anyway to do the params update during binding.

ranj063 · 2025-07-16T01:41:27Z

SOFCI TEST

lyakh · 2025-07-16T06:27:02Z

ouch - IPC timeout on PTL https://sof-ci.01.org/sofpr/PR10094/build13984/devicetest/index.html?model=PTLH_RVP_NOCODEC&testcase=multiple-pause-resume-50 - that isn't good. Now it isn't obvious that it's this PR that does that, we have observed "somewhat similar" problems recently #10116 but no IPC timeouts on PTL until now... So it might or might not be this PR. Let's put a DNM here for a bit.

lgirdwood · 2025-07-16T09:19:38Z

ouch - IPC timeout on PTL https://sof-ci.01.org/sofpr/PR10094/build13984/devicetest/index.html?model=PTLH_RVP_NOCODEC&testcase=multiple-pause-resume-50 - that isn't good. Now it isn't obvious that it's this PR that does that, we have observed "somewhat similar" problems recently #10116 but no IPC timeouts on PTL until now... So it might or might not be this PR. Let's put a DNM here for a bit.

I think this is probably unrelated since this change will impact the initial stream start state transitions, but lets run another round of tests.

lgirdwood · 2025-07-16T09:19:45Z

SOFCI TEST

ranj063 · 2025-07-16T16:42:57Z

I think this is probably unrelated since this change will impact the initial stream start state transitions, but lets run another round of tests.

Yeah, I doubt this PR can cause any impact on pause/release. Testing again now

ranj063 · 2025-07-16T19:12:23Z

SOFCI TEST

The module_adapter_params() functions set the stream params based on the base config for each module which is available right after module init. So configure the params for IPC4 during module_new() and remove the call to ipc4_pipeline_params(). In order to update the buffer params, add a call to comp_verify_params() during module_adapter_prepare in order to update the intermediate buffers. This should help reduce the time to trigger pipelines during start. Add the params handling for the KPB and detect_test modules explicitly because they don't use the module adapter. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 · 2025-07-17T04:35:06Z

@wszypelt looks like the MTL drive was skipped in the QB tests. Would it be possible to trigger it again?

lyakh · 2025-07-17T06:18:24Z

@lgirdwood @ranj063 looks like since yesterday this PR ran through the CI up to 4 times: twice triggered by a git push and twice by the "magic keyword" comment. There are no multiple-pause-resume failures in the last round, but the one before the last again had IPC timeout on PTL in the same test.

We currently have failures in that test on multiple platforms #10116 but none of those had IPC timeouts. I also can easily reproduce those failures without IPC timeouts on a test MTL system. I'll try to dig some more into this today and possibly tomorrow, let's decide then.

ranj063 · 2025-07-17T14:38:38Z

@lgirdwood @ranj063 looks like since yesterday this PR ran through the CI up to 4 times: twice triggered by a git push and twice by the "magic keyword" comment. There are no multiple-pause-resume failures in the last round, but the one before the last again had IPC timeout on PTL in the same test.

We currently have failures in that test on multiple platforms #10116 but none of those had IPC timeouts. I also can easily reproduce those failures without IPC timeouts on a test MTL system. I'll try to dig some more into this today and possibly tomorrow, let's decide then.
@lyakh I tried to trigger the test multiple times to see if the IPC timeout can be reproduced. But looks like the dmesg logs were missing in both the times they occured so it's hard to tell the sequence that caused it. But I have a suspicion on the xruns causing this. Do you know what is the sequence when an xrun occurs? Do we stop the pipeline and reset the pipeline within the FW or do we wait for the host to stop, reset and restart with a prepare?

ranj063 · 2025-07-17T14:40:00Z

SOFCI TEST

lyakh · 2025-07-18T06:21:30Z

We currently have failures in that test on multiple platforms #10116 but none of those had IPC timeouts. I also can easily reproduce those failures without IPC timeouts on a test MTL system. I'll try to dig some more into this today and possibly tomorrow, let's decide then.

update: I cannot really reproduce them, those were a different kind of failure

lyakh · 2025-07-22T08:12:35Z

src/audio/module_adapter/module_adapter.c

+	 * retrieve the params from the basecfg, allocate stream_params and then update the
+	 * sink/source buffer params.
+	 */
+	if (!mod->stream_params) {


this doesn't seem possible any more? It's now allocated in .new()

No it is possible when you restart a stream after a pipeline reset without freeing it

@ranj063 interesting. Could you explain a bit more just for my understanding - "restart" a stream - you don't mean stop -> start, just a reset, right? But when do such "resets" happen apart from when stopping streams?

@lyakh it never happens in our tests. But there's a QB test that does this sequence (it called randomDMAtest or something like that). I added this specifically to make that test pass:
set up pipelines -> start pipelines -> stop pipelines - >reset pipelines -> Put the DSP in D3 -> exit D3 -> prepare & restart pipelines -> stop pipelines -> reset pipelines -> free pipelines

kv2019i · 2025-07-28T10:19:24Z

@lyakh Are we good to go? Tests seems on par with other PRs now.

ranj063 requested review from dbaluta, jxstelter, kv2019i, lbetlej, lgirdwood, mmaka1 and plbossart as code owners July 3, 2025 20:15

kv2019i approved these changes Jul 4, 2025

View reviewed changes

lyakh previously requested changes Jul 7, 2025

View reviewed changes

ranj063 force-pushed the main-070225 branch from 319c806 to 8269eba Compare July 7, 2025 20:08

ranj063 force-pushed the main-070225 branch 2 times, most recently from ad23a94 to 90f94b7 Compare July 8, 2025 01:53

lyakh reviewed Jul 8, 2025

View reviewed changes

src/audio/module_adapter/module_adapter.c Show resolved Hide resolved

ranj063 force-pushed the main-070225 branch from 90f94b7 to 08d1702 Compare July 8, 2025 18:07

ranj063 requested review from fkwasowi and iganakov as code owners July 8, 2025 18:07

ranj063 force-pushed the main-070225 branch 11 times, most recently from e6e7b1e to a658cc1 Compare July 10, 2025 03:21

lyakh reviewed Jul 14, 2025

View reviewed changes

ranj063 force-pushed the main-070225 branch from c99c771 to 291cf04 Compare July 14, 2025 19:50

ranj063 force-pushed the main-070225 branch 3 times, most recently from 1c604fa to 5715eff Compare July 15, 2025 21:39

lyakh added the DNM Do Not Merge tag label Jul 16, 2025

lyakh mentioned this pull request Jul 16, 2025

[BUG] multiple platforms and topologies: multiple-pause-resume failures with "nothing to copy" #10116

Closed

ranj063 force-pushed the main-070225 branch from 5715eff to f2fca1b Compare July 16, 2025 16:42

ranj063 force-pushed the main-070225 branch from f2fca1b to be7f09a Compare July 16, 2025 21:17

lgirdwood approved these changes Jul 21, 2025

View reviewed changes

lyakh reviewed Jul 22, 2025

View reviewed changes

lyakh removed the DNM Do Not Merge tag label Jul 25, 2025

lyakh approved these changes Jul 29, 2025

View reviewed changes

kv2019i merged commit 86cbc3b into thesofproject:main Jul 29, 2025
40 of 45 checks passed


		kpb_set_params(dev, &params);

		ret = kbp_verify_params(dev, &params);

src: audio: module_adapter: Do the params config right after init #10094

src: audio: module_adapter: Do the params config right after init #10094

Uh oh!

Conversation

ranj063 commented Jul 3, 2025

Uh oh!

ranj063 commented Jul 3, 2025

Uh oh!

kv2019i left a comment

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!

ranj063 Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Jul 7, 2025

Uh oh!

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!

Uh oh!

Uh oh!

lyakh commented Jul 14, 2025

Uh oh!

ranj063 commented Jul 15, 2025

Uh oh!

ranj063 commented Jul 16, 2025

Uh oh!

ranj063 commented Jul 16, 2025

Uh oh!

lyakh commented Jul 16, 2025

Uh oh!

lgirdwood commented Jul 16, 2025

Uh oh!

lgirdwood commented Jul 16, 2025

Uh oh!

ranj063 commented Jul 16, 2025

Uh oh!

ranj063 commented Jul 16, 2025

Uh oh!

ranj063 commented Jul 17, 2025

Uh oh!

lyakh commented Jul 17, 2025

Uh oh!

ranj063 commented Jul 17, 2025

Uh oh!

ranj063 commented Jul 17, 2025

Uh oh!

lyakh commented Jul 18, 2025

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!

kv2019i commented Jul 28, 2025

Uh oh!

Uh oh!

Reviewers

ranj063 Jul 7, 2025 •

edited

Loading