[btian1]

@kv2019i note: this is split from #7909

[lyakh]

before this change mod->stream_params wasn't modified, it was used as a template, now it's modified. Is this correct?

[btian1]

Yes, exactly, I followed Ranjani's template:
d6e4583
It also changed mod->stream_params directly, so I think update mod->stream_params is necessary, what do you think?

[kv2019i]

@ranj063 and @btian1 This looks ok, but it would be good to have rationale recorded somewhere. The text in d6e4583 doesn't tell why we now set both sink and source formats, why this worked before this change and why this is needed. I mean using helpers instead of open-coding in each module is perfectly clear and does not need any explanations, but that patch also changes the functionality.

This probably obvious to you, but at least for me it's hard to keep up with the IPC4 module refactorings and what should be done and what not.

For this series, you could point to the the earlier commit in git commit, so anyone looking at this can connected to the original change.

[lyakh]

I'm confused so I'm going to give this PR a -1 for now, I think we have to clarify something. I tried to follow where parameters come from and how they're used, so I went back to a25cdbb and I now don't think it's quite correct. At the very least it doesn't seem to do what its description is saying. It isn't updating parameters, it's degrading them. Parameters are allocated in module_adapter_params() and copied from the IPC, but then that commit overwrites it with original data from .base_cfg. Now, I think with IPC4 data "from the IPC" is actually fake and all 0 - it comes from

sof/src/ipc/ipc4/handler.c

Line 117 in 214f983

struct sof_ipc_pcm_params hw_params = {{ 0 }};

but then some of it is overwritten in

sof/src/audio/module_adapter/module_adapter.c

Line 522 in 214f983

ret = comp_verify_params(dev, mod->verify_params_flags, params);

and also some of it is used to configure buffers e.g. in

sof/src/ipc/ipc-helper.c

Line 140 in 214f983

buffer_set_params(buf_c, params, BUFFER_UPDATE_FORCE);

or in

sof/src/ipc/ipc-helper.c

Line 156 in 214f983

buffer_set_params(buf_c, params, BUFFER_UPDATE_FORCE);

. Then parameters are overwritten, but buffer parameters remain as they were set in comp_verify_params()... At least should make it obvious in module_adapter_params() and only use that data for IPC3? Then, those two added lines inhttps://github.com/thesofproject/sof/commit/a25cdbb90b67be1f08c81e8de1641b7cefdf5fd5 - they fill the same data into two locations - can we just use memcpy() for the second one? And do we actually need to write data to params - it's a local variable in

sof/src/ipc/ipc4/handler.c

Line 117 in 214f983

struct sof_ipc_pcm_params hw_params = {{ 0 }};

and it will be used for all components in the pipeline while recursing in

sof/src/ipc/ipc4/handler.c

Line 130 in 214f983

return param_ctx.comp_func(host, NULL, &param_ctx, host->direction);

so every component (if it's using the module adapter API) will just overwrite it with its own copy...
Sorry for a long and not very consistent comment, but I think we need to clean up and clarify a bit before we add more complexity to parameter update and propagation flow

[btian1]

@lyakh , @kv2019i really thanks you bring so much for this topic, how to say, like I may have similar feelings like you, just can't change too much a time(CI need pass), I have below things want to clarify:

1. Nobody said the logic can't change, yes, params are changing compared with previous.
2. there are also other functions to change params, like comp_verify_params, comp_update_params.

so, I will change first when there is a reason and check CI and then your comments, somehow, CI is first gold criteria for me.

I can remove these part of changes first, and then discuss further for the actual changes to unblock the PR merge, let me know what do you think.

[lyakh]

@btian1 in fact I'd even try to make a PR to clean up and clarify my above comments and to add any of your fixes to that parameter handling code, if needed. And then update this PR on top of that one. I think that order would be better because fixes should come before optimisations. Alternatively you could still remove those changes from this PR and add a PR after this one, but that would probably (1) first add more confusion to the current situation in this PR, and (2) make it then more difficult to fix it in a follow-up.

[btian1]

ok, let me remove params related patches as first step to make this PR can be reviewed and merged, and we can discuss later for params related changes.

[lyakh]

same here

[btian1]

yes, check previous comments

[lyakh]

mod->stream_params is modified

[kv2019i]

Thanks for splitting this out @btian1 ! I think this is very good cleanup for the most part. There are a few smaller things were we change functionality and it's not a plain refactoring. I'm ok with this but please @ranj063 ack these are correct.

And a generic rant and reminder that please let's remember to add rationale to git commit message if we a change is not a simpler refactor.

[kv2019i]

@ranj063 and @btian1 This looks ok, but it would be good to have rationale recorded somewhere. The text in d6e4583 doesn't tell why we now set both sink and source formats, why this worked before this change and why this is needed. I mean using helpers instead of open-coding in each module is perfectly clear and does not need any explanations, but that patch also changes the functionality.

This probably obvious to you, but at least for me it's hard to keep up with the IPC4 module refactorings and what should be done and what not.

For this series, you could point to the the earlier commit in git commit, so anyone looking at this can connected to the original change.

[kv2019i]

Same comments as in #7909 . @ranj063 is this safe to remove. I remember @lgirdwood comment in loadable modules PRs he wants generic module code to maintain/enforce the module state machine. If we remove checks from generic.c, that's going to the other direction.

[ranj063]

@kv2019i this is safe to remove for IPC4 but as for IPC3, I think i'd like @dbaluta to confirm is it is OK to remove for the cadence codecs

[btian1]

@dbaluta , will IPC3 also support cadence codecs?

[dbaluta]

@btian1 yes!

[btian1]

so we can't remove those ipc3 specific code? @dbaluta @lyakh @ranj063, since ipc3 need it? from my observation, I did not see those code is must even for IPC3

[dbaluta]

@btian1 we can't remove IPC3 specific code as IMX uses it.

This patch works fine for imx cadence tests. You just need to do the following changes:

diff --git a/src/audio/module_adapter/module/cadence.c b/src/audio/module_adapter/module/cadence.c
index ef912d7e2..dd0654228 100644
--- a/src/audio/module_adapter/module/cadence.c
+++ b/src/audio/module_adapter/module/cadence.c
@@ -353,7 +353,7 @@ static int cadence_codec_init(struct processing_module *mod)
        codec->mpd.init_done = 0;
 
        /* copy the setup config only for the first init */
-       if (codec->state == MODULE_DISABLED && codec->cfg.avail) {
+       if (codec->cfg.avail) {
                setup_cfg = &cd->setup_cfg;
 
                /* allocate memory for set up config */
@@ -852,7 +852,6 @@ cadence_codec_set_configuration(struct processing_module *mod, uint32_t config_i
                                const uint8_t *fragment, size_t fragment_size, uint8_t *response,
                                size_t response_size)
 {
-       struct module_data *md = &mod->priv;
        struct comp_dev *dev = mod->dev;
        int ret;

[lgirdwood]

@btian1 IPC3 is used on non Intel platforms so must be supported in any change PR.

[btian1]

@dbaluta yes, I did it last night, if this patch works fine for imx ipc3 cadence codec test, means md->state can be removed for both ipc4 and ipc3, if so, could you add your approval to this PR?

@lgirdwood ack, due to CI does not check ipc3 build, some place change are missed, I added last night.

This change is not a irrelevant change, it is critical for host performance in #7909

[RanderWang]

Good job! Waiting for IPC3 part check

[lyakh]

Can we hold on with this a bit until we clarify my doubts about parameter setting. I'll give this -1 for now just to make sure that we don't forget to answer these questions before merging

[lyakh]

I'm confused so I'm going to give this PR a -1 for now, I think we have to clarify something. I tried to follow where parameters come from and how they're used, so I went back to a25cdbb and I now don't think it's quite correct. At the very least it doesn't seem to do what its description is saying. It isn't updating parameters, it's degrading them. Parameters are allocated in module_adapter_params() and copied from the IPC, but then that commit overwrites it with original data from .base_cfg. Now, I think with IPC4 data "from the IPC" is actually fake and all 0 - it comes from

sof/src/ipc/ipc4/handler.c

Line 117 in 214f983

struct sof_ipc_pcm_params hw_params = {{ 0 }};

but then some of it is overwritten in

sof/src/audio/module_adapter/module_adapter.c

Line 522 in 214f983

ret = comp_verify_params(dev, mod->verify_params_flags, params);

and also some of it is used to configure buffers e.g. in

sof/src/ipc/ipc-helper.c

Line 140 in 214f983

buffer_set_params(buf_c, params, BUFFER_UPDATE_FORCE);

or in

sof/src/ipc/ipc-helper.c

Line 156 in 214f983

buffer_set_params(buf_c, params, BUFFER_UPDATE_FORCE);

. Then parameters are overwritten, but buffer parameters remain as they were set in comp_verify_params()... At least should make it obvious in module_adapter_params() and only use that data for IPC3? Then, those two added lines inhttps://github.com/thesofproject/sof/commit/a25cdbb90b67be1f08c81e8de1641b7cefdf5fd5 - they fill the same data into two locations - can we just use memcpy() for the second one? And do we actually need to write data to params - it's a local variable in

sof/src/ipc/ipc4/handler.c

Line 117 in 214f983

struct sof_ipc_pcm_params hw_params = {{ 0 }};

and it will be used for all components in the pipeline while recursing in

sof/src/ipc/ipc4/handler.c

Line 130 in 214f983

return param_ctx.comp_func(host, NULL, &param_ctx, host->direction);

so every component (if it's using the module adapter API) will just overwrite it with its own copy...
Sorry for a long and not very consistent comment, but I think we need to clean up and clarify a bit before we add more complexity to parameter update and propagation flow

[dbaluta]

@btian1 will give it a test today!

[dbaluta]

This PR doesn't compile with IPC3

/home/nxf25322/work/sof_dir/sof/src/audio/module_adapter/module/cadence.c: In function 'cadence_codec_init':
/home/nxf25322/work/sof_dir/sof/src/audio/module_adapter/module/cadence.c:356:11: error: 'struct module_data' has no member named 'state'
  if (codec->state == MODULE_DISABLED && codec->cfg.avail) {
           ^~
/home/nxf25322/work/sof_dir/sof/src/audio/module_adapter/module/cadence.c:356:22: error: 'MODULE_DISABLED' undeclared (first use in this function); did you mean 'LOG_DISABLE'?
  if (codec->state == MODULE_DISABLED && codec->cfg.avail) {
                      ^~~~~~~~~~~~~~~
                      LOG_DISABLE
/home/nxf25322/work/sof_dir/sof/src/audio/module_adapter/module/cadence.c:356:22: note: each undeclared identifier is reported only once for each function it appears in
/home/nxf25322/work/sof_dir/sof/src/audio/module_adapter/module/cadence.c: In function 'cadence_codec_set_configuration':
/home/nxf25322/work/sof_dir/sof/src/audio/module_adapter/module/cadence.c:855:22: error: unused variable 'md' [-Werror=unused-variable]
  struct module_data *md = &mod->priv;
                      ^~
cc1: all warnings being treated as errors
make[2]: *** [CMakeFiles/sof.dir/build.make:1123: CMakeFiles/sof.dir/src/audio/module_adapter/module/cadence.c.o] Error 1

[lgirdwood]

@btian1 it may be simpler here for any IPC changes to split between files i.e.
we could have file-generic.c (core and generic feature code) with file-ipc3.c and file-ipc4.c (for IPC specifc feature adaptions).

[lgirdwood]

@btian1 IPC3 is used on non Intel platforms so must be supported in any change PR.

[lgirdwood]

@dbaluta good for you now ?

[juimonen]

changes look fine with me if CI is happy...

[btian1]

d for you n

There is no ipc3 specific changes in this PR, @dbaluta

[ranj063]

a part of this is in the previous commit. Can you please reorganize to move all of the same change to the same patch?

[btian1]

done

[lgirdwood]

@dbaluta good for you - assuming so as @iuliana-prodan has approved ?

[btian1]

@dbaluta good for you - assuming so as @iuliana-prodan has approved ?

yes, Prodan helped verify and approved, since Baluta is taking annual leave.

[kv2019i]

https://sof-ci.01.org/sofpr/PR7958/build11365/devicetest/index.html and https://sof-ci.01.org/sofpr/PR7958/build11366/devicetest/index.html are ok, with known fails only. With @iuliana-prodan 's ack in place of @dbaluta , I'll proceed with merge.

Comment addressed and Prodan acked the change for NXP