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.

Sign up for GitHub

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

Better handling of module base_config_ext data #8685

Merged
merged 2 commits into from
Jan 9, 2024
Merged

Better handling of module base_config_ext data #8685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b61f80
module_adapter_ipc4: Save and pre-parse base_cfg_ext data
andyross Jan 2, 2024
877ef51
ipc4: Use saved base_cfg_ext data in ipc_comp_connect()
andyross Jan 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/audio/module_adapter/module_adapter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1333,6 +1333,10 @@ int module_adapter_reset(struct comp_dev *dev)
rfree(mod->stream_params);
mod->stream_params = NULL;

#if CONFIG_IPC_MAJOR_4
rfree(mod->priv.cfg.input_pins);
#endif

btian1 marked this conversation as resolved.
Show resolved Hide resolved
comp_dbg(dev, "module_adapter_reset(): done");

return comp_set_state(dev, COMP_TRIGGER_RESET);
Expand Down
38 changes: 29 additions & 9 deletions src/audio/module_adapter/module_adapter_ipc4.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,19 +38,39 @@ int module_adapter_init_data(struct comp_dev *dev,
const struct comp_ipc_config *config,
const void *spec)
{
if (dev->drv->type == SOF_COMP_MODULE_ADAPTER) {
const struct ipc_config_process *ipc_module_adapter = spec;
const struct ipc_config_process *args = spec;
const struct ipc4_base_module_extended_cfg *cfg = (void *)args->data;
size_t cfgsz = args->size;

dst->init_data = ipc_module_adapter->data;
dst->size = ipc_module_adapter->size;
dst->avail = true;
assert(dev->drv->type == SOF_COMP_MODULE_ADAPTER);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be ok. @jxstelter you added the if check on this in commit b570b80 , can you check?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would seem this is ok. Most modules use "DECLARE_MODULE_ADAPTER()" to register the driver (which sets the type correctly) and library_manager calls "declare_dynamic_module_adapter(drv, SOF_COMP_MODULE_ADAPTER, ..)". So I don't see any use for IPC4 where the type would not be set to adapter. It still bugs me a bit a specific branch was added to cover this case and I can't find from git history any use for this, but maybe this was some variant of loadable modules. @jxstelter @ranj063 @RanderWang @softwarecki @pjdobrowolski please do comment if I got this wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my read, yeah. The original had a check for that that then set the init_data unconditionally, which seems a little dangerous (what if it's not? what's in the data and how do we prevent treating it like pin config?". I grepped around and couldn't find any situations where it would happen, so I replaced it with an assert to be sure, figuring that CI would tell me if I missed something.

Copy link
Member

@lgirdwood lgirdwood Jan 9, 2024 •
edited by andyross
Loading

Choose a reason for hiding this comment

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

Is this really fatal ?

I'm pretty sure it just can't happen. This is a method on module_adapter, it sorta strains reason that it would somehow be called on anything else. And what grepping I could do seems to confirm that. But the previous version of the code had a check for the != case where it would fall back to just assigning init_data. So the assert is here to catch any mistakes or edge cases that crept through; no reason it should stay long term.

if (cfgsz < sizeof(cfg->base_cfg))
return -EINVAL;

memcpy_s(&dst->base_cfg, sizeof(dst->base_cfg), ipc_module_adapter->data,
sizeof(dst->base_cfg));
} else {
dst->init_data = spec;
dst->base_cfg = cfg->base_cfg;
dst->size = cfgsz;

if (cfgsz >= sizeof(*cfg)) {
int n_in = cfg->base_cfg_ext.nb_input_pins;
int n_out = cfg->base_cfg_ext.nb_output_pins;
size_t pinsz = (n_in * sizeof(*dst->input_pins))
+ (n_out * sizeof(*dst->output_pins));

if (cfgsz == (sizeof(*cfg) + pinsz)) {
dst->nb_input_pins = n_in;
dst->nb_output_pins = n_out;
dst->input_pins = rmalloc(SOF_MEM_ZONE_RUNTIME_SHARED,
0, SOF_MEM_CAPS_RAM, pinsz);
if (!dst->input_pins)
return -ENOMEM;

dst->output_pins = (void *)&dst->input_pins[n_in];
memcpy_s(dst->input_pins, pinsz,
&cfg->base_cfg_ext.pin_formats[0], pinsz);
andyross marked this conversation as resolved.
Show resolved Hide resolved
}
}

dst->init_data = cfg; /* legacy API */
dst->avail = true;
Comment on lines +52 to +73
Copy link
Member

Choose a reason for hiding this comment

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

This block could do with an inline comment describing what its doing.

return 0;
}

Expand Down
4 changes: 4 additions & 0 deletions src/include/module/module/base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ struct module_config {
const void *init_data; /**< Initial IPC configuration. */
#if CONFIG_IPC_MAJOR_4
struct ipc4_base_module_cfg base_cfg;
uint8_t nb_input_pins;
Copy link
Contributor

Choose a reason for hiding this comment

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

now we have this twice
struct processing_module->num_of_sinks
struct processing_module->num_of_sources

(not a blocker) add please at least an assert (by now) in module_adapter_dp_queue_prepare
Target - remove it from struct processing_module (I'll do it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're actually different values from different sources. The fields in processing_module count the number of actual comp_buffer objects connected to the module. The one here is a field set in an IPC message from the kernel based on topology input. In the case where the base_cfg_ext is provided, they surely should be the same value. But the extended config is actually not always present (some components have their own config structs here instead), so they might differ at runtime and we need to know how much memory was actually allocated behind the pointers here.

uint8_t nb_output_pins;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: is "nb" an abbreviation for "number?" If so, it seems a bit non-standard to me, maybe just "n" or "num" or even "number_of..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the pre-existing name for the field in struct ipc4_base_module_cfg_ext from which this is derived. Personally I don't like it much either, but I'm trying to be a good citizen and not rock the boat too much. :)

struct ipc4_input_pin_format *input_pins;
struct ipc4_output_pin_format *output_pins;
#endif
};

Expand Down
58 changes: 43 additions & 15 deletions src/ipc/ipc4/helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
#include <sof/audio/buffer.h>
#include <sof/audio/component.h>
#include <sof/audio/component_ext.h>
#include <sof/audio/module_adapter/module/generic.h>
#include <sof/audio/pipeline.h>
#include <module/module/base.h>
#include <sof/common.h>
#include <rtos/interrupt.h>
#include <sof/ipc/topology.h>
Expand Down Expand Up @@ -431,6 +433,9 @@ int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect)
struct ipc4_base_module_cfg source_src_cfg;
struct ipc4_base_module_cfg sink_src_cfg;
uint32_t flags;
uint32_t ibs = 0;
uint32_t obs = 0;
uint32_t buf_size;
int src_id, sink_id;
int ret;

Expand All @@ -453,17 +458,42 @@ int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect)
if (!cpu_is_me(source->ipc_config.core) && !cross_core_bind)
return ipc4_process_on_core(source->ipc_config.core, false);

/* these might call comp_ipc4_get_attribute_remote() if necessary */
ret = comp_get_attribute(source, COMP_ATTR_BASE_CONFIG, &source_src_cfg);
if (ret < 0) {
tr_err(&ipc_tr, "failed to get base config for module %#x", dev_comp_id(source));
return IPC4_FAILURE;
struct processing_module *srcmod = comp_get_drvdata(source);
struct processing_module *dstmod = comp_get_drvdata(sink);
struct module_config *dstcfg = &dstmod->priv.cfg;
struct module_config *srccfg = &srcmod->priv.cfg;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something missing in the patch, "struct processing_module" definition is not known and there are lot of fails in CI (defined in module/base.h). Now this is supposed to be private to module implementations, which is why I think @ranj063 opted to add a new property so it can be "set from outside".

Copy link
Collaborator

Choose a reason for hiding this comment

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

@andyross I do like the idea of parsing/saving the base_cfg_ext data in the module adapter instead of doing it in the individual modules. But I think you're going to have to add a new property to retrieve the extn data just like we do for the base_cfg data today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see the failures, presumably that's just a header ordering issue? Or if processing_module is an illegal header in some contexts, can you provide details? It would be easy enough to wrap these behind an API. That seems unlikely though, as the alternate code was already banging directly on the mod->priv.cfg.

@ranj063 you mean the COMP_ATTR_BASE_CONFIG_EXT from the version in your first PR? That's actually deliberately omitted: the only spot that would use it is using the original/kernel-derived data in the new parsed fields. The reason for making this a property would presumably be that components could then override it, but (and, OK, this is becoming a crusade of mine I guess) I think that's emphatically bad and should be avoided. If topology lied and gave us an invalid value, the job of the firmware is to fail gracefully and get the integrator to fix the topology.

Copy link
Collaborator

@ranj063 ranj063 Jan 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@andyross I think originally processing_module was intended to be private data accessible only by the module itself. But given that we are already accessing the base_cfg and base_cfg_ext from the common code, I guess it makes sense to make it shared. By the same argument maybe we should remove the COMP_ATTR_BASE_CONFIG attribute as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I'll look at that as a followup cleanup. The "kpb" component is the only spot I see in current code that's implementing the BASE_CONFIG attribute, and it's just echoing back the default base config anyway.


/* get obs from the base config extension if the src queue ID is non-zero */
if (bu->extension.r.src_queue && bu->extension.r.src_queue < srccfg->nb_output_pins)
obs = srccfg->output_pins[bu->extension.r.src_queue].obs;

/* get obs from base config if src queue ID is 0 or if base config extn is missing */
if (!obs) {
/* these might call comp_ipc4_get_attribute_remote() if necessary */
ret = comp_get_attribute(source, COMP_ATTR_BASE_CONFIG, &source_src_cfg);

if (ret < 0) {
tr_err(&ipc_tr, "failed to get base config for src module %#x",
dev_comp_id(source));
return IPC4_FAILURE;
}
obs = source_src_cfg.obs;
}

ret = comp_get_attribute(sink, COMP_ATTR_BASE_CONFIG, &sink_src_cfg);
if (ret < 0) {
tr_err(&ipc_tr, "failed to get base config for module %#x", dev_comp_id(sink));
return IPC4_FAILURE;
/* get ibs from the base config extension if the sink queue ID is non-zero */
if (bu->extension.r.dst_queue && bu->extension.r.dst_queue < dstcfg->nb_input_pins)
ibs = dstcfg->input_pins[bu->extension.r.dst_queue].ibs;

/* get ibs from base config if sink queue ID is 0 or if base config extn is missing */
if (!ibs) {
ret = comp_get_attribute(sink, COMP_ATTR_BASE_CONFIG, &sink_src_cfg);
if (ret < 0) {
tr_err(&ipc_tr, "failed to get base config for sink module %#x",
dev_comp_id(sink));
return IPC4_FAILURE;
}

ranj063 marked this conversation as resolved.
Show resolved Hide resolved
ibs = sink_src_cfg.ibs;
}

/* create a buffer
Expand All @@ -472,12 +502,10 @@ int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect)
* in case of DP -> LL
* size = 2*ibs of destination (LL) module. DP queue will handle obs of DP module
*/
uint32_t buf_size;

if (source->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_LL)
buf_size = source_src_cfg.obs * 2;
buf_size = obs * 2;
else
buf_size = sink_src_cfg.ibs * 2;
buf_size = ibs * 2;

buffer = ipc4_create_buffer(source, cross_core_bind, buf_size, bu->extension.r.src_queue,
bu->extension.r.dst_queue);
Expand All @@ -496,8 +524,8 @@ int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect)
* sink_module needs to set its IBS (input buffer size)
* as min_available in buffer's source ifc
*/
sink_set_min_free_space(audio_stream_get_sink(&buffer->stream), source_src_cfg.obs);
source_set_min_available(audio_stream_get_source(&buffer->stream), sink_src_cfg.ibs);
sink_set_min_free_space(audio_stream_get_sink(&buffer->stream), obs);
source_set_min_available(audio_stream_get_source(&buffer->stream), ibs);

/*
* Connect and bind the buffer to both source and sink components with LL processing been
Expand Down