Add extraction of new component properties from topology and pass them to modules with new ext_init object array. #5460
Conversation
jsarha
force-pushed
the
new_component_attributes
branch
from
38a9088 to
d22b571
Compare
sound/soc/sof/ipc4-topology.c
Outdated
There was a problem hiding this comment.
@jsarha does it make sense to move this to each widget's setup op? I mean this is static data parsed from the topology right? So it isnt going to change from every instance of start/stop isnt it?
There was a problem hiding this comment.
I put it there only because that is where the IPC payload is put together. The ext_init payload could be created already at sof_ipc4_widget_setup_msg() and store it somewhere, but we would still have to allocate memory and concatenate different payloads together in sof_ipc4_widget_setup(), so the benefit would be mostly cosmetic.
That is unless I take on a bigger rework, find a standard place for setup msg payloads in snd_sof_widget for all modules (except pipelines), and put custom made payloads there. That would still only be a trade from saved cycles to consumed memory.
There was a problem hiding this comment.
@jsarha why would you need to allocate memory only here? You can do the same earlier as well.
There was a problem hiding this comment.
Currently the payloads are module specific, initialized where ever, and not having common place in sof_widget. The ext_init payload however would not depend on the module its using. It would be relatively straight forward to add a pointer for storing the ext_init payload to sof_widget in setup_msg(). However, the ext_init payload should still be concatenated with the current module specific payload in widget_setup().
I guess it would be possible to create the full IPC payloads also already at topology read phase. That would however require fixing the current situation, where there is no common place for init message payloads, and each module code stores it somewhere in its own private memory space.
jsarha
force-pushed
the
new_component_attributes
branch
from
d22b571 to
a858b2f
Compare
include/sound/sof/ipc4/header.h
Outdated
There was a problem hiding this comment.
what are the options for domain ID?
There was a problem hiding this comment.
Its supposed to describe a domain in usespace that can share data or something like that. I do not exactly know, and the implementation does not exist yet. @lgirdwood asked me to add it.
sound/soc/sof/ipc4-topology.c
Outdated
There was a problem hiding this comment.
return 1? This looks awkward. How about returning the size of the payload?
sound/soc/sof/ipc4-topology.c
Outdated
There was a problem hiding this comment.
I think we should return new_size instead of passing the pointer to it
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
One comment I left also to FW PR review.
include/sound/sof/ipc4/header.h
Outdated
There was a problem hiding this comment.
Same comment as to FW 10064. We already have a concept of "extended init". E.g. we have:
» struct sof_ipc4_base_module_cfg base_config;
» struct sof_ipc4_base_module_cfg_ext *base_config_ext;
All modules have base_config, but some have base_config_ext additionally. This new PR (and the bit29) seems to add a way to extend the module init data, but still keeps the old "base_config_ext" around as well. Having too "extended" module configs is highly confusing. Can we call this new variant something clearly different?
There was a problem hiding this comment.
I address all the other comments first and push a new version. I take on this tomorrow, when I am certain we have an agreement on what to do. Let's continue the discussion on FW PR side.
…bytes Add topology tokens for defining user-space domain_id, required stack and heap size byte for a component. The new topology tokens are SOF_TKN_COMP_DOMAIN_ID, SOF_TKN_COMP_HEAP_BYTES_REQUIREMENT and SOF_TKN_COMP_STACK_BYTES_REQUIREMENT for defining required stack and heap size for a component. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
jsarha
force-pushed
the
new_component_attributes
branch
from
a858b2f to
dd43c3e
Compare
|
@ranj063 your comments should be addressed, could you check? I try to agree with @kv2019i on #5460 (comment) tomorrow. |
|
@jsarha there are some legit checkpatch warnings. Can you please fix those? And I think its better to use u32 over uint32_t. I understand you want to keep it aligned with the firmware side but even so someone else will definitely complain about it. SO better to do it ourselves. |
Add dp_domain_id, dp_heap_bytes and dp_stack_bytes to struct snd_sof_widget and fill the values from topology tuples with SOF_TKN_COMP_DOMAIN_ID, SOF_TKN_COMP_STACK_BYTES_REQUIREMENT and SOF_TKN_COMP_HEAP_BYTES_REQUIREMENT tokens. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Add structs and macros for struct sof_ipc4_module_init_ext_init, following struct sof_ipc4_module_init_ext_object array, and struct sof_ipc4_mod_init_ext_dp_memory_data as object payload. Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
Add of_ipc4_widget_setup_msg_payload() for adding struct sof_ipc4_module_init_ext_init payload with associated objects. The function allocates memory for the additional payload, sets up the payload according to data collected from topology, and copies pre-encoded module specific payload after the ext_init payload. The function is called in sof_ipc4_widget_setup(). Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
I do not know which ones you considered legit, and do not have time to be stubborn any more, so I fixed all of them... Even if shortening 75 char commit message line due to "Prefer a maximum 75 chars per line" feels a bit stupid, and there were some reasonable arguments against some other warnings too. |
jsarha
force-pushed
the
new_component_attributes
branch
from
dd43c3e to
b7e8825
Compare
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
yep, I think I mentioned u32 before ;-)
|
@ujfalusi , I hoped your approval would mean that this is to be merged soon, but its been two weeks now. Are we waiting for something? |
|
@jsarha, the stars now aligned, sorry for the delay. |
5 participants
Add new module init payload (this has already been partly specified in SOF FW src/include/ipc4/module.h) and use that to pass new topology widget properties to FW. The original ext_init payload needed some extension for this and there is a draft PR supporting this new payload here:
thesofproject/sof#10064