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

ranj063 · 2023-12-05T16:51:10Z

Modules are bound with one another with a buffer in between. Today, this buffer's size is based on the source module's base config obs and sink module's base config ibs. But this is incorrect because this assumes that the connection is always from output pin 0 of the source module and input pin 0 of the sink module.

To determine the buffer size correctly, the buffer size should be based on the pin-based obs/ibs of the source/sink module. To allow this, save the base config extension for the module during init so that it can be retrieved during module binding to get the obs/ibs based on the source/sink pin ID.

Modify the google rtc and smart amp modules to save the base config extension during their init.

src/audio/google/google_rtc_audio_processing.c

btian1 · 2023-12-06T06:57:18Z

src/audio/google/google_rtc_audio_processing.c

@@ -392,7 +392,7 @@ 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 size = sizeof(struct ipc4_input_pin_format);


I would suggest with input_size and output_size, and use input_size and output_size related line 409
sizeof(struct ipc4_input_pin_format)
sizeof(struct ipc4_output_pin_format)

use variable will not make code lose performance( no matter you use or not, compiler will do this), it can make code more easy to read.

src/ipc/ipc4/helper.c

lyakh · 2023-12-06T09:15:32Z

src/include/sof/audio/component_ext.h

@@ -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 *base_cfg;
+	struct ipc4_base_module_cfg_ext *basecfg_ext = NULL;
+	struct ipc4_base_module_cfg *base_cfg = NULL;


you never need both pointers, right? So you could just use one, the only difference is the size, otherwise you'd reuse the same code.

two pointers does help with type correctness, in case we ever have to deref. however, I cant see from here if one struct is embedded in the other (then 1 ptr makes sense).

no they're not embedded. i will move the free's to the respective switches and remove the inits here

src/ipc/ipc4/helper.c

marcinszkudlinski · 2023-12-06T12:08:57Z

LGTM

lgirdwood

Just some minor questions from me.

lgirdwood · 2023-12-08T15:37:40Z

src/include/sof/audio/component_ext.h

@@ -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 *base_cfg;
+	struct ipc4_base_module_cfg_ext *basecfg_ext = NULL;
+	struct ipc4_base_module_cfg *base_cfg = NULL;


two pointers does help with type correctness, in case we ever have to deref. however, I cant see from here if one struct is embedded in the other (then 1 ptr makes sense).

src/ipc/ipc4/helper.c

lgirdwood · 2023-12-08T15:40:01Z

src/ipc/ipc4/helper.c

+		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,


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

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

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

Modules are bound with one another with a buffer in between. Today, this buffer's size is based on the source module's base config obs and sink module's base config ibs. But this is incorrect because this assumes that the connection is always from output pin 0 of the source module and input pin 0 of the sink module. To determine the buffer size correctly, the buffer size should be based on the pin-based obs/ibs of the source/sink module. To allow this, save the base config extension for the module during init so that it can be retrieved during module binding to get the obs/ibs based on the source/sink pin ID. Modify the google rtc and smart amp modules to save the base config extension during their init. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

udaymb · 2023-12-13T17:43:08Z

This patch is verified on Chrome rex system and is working without issue

lyakh · 2023-12-14T09:12:37Z

src/audio/google/google_rtc_audio_processing.c

+
+	/* 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;


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

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

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

lyakh · 2023-12-14T09:13:41Z

src/audio/module_adapter/module_adapter_ipc4.c

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


similar here with size

lyakh · 2023-12-14T09:15:08Z

src/include/sof/audio/component_ext.h

+			rfree(base_cfg);
+			break;
+		case COMP_ATTR_BASE_CONFIG_EXT:
+			memcpy_s(value, size, basecfg_ext, size);


here without sizeof(*basecfg_ext), right?

lyakh · 2023-12-14T09:23:54Z

src/ipc/ipc4/helper.c

+		if (!src_basecfg_ext)
+			return IPC4_OUT_OF_MEMORY;
+
+		ret = comp_get_attribute(source, COMP_ATTR_BASE_CONFIG_EXT, src_basecfg_ext);


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

lyakh · 2023-12-14T09:30:31Z

src/ipc/ipc4/helper.c

+		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,


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

kv2019i

Please check the one functional comment. Multiple minor ones, but not necessary to address in this PR.

kv2019i · 2023-12-14T16:45:11Z

src/audio/google/google_rtc_audio_processing.c

+		 &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);


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

memcpy_s(&output_fmt, out_fmt_size, , out_fmt_size);

..?

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

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

kv2019i · 2023-12-14T16:47:08Z

src/audio/google/google_rtc_audio_processing.c

+
+	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));


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

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

kv2019i · 2023-12-14T16:49:33Z

src/include/sof/audio/component_ext.h

 	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) +


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

kv2019i · 2023-12-14T17:05:36Z

src/ipc/ipc4/helper.c

+		}
+
+		ibs = sink_src_cfg.ibs;
+	}


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

marcinszkudlinski

Changes verified om 007, working very well, please proceed with merging

lgirdwood · 2023-12-21T16:01:20Z

Changes verified om 007, working very well, please proceed with merging

Thanks @marcinszkudlinski for validating !

@ranj063 fyi, some conflicts.

andyross · 2024-01-03T02:47:12Z

It's probably way too disruptive for me to -1 this given that it's already been pushed into mtl-007-drop-stable, but see #8685 for a different take on the same problem that IMHO is a lot cleaner long-term.

Basically: I... kinda really, really hate the API picked here: Module code (literally every module that might in principle be connected to more than two other components) needs to make a copy of raw IPC memory that it finds in the module_config, set it back on a different field in module_config (for the benefit of ibs/obs buffer sizing, which it can't control), and then set that metadata back onto the connected buffers at prepare time. Literally none of those steps should be the responsibility of module code.

Module code is written by dumb third party people like me who don't understand the module_adapter internals!

andyross

Actually I did find a spot with an actual bug, so -1. But not because of the dueling solutions, I swear. :)

I actually hit coherency issues in my version, which does basically the same copy but at a different place with different timing. It's presumably benign here, but these bugs can lurk for a long time before becoming traps.

FWIW, just to pontificate a little: SOF's cached-by-default heap really seems likely to be a reliability problem as more stuff gets moved out into DP-world. It might be worth thinking about e.g. changing "MEM_ZONE_RUNTIME" to be uncached and then auditing components to see (1) which ones are using heap blocks inside their copy/process routines and (2) decide on strategies for making those usages coherence-safe.

andyross · 2024-01-03T18:07:07Z

src/audio/google/google_rtc_audio_processing.c

+		base_cfg->base_cfg_ext.nb_output_pins * out_fmt_size;
+
+	md->cfg.basecfg_ext = rzalloc(SOF_MEM_ZONE_RUNTIME, 0, SOF_MEM_CAPS_RAM,
+				      sizeof(struct ipc4_base_module_cfg_ext) + size);


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.

@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

ranj063 · 2024-01-10T15:56:39Z

replaced by #8685

ranj063 requested review from yongzhi1 and marcinszkudlinski December 5, 2023 16:51

ranj063 force-pushed the fix/get_base_cfg_extn branch 2 times, most recently from 2ed73f7 to 0a7cd2d Compare December 5, 2023 17:06

yongzhi1 reviewed Dec 5, 2023

View reviewed changes

src/audio/google/google_rtc_audio_processing.c Outdated Show resolved Hide resolved

ranj063 force-pushed the fix/get_base_cfg_extn branch 2 times, most recently from 589eafe to 6b6c154 Compare December 5, 2023 22:32

ranj063 marked this pull request as ready for review December 6, 2023 02:02

ranj063 requested review from a team, jxstelter, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners December 6, 2023 02:02

btian1 reviewed Dec 6, 2023

View reviewed changes

lyakh reviewed Dec 6, 2023

View reviewed changes

lgirdwood reviewed Dec 8, 2023

View reviewed changes

ranj063 force-pushed the fix/get_base_cfg_extn branch from 6b6c154 to 09ace5c Compare December 13, 2023 17:38

ranj063 requested review from lyakh, btian1 and lgirdwood December 13, 2023 17:38

lyakh reviewed Dec 14, 2023

View reviewed changes

kv2019i reviewed Dec 14, 2023

View reviewed changes

marcinszkudlinski mentioned this pull request Dec 15, 2023

[DNM] Google aec sink src dp #8469

Closed

7 tasks

marcinszkudlinski approved these changes Dec 21, 2023

View reviewed changes

marcinszkudlinski mentioned this pull request Dec 21, 2023

dp: look at all sinks and sources in default "is ready" #8668

Merged

2 tasks

This was referenced Jan 2, 2024

[BUG] IPC4 fails to set up stream metadata correctly #8639

Open

audio_stream: Fix uninitialized data in alignment_constants #8672

Merged

andyross mentioned this pull request Jan 3, 2024

Better handling of module base_config_ext data #8685

Merged

andyross requested changes Jan 3, 2024

View reviewed changes

ranj063 closed this Jan 10, 2024

ranj063 deleted the fix/get_base_cfg_extn branch January 10, 2024 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ranj063 commented Dec 5, 2023

btian1 Dec 6, 2023

lyakh Dec 6, 2023

lgirdwood Dec 8, 2023

ranj063 Dec 13, 2023

marcinszkudlinski commented Dec 6, 2023

lgirdwood left a comment

lgirdwood Dec 8, 2023

lgirdwood Dec 8, 2023

ranj063 Dec 13, 2023

lyakh Dec 14, 2023

udaymb commented Dec 13, 2023

lyakh Dec 14, 2023

btian1 Dec 15, 2023

lyakh Dec 15, 2023

lyakh Dec 14, 2023

lyakh Dec 14, 2023

lyakh Dec 14, 2023

lyakh Dec 14, 2023

kv2019i left a comment

kv2019i Dec 14, 2023

btian1 Dec 15, 2023

lgirdwood Dec 18, 2023

kv2019i Dec 14, 2023

btian1 Dec 15, 2023

kv2019i Dec 14, 2023

kv2019i Dec 14, 2023

marcinszkudlinski left a comment

lgirdwood commented Dec 21, 2023

andyross commented Jan 3, 2024

andyross left a comment

andyross Jan 3, 2024

marcinszkudlinski Jan 9, 2024 •

edited

Loading

ranj063 commented Jan 10, 2024

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

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

Conversation

ranj063 commented Dec 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcinszkudlinski commented Dec 6, 2023

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

udaymb commented Dec 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcinszkudlinski left a comment

Choose a reason for hiding this comment

lgirdwood commented Dec 21, 2023

andyross commented Jan 3, 2024

andyross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcinszkudlinski Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

ranj063 commented Jan 10, 2024

marcinszkudlinski Jan 9, 2024 •

edited

Loading