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

Jump to bottom

module: Add support for saving and retrieving the base config extension #8575

Closed
wants to merge 1 commit into from
Closed

module: Add support for saving and retrieving the base config extension #8575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 31 additions & 5 deletions src/audio/google/google_rtc_audio_processing.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,18 +392,36 @@ static int google_rtc_audio_processing_init(struct processing_module *mod)
#if CONFIG_IPC_MAJOR_4
const struct ipc4_base_module_extended_cfg *base_cfg = md->cfg.init_data;
struct ipc4_input_pin_format reference_fmt, output_fmt;
const size_t size = sizeof(struct ipc4_input_pin_format);
size_t in_fmt_size = sizeof(struct ipc4_input_pin_format);
size_t out_fmt_size = sizeof(struct ipc4_output_pin_format);
size_t size;

cd->config.base_cfg = base_cfg->base_cfg;

/* Copy the reference format from input pin 1 format */
memcpy_s(&reference_fmt, size,
&base_cfg->base_cfg_ext.pin_formats[size], size);
memcpy_s(&output_fmt, size,
&base_cfg->base_cfg_ext.pin_formats[size * GOOGLE_RTC_NUM_INPUT_PINS], size);
memcpy_s(&reference_fmt, in_fmt_size,
&base_cfg->base_cfg_ext.pin_formats[in_fmt_size], in_fmt_size);
memcpy_s(&output_fmt, in_fmt_size,
&base_cfg->base_cfg_ext.pin_formats[in_fmt_size * GOOGLE_RTC_NUM_INPUT_PINS],
in_fmt_size);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this copy output-sized format? I.e.

memcpy_s(&output_fmt, out_fmt_size, , out_fmt_size);

..?

Copy link
Contributor

Choose a reason for hiding this comment

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

and since size is same, I would prefer use memcpy(dst, src, size) to reduce one parameters.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, the array index [in_fmt_size], looks like copy paste error (even though its replacing a similar size/index bit of code) ?


cd->config.reference_fmt = reference_fmt.audio_fmt;
cd->config.output_fmt = output_fmt.audio_fmt;

/* save the base config extension */
size = base_cfg->base_cfg_ext.nb_input_pins * in_fmt_size +
base_cfg->base_cfg_ext.nb_output_pins * out_fmt_size;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: you could have

	size = base_cfg->base_cfg_ext.nb_input_pins * in_fmt_size +
		base_cfg->base_cfg_ext.nb_output_pins * out_fmt_size +
		sizeof(struct ipc4_base_module_cfg_ext);

Copy link
Contributor

Choose a reason for hiding this comment

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

@lyakh , no , see line 416, extra struct size are added when rzalloc

Copy link
Collaborator

Choose a reason for hiding this comment

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

@btian1 that's exactly why I'm proposing that


md->cfg.basecfg_ext = rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM,
sizeof(struct ipc4_base_module_cfg_ext) + size);
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 SOF_MEM_ZONE_RUNTIME_SHARED or else to have SOF_MEM_CAPS_COHERENT. By default you get cached memory from the heap, and this code is running in a DP component on another core. Globally-shared data like module config either needs to be uncached or to be accessed via a cache-aware wrapper API.

Copy link
Contributor

@marcinszkudlinski marcinszkudlinski Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@andyross that is not true in this case.
All operations - create/configure/run/delete/whatever will be performed by a core the module is bound to. No exceptions. Using cached structures is safe

if (!md->cfg.basecfg_ext) {
ret = -ENOMEM;
goto fail;
}

memcpy_s(md->cfg.basecfg_ext, size + sizeof(struct ipc4_base_module_cfg_ext),
&base_cfg->base_cfg_ext,
size + sizeof(struct ipc4_base_module_cfg_ext));
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: "size + sizeof(struct ipc4_base_module_cfg_ext)" is repeated three times, maybe use a local variable for readability?

Copy link
Contributor

Choose a reason for hiding this comment

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

this is not a complex operation, we should make code more straightforward to read.
total_size = size + sizeof(struct ipc4_base_module_cfg_ext);

#endif

cd->tuning_handler = comp_data_blob_handler_new(dev);
Expand Down Expand Up @@ -510,6 +528,9 @@ static int google_rtc_audio_processing_init(struct processing_module *mod)
rfree(cd->memory_buffer);
rfree(cd->raw_mic_buffer);
comp_data_blob_handler_free(cd->tuning_handler);
#if CONFIG_IPC_MAJOR_4
rfree(md->cfg.basecfg_ext);
#endif
rfree(cd);
}

Expand All @@ -530,6 +551,11 @@ static int google_rtc_audio_processing_free(struct processing_module *mod)
rfree(cd->memory_buffer);
rfree(cd->raw_mic_buffer);
comp_data_blob_handler_free(cd->tuning_handler);
#if CONFIG_IPC_MAJOR_4
struct module_data *md = &mod->priv;

rfree(md->cfg.basecfg_ext);
#endif
rfree(cd);
return 0;
}
Expand Down
16 changes: 16 additions & 0 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 @@ -148,6 +148,22 @@ int module_adapter_get_attribute(struct comp_dev *dev, uint32_t type, void *valu
memcpy_s(value, sizeof(struct ipc4_base_module_cfg),
&mod->priv.cfg.base_cfg, sizeof(mod->priv.cfg.base_cfg));
break;
case COMP_ATTR_BASE_CONFIG_EXT:
{
struct ipc4_base_module_cfg_ext *basecfg_ext = mod->priv.cfg.basecfg_ext;
size_t size;

if (!basecfg_ext) {
comp_err(mod->dev, "No base config extn set for module");
return -EINVAL;
}

size = basecfg_ext->nb_input_pins * sizeof(struct ipc4_input_pin_format) +
basecfg_ext->nb_output_pins * sizeof(struct ipc4_output_pin_format);
Copy link
Collaborator

Choose a reason for hiding this comment

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

similar here with size

memcpy_s(value, sizeof(*basecfg_ext) + size,
basecfg_ext, sizeof(*basecfg_ext) + size);
break;
}
default:
return -EINVAL;
}
Expand Down
1 change: 1 addition & 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,7 @@ struct module_config {
const void *init_data; /**< Initial IPC configuration. */
#if CONFIG_IPC_MAJOR_4
struct ipc4_base_module_cfg base_cfg;
struct ipc4_base_module_cfg_ext *basecfg_ext;
#endif
};

Expand Down
1 change: 1 addition & 0 deletions src/include/sof/audio/component.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ enum {
#define COMP_ATTR_COPY_DIR 2 /**< Comp copy direction */
#define COMP_ATTR_VDMA_INDEX 3 /**< Comp index of the virtual DMA at the gateway. */
#define COMP_ATTR_BASE_CONFIG 4 /**< Component base config */
#define COMP_ATTR_BASE_CONFIG_EXT 5 /**< Component base config extension */
/** @}*/

/** \name Trace macros
Expand Down
53 changes: 41 additions & 12 deletions src/include/sof/audio/component_ext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#ifndef __SOF_AUDIO_COMPONENT_INT_H__
#define __SOF_AUDIO_COMPONENT_INT_H__

#include <sof/audio/module_adapter/module/generic.h>
#include <sof/audio/component.h>
#include <ipc/topology.h>

Expand Down Expand Up @@ -216,31 +217,59 @@ struct get_attribute_remote_payload {
static inline int comp_ipc4_get_attribute_remote(struct comp_dev *dev, uint32_t type,
void *value)
{
struct ipc4_base_module_cfg_ext *basecfg_ext;
struct ipc4_base_module_cfg *base_cfg;
struct get_attribute_remote_payload payload = {};
struct idc_msg msg = { IDC_MSG_GET_ATTRIBUTE,
IDC_EXTENSION(dev->ipc_config.id), dev->ipc_config.core,
sizeof(payload), &payload};
size_t size = MODULE_MAX_SINKS * sizeof(struct ipc4_output_pin_format) +
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor: I found "size" a too generic as it's only used for one type of attributes. I.e. size of what? "ext_size"

MODULE_MAX_SOURCES * sizeof(struct ipc4_input_pin_format);
int ret;

/* Only COMP_ATTR_BASE_CONFIG is supported for remote access */
if (type != COMP_ATTR_BASE_CONFIG)
return -EINVAL;
payload.type = type;

base_cfg = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*base_cfg));
if (!base_cfg)
return -ENOMEM;
/*
* Only COMP_ATTR_BASE_CONFIG and COMP_ATTR_BASE_CONFIG_EXT are supported for
* remote access
*/
switch (type) {
case COMP_ATTR_BASE_CONFIG:
base_cfg = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM,
sizeof(*base_cfg));
if (!base_cfg)
return -ENOMEM;

payload.value = base_cfg;
break;
case COMP_ATTR_BASE_CONFIG_EXT:
basecfg_ext = rzalloc(SOF_MEM_ZONE_RUNTIME_SHARED, 0, SOF_MEM_CAPS_RAM,
sizeof(*basecfg_ext) + size);
if (!basecfg_ext)
return -ENOMEM;

payload.type = type;
payload.value = base_cfg;
payload.value = basecfg_ext;
break;
default:
return -EINVAL;
}

ret = idc_send_msg(&msg, IDC_BLOCKING);

if (ret == 0)
memcpy_s(value, sizeof(struct ipc4_base_module_cfg),
base_cfg, sizeof(struct ipc4_base_module_cfg));
if (!ret) {
switch (type) {
case COMP_ATTR_BASE_CONFIG:
memcpy_s(value, sizeof(struct ipc4_base_module_cfg),
base_cfg, sizeof(struct ipc4_base_module_cfg));
rfree(base_cfg);
break;
case COMP_ATTR_BASE_CONFIG_EXT:
memcpy_s(value, size, basecfg_ext, size);
Copy link
Collaborator

Choose a reason for hiding this comment

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

here without sizeof(*basecfg_ext), right?

rfree(basecfg_ext);
break;
}
}

rfree(base_cfg);
return ret;
}
#endif /* CONFIG_IPC_MAJOR_4 */
Expand Down
1 change: 1 addition & 0 deletions src/include/sof/audio/module_adapter/module/generic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@

#define MAX_BLOB_SIZE 8192
#define MODULE_MAX_SOURCES 8
#define MODULE_MAX_SINKS 8

#define API_CALL(cd, cmd, sub_cmd, value, ret) \
do { \
Expand Down
106 changes: 84 additions & 22 deletions src/ipc/ipc4/helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#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 <sof/common.h>
#include <rtos/interrupt.h>
Expand Down Expand Up @@ -424,15 +425,20 @@ static int ll_wait_finished_on_core(struct comp_dev *dev)

int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect)
{
struct ipc4_base_module_cfg_ext *src_basecfg_ext, *sink_basecfg_ext;
struct ipc4_module_bind_unbind *bu;
struct comp_buffer *buffer;
struct comp_dev *source;
struct comp_dev *sink;
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;
size_t size;

bu = (struct ipc4_module_bind_unbind *)_connect;
src_id = IPC4_COMP_ID(bu->primary.r.module_id, bu->primary.r.instance_id);
Expand All @@ -453,32 +459,88 @@ 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;
size = MODULE_MAX_SINKS * sizeof(struct ipc4_output_pin_format) +
MODULE_MAX_SOURCES * sizeof(struct ipc4_input_pin_format);

/* get obs from the base config extension if the src queue ID is non-zero */
if (bu->extension.r.src_queue) {
struct ipc4_output_pin_format *out_fmt;
size_t output_fmt_offset;

src_basecfg_ext = rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM,
sizeof(*src_basecfg_ext) + size);
if (!src_basecfg_ext)
return IPC4_OUT_OF_MEMORY;

ret = comp_get_attribute(source, COMP_ATTR_BASE_CONFIG_EXT, src_basecfg_ext);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks scary... Can we add a size argument to comp_get_attribute()?

if (!ret) {
size_t in_fmt_size = sizeof(struct ipc4_input_pin_format);
size_t out_fmt_size = sizeof(struct ipc4_output_pin_format);

/*
* offset of the format for the output pin with ID 'src_queue' within the
* base config extension.
*/
output_fmt_offset =
btian1 marked this conversation as resolved.
Show resolved Hide resolved
src_basecfg_ext->nb_input_pins * in_fmt_size +
bu->extension.r.src_queue * out_fmt_size;
out_fmt = (struct ipc4_output_pin_format *)&sink_basecfg_ext->pin_formats[output_fmt_offset];
obs = out_fmt->obs;
}
rfree(src_basecfg_ext);
}

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 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;
}

/* create a buffer
* in case of LL -> LL or LL->DP
* size = 2*obs of source module (obs is single buffer size)
* in case of DP -> LL
* size = 2*ibs of destination (LL) module. DP queue will handle obs of DP module
*/
uint32_t buf_size;
/* get ibs from the base config extension if the sink queue ID is non-zero */
if (bu->extension.r.dst_queue) {
struct ipc4_input_pin_format *in_fmt;
size_t input_fmt_offset;

sink_basecfg_ext = rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM,
Copy link
Member

Choose a reason for hiding this comment

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

Btw, if these are always copied and freed (in the same func), why not just have on the stack ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because basecfg_ext structure contains a flexible array member for the number of pin formats

Copy link
Collaborator

Choose a reason for hiding this comment

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

could we add a component method to get a pin format?

sizeof(*sink_basecfg_ext) + size);
if (!sink_basecfg_ext)
return IPC4_OUT_OF_MEMORY;

ret = comp_get_attribute(sink, COMP_ATTR_BASE_CONFIG_EXT, sink_basecfg_ext);
if (!ret) {
/*
* offset of the format for the input pin with ID 'dst_queue' within the
* base config extension.
*/
input_fmt_offset =
bu->extension.r.dst_queue * sizeof(struct ipc4_input_pin_format);
in_fmt = (struct ipc4_input_pin_format *)&sink_basecfg_ext->pin_formats[input_fmt_offset];
ibs = in_fmt->ibs;
}
rfree(sink_basecfg_ext);
}

if (source->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_LL)
buf_size = source_src_cfg.obs * 2;
else
buf_size = sink_src_cfg.ibs * 2;
/* 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;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another minor note: would be nice to split out into subfunctions somehow, this function is getting very, very long.


/* allocate buffer with size large enough to fit ibs of the sink or obs of the source */
buf_size = MAX(ibs * 2, obs * 2);
buffer = ipc4_create_buffer(source, cross_core_bind, buf_size, bu->extension.r.src_queue,
bu->extension.r.dst_queue);
if (!buffer) {
Expand All @@ -496,8 +558,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
13 changes: 13 additions & 0 deletions src/samples/audio/smart_amp_test_ipc4.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ static int smart_amp_init(struct processing_module *mod)
const size_t out_size = sizeof(struct ipc4_output_pin_format) * SMART_AMP_NUM_OUT_PINS;
int ret;
const struct ipc4_base_module_extended_cfg *base_cfg = mod_data->cfg.init_data;
size_t size;

comp_dbg(dev, "smart_amp_init()");
sad = rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM, sizeof(*sad));
Expand Down Expand Up @@ -78,6 +79,16 @@ static int smart_amp_init(struct processing_module *mod)

mod->max_sources = SMART_AMP_NUM_IN_PINS;

/* save the base config extension */
size = sizeof(struct ipc4_base_module_cfg_ext) + in_size + out_size;
mod_data->cfg.basecfg_ext = rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM, size);
if (!mod_data->cfg.basecfg_ext) {
ret = -ENOMEM;
goto sad_fail;
}

memcpy_s(mod_data->cfg.basecfg_ext, size, &base_cfg->base_cfg_ext, size);

return 0;

sad_fail:
Expand Down Expand Up @@ -163,10 +174,12 @@ static inline int smart_amp_get_config(struct processing_module *mod,
static int smart_amp_free(struct processing_module *mod)
{
struct smart_amp_data *sad = module_get_private_data(mod);
struct module_data *mod_data = &mod->priv;
struct comp_dev *dev = mod->dev;

comp_dbg(dev, "smart_amp_free()");
comp_data_blob_handler_free(sad->model_handler);
rfree(mod_data->cfg.basecfg_ext);
rfree(sad);
return 0;
}
Expand Down
Loading