audio: module_adapter: Remove use of comp_verify_params for IPC4 #10244
Conversation
ranj063
requested review from
dbaluta,
jxstelter,
kv2019i,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR removes the use of comp_verify_params for IPC4 in the module adapter component and reorganizes parameter handling to avoid unnecessary operations. Since IPC4 doesn't use buffer flags, the component parameter updates based on these flags are eliminated.
- Removed
comp_verify_paramscalls from module adapter functions for IPC4 - Moved buffer parameter setting to the component connection phase in
ipc_comp_connect - Added component period frames setting during module initialization
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/ipc/ipc4/helper.c | Added buffer parameter setting during component connection for module adapters |
| src/audio/module_adapter/module_adapter.c | Removed comp_verify_params calls and added period frames setting during initialization |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ranj063
force-pushed
the
fix/ipc4_comp_params
branch
from
cfbe8c8 to
14ad55b
Compare
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
One comment about IPC3, otherwise looks good.
ranj063
force-pushed
the
fix/ipc4_comp_params
branch
from
14ad55b to
e891b75
Compare
softwarecki
left a comment
softwarecki
left a comment
There was a problem hiding this comment.
Thanks for taking the time to clean this up. It's very confusing when a function whose name suggests it's for verification is actually responsible for setting buffer parameters.
|
@softwarecki who can help me with the QB failures? looks like NVL failed all the tests? |
|
Fails with 1 test. MTL |
ranj063
force-pushed
the
fix/ipc4_comp_params
branch
from
e891b75 to
5af4b1e
Compare
ranj063
force-pushed
the
fix/ipc4_comp_params
branch
from
5af4b1e to
7ab8fd7
Compare
ranj063
force-pushed
the
fix/ipc4_comp_params
branch
2 times, most recently
from
4f9b3ff to
a5de9c0
Compare
ranj063
requested review from
RanderWang,
marcinszkudlinski and
pblaszko
as code owners
ranj063
force-pushed
the
fix/ipc4_comp_params
branch
from
a5de9c0 to
b5032d5
Compare
| struct module_config *dst = &mod->priv.cfg; | ||
| struct sof_ipc_stream_params params; | ||
| struct comp_buffer *buffer; | ||
| int sink_queue_id = bind_data->ipc4_data->extension.r.dst_queue; |
There was a problem hiding this comment.
From the buffer's perspective, the sink is the source for the module. The adapter module looks at the buffer from the module's perspective, so it's the source for the module. To avoid discussion about the proper name, I suggest sticking with dst as it appears in the ipc4 structure.
| mod->stream_copy_single_to_single = !module_adapter_multi_sink_source_prepare(dev); | ||
|
|
||
| return 0; | ||
| return module_update_source_buffer_params(mod, bind_data); |
There was a problem hiding this comment.
Wouldn't it be better to call module_update_source_buffer_params before calling the module's bind function? It will allow module_bind to use already configured buffer parameters and allow change some parameter if it decides
There was a problem hiding this comment.
yeah I suppose that makes sense. Although going by today's logic that shouldnt happen because the buffer params arent updated until pipeline_prepare. But I think doing it before module_bind would make sense to make it more useful in the future.
| rfree(mod->stream_params); | ||
|
|
||
| mod->stream_params = rzalloc(SOF_MEM_FLAG_USER, | ||
| mod->stream_params = rzalloc(SOF_MEM_FLAG_USER | SOF_MEM_FLAG_COHERENT, |
There was a problem hiding this comment.
As I understand it, the module's bind function is called from the same core that the module is assigned to. Therefore, there's no need for this structure to be coherent.
Since this is the only place where this structure can be modified, I suggest marking the stream_params field as const and only perform a cache writeback in case someone from another core wants to look here.
ranj063
force-pushed
the
fix/ipc4_comp_params
branch
from
b5032d5 to
a29aff0
Compare
… params This will be used for converting the input/output audio formats in the base config extension to the stream params format. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
ranj063
force-pushed
the
fix/ipc4_comp_params
branch
from
a29aff0 to
6a50158
Compare
|
SOFCI TEST |
comp_verify_params does 3 things, update comp params based on buffer flags, then set buffer params based on comp params and then finally set the component period frames based on buffer frames. In the case of IPC4, buffer flags are unused, so there's no need to update comp params based on these flags. So, move the setting of buffer params during the modules binding call where the buffers are allocated. Finally, set the component period frames based on the module's params and the device period frames during module init Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
ranj063
force-pushed
the
fix/ipc4_comp_params
branch
from
6a50158 to
23bf8d5
Compare
7 participants
comp_verify_params does 3 things, update comp params based on buffer flags, then set buffer params based on comp params and then finally set the component period frames based on buffer frames. In the case of IPC4, buffer flags are unused, so there's no need to update comp params based on these flags. So, move the setting of buffer params during the modules binding call where the buffers are allocated. Finally, set the component period frames based on the module's params during module init.