Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Init config type for modules #130

Merged
merged 3 commits into from
Feb 9, 2023
Merged

Conversation

ranj063
Copy link
Contributor

@ranj063 ranj063 commented Feb 8, 2023

USe some reverved bits to add the init config information for modules that will be used to indicate whether the base config extension needs to be added to the init module payload or not.

Use some of the reserved bits to add a new field, init_config, in struct
sof_man_module_type. This will be used to specify the type of payload
that the module expects. For now, the 2 options are to have the base
config only or the base config with an extension that contains the pin
formats. This can be extended in the future to support additional
options for modules that need more than the base config or base config +
extension.

Signed-off-by: Ranjani Sridharan <[email protected]>
Smart amp needs the base config extension during init.

Signed-off-by: Ranjani Sridharan <[email protected]>
Smart amp needs the base config extension during module init.

Signed-off-by: Ranjani Sridharan <[email protected]>
@kv2019i kv2019i requested a review from mwasko February 9, 2023 09:12
@mwasko mwasko requested a review from pblaszko February 9, 2023 10:21
Copy link
Contributor

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good!

struct sof_man_module_type {
uint32_t load_type:4; /* SOF_MAN_MOD_TYPE_ */
uint32_t auto_start:1;
uint32_t domain_ll:1;
uint32_t domain_dp:1;
uint32_t lib_code:1;
uint32_t rsvd_:24;
uint32_t init_config:4; /* SOF_MAN_MOD_INIT_CONFIG_ */
uint32_t rsvd_:20;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be added to sof-docs as well (follow-up to thesofproject/sof-docs#451 ).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack - lets get it in the docs.

@pblaszko
Copy link
Contributor

pblaszko commented Feb 9, 2023

This implementation does not break anything itself, as it just reuses some reserved bits of module type in manifest.
But what is a purpose of this change? Modules with only base_cfg or with base_cfg+base_cfg_extension are only a subset of overall FW modules. Many infrastructure modules have more data in payload. Is your change intended for modules built with IADK?

@kv2019i
Copy link
Contributor

kv2019i commented Feb 9, 2023

@pblaszko wrote:

This implementation does not break anything itself, as it just reuses some reserved bits of module type in manifest. But what is > a purpose of this change? Modules with only base_cfg or with base_cfg+base_cfg_extension are only a subset of overall FW
modules. Many infrastructure modules have more data in payload. Is your change intended for modules built with IADK?

This is related to kernel PR 4160 and specifically this comment thesofproject/linux#4160 (comment) .

AFAIU, for base modules (like copier) the kernel can have hard-coded knowledge of each module, but for generic processing modules (like IADK ones), it's not scalable to need separate changes to Linux for each new type of module. I.e. the manifest needs to tell kernel if a module absolutely needs the base-ext to function correctly (smart amp is one example). Our earlier alternative was to use the topology file, but during the discussion it became apparent this is not the correct place to store the meta information.

This of course leaves still the case where a module has a completely custom initialization interface and it needs the complete set to function correctly. For these, the Linux kernel has to be modified to support the module. This is possible, but a very slow to deploy path -- this should really only be used for base modules such as copier.

@pblaszko
Copy link
Contributor

pblaszko commented Feb 9, 2023

@pblaszko wrote:

This implementation does not break anything itself, as it just reuses some reserved bits of module type in manifest. But what is > a purpose of this change? Modules with only base_cfg or with base_cfg+base_cfg_extension are only a subset of overall FW
modules. Many infrastructure modules have more data in payload. Is your change intended for modules built with IADK?

This is related to kernel PR 4160 and specifically this comment thesofproject/linux#4160 (comment) .

AFAIU, for base modules (like copier) the kernel can have hard-coded knowledge of each module, but for generic processing modules (like IADK ones), it's not scalable to need separate changes to Linux for each new type of module. I.e. the manifest needs to tell kernel if a module absolutely needs the base-ext to function correctly (smart amp is one example). Our earlier alternative was to use the topology file, but during the discussion it became apparent this is not the correct place to store the meta information.

This of course leaves still the case where a module has a completely custom initialization interface and it needs the complete set to function correctly. For these, the Linux kernel has to be modified to support the module. This is possible, but a very slow to deploy path -- this should really only be used for base modules such as copier.

Thank you @kv2019i !
So this will be an optimization for modules that fit into specified structures, but other modules with different structure can still be used (preferably only FW infrastructure). This is fine for me.

struct sof_man_module_type {
uint32_t load_type:4; /* SOF_MAN_MOD_TYPE_ */
uint32_t auto_start:1;
uint32_t domain_ll:1;
uint32_t domain_dp:1;
uint32_t lib_code:1;
uint32_t rsvd_:24;
uint32_t init_config:4; /* SOF_MAN_MOD_INIT_CONFIG_ */
uint32_t rsvd_:20;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack - lets get it in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants