From 1b61f80891acc4b04e7652022fde82eeda96b182 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Tue, 2 Jan 2024 11:29:05 -0800 Subject: [PATCH 1/2] module_adapter_ipc4: Save and pre-parse base_cfg_ext data The kernel-provided "base_cfg_ext" data wasn't being handled correctly by the module adapter layer. These structs (packed wire formats) only ever lived in the IPC memory. The module would set them on an "init_data" before calling into driver init, and then clear that pointer afterwards. That's a critical problem, because the values in that struct need to be used at pipeline setup time to configure buffer formats! Save the data correctly. Also pre-parse it so users don't need to do byte math on raw buffers and can just use "in/output_pins[]" arrays on the module_config struct. Signed-off-by: Andy Ross --- src/audio/module_adapter/module_adapter.c | 4 ++ .../module_adapter/module_adapter_ipc4.c | 38 ++++++++++++++----- src/include/module/module/base.h | 4 ++ 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/audio/module_adapter/module_adapter.c b/src/audio/module_adapter/module_adapter.c index 9c56beddb319..1cc6cf83816c 100644 --- a/src/audio/module_adapter/module_adapter.c +++ b/src/audio/module_adapter/module_adapter.c @@ -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 + comp_dbg(dev, "module_adapter_reset(): done"); return comp_set_state(dev, COMP_TRIGGER_RESET); diff --git a/src/audio/module_adapter/module_adapter_ipc4.c b/src/audio/module_adapter/module_adapter_ipc4.c index 1e0257b0a1cb..93d03d93d459 100644 --- a/src/audio/module_adapter/module_adapter_ipc4.c +++ b/src/audio/module_adapter/module_adapter_ipc4.c @@ -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); + 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); + } } + dst->init_data = cfg; /* legacy API */ + dst->avail = true; return 0; } diff --git a/src/include/module/module/base.h b/src/include/module/module/base.h index 904b83741961..323599b055f2 100644 --- a/src/include/module/module/base.h +++ b/src/include/module/module/base.h @@ -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; + uint8_t nb_output_pins; + struct ipc4_input_pin_format *input_pins; + struct ipc4_output_pin_format *output_pins; #endif }; From 877ef512eaf5b168bcdda1d26f28ff6ea95d20c3 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Tue, 2 Jan 2024 16:43:44 -0800 Subject: [PATCH 2/2] ipc4: Use saved base_cfg_ext data in ipc_comp_connect() The code here never really worked right. The module base config values weren't saved by the core layers, so querying the attribute returned nothing. A few components have been modified with a whitebox workaround where they write directly to the basecfg_ext field in the module_config, but that's not really an API we should want to support. Get the values from the now-correctly-saved-and-parsed fields the module init layer has left for us, with considerable code savings and much less heap thrashing for the temporary copy. Signed-off-by: Andy Ross --- src/ipc/ipc4/helper.c | 58 ++++++++++++++++++++++++++++++++----------- 1 file changed, 43 insertions(+), 15 deletions(-) diff --git a/src/ipc/ipc4/helper.c b/src/ipc/ipc4/helper.c index 4006ce82be8d..369e94a70a2c 100644 --- a/src/ipc/ipc4/helper.c +++ b/src/ipc/ipc4/helper.c @@ -8,7 +8,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -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; @@ -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; + + /* 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; + } + + ibs = sink_src_cfg.ibs; } /* create a buffer @@ -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); @@ -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