Better handling of module base_config_ext data #8685

andyross · 2024-01-03T02:36:10Z

This is a different take on a fix for #8639 than the workaround in #8575 on which it was originally coded. It removes the requirement for components to have to muck with module internals[1] and places the complicated byte-offset math into just one place (in the module code that owns the struct format). Really I think this is a lot cleaner. If you guys want to merge the first version for clarity (given it's already "shipped"[2]), I can submit this as cleanup on top of that instead (which, again, is how this was written originally anyway).

With this, the big IPC4 workaround patch at the end of the AEC work is down to just one line, hope to have that cleaned up by tomorrow. (see https://github.com/andyross/sof/commits/mtl-aec-tmp/ for the current status of my working tree).

[1] Basically, the first version: (1) requires every module to make a copy of the untyped "init_data" field of module_config, then (2) store that pointer back into a different field of module_config (because the first one points into the IPC command buffer and won't survive) where (3) it will be eventually used, not by the module code itself but by the ipc4/module layer that created it in the first place. This avoids all that by having the module_adapter init code just parse and store the data at the start.

[2] In the MTL "stable" branch, where it's already been merged. Can I just whine a bit about how frustrating it is to have to work across branches like this? I didn't realize until I was about to submit that the code I was modifying wasn't actually upstream!

andyross · 2024-01-03T02:55:03Z

I didn't realize until I was about to submit that the code I was modifying wasn't actually upstream!

As in, I wrote up a whole commit message carefully explaining and advocating my removal of an API that never merged, heh:

andyross@37f784f

src/audio/module_adapter/module_adapter.c

src/audio/module_adapter/module_adapter_ipc4.c

src/ipc/ipc4/helper.c

kv2019i

This does look cleaner. I'll let others chime in. Ext-cfg is optional, but given it's more common to have it, don't see why not to handle it in common code. There are some corner cases I'd like others to chime in.

kv2019i · 2024-01-03T09:47:09Z

src/audio/module_adapter/module_adapter_ipc4.c

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


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

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.

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.

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.

src/audio/module_adapter/module_adapter_ipc4.c

kv2019i · 2024-01-03T10:08:02Z

src/ipc/ipc4/helper.c

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


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".

@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.

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.

@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.

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.

andyross · 2024-01-03T21:57:11Z

Cleaned up the backport from mtl-007-drop-stable (the only place I can test this) that rebases this on top of a reversion that should match main better. Hopefully will fix the build issues.

marcinszkudlinski

In general LGTM, just minor comment

marcinszkudlinski · 2024-01-04T10:05:22Z

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;


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)

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.

src/ipc/ipc4/helper.c

src/audio/module_adapter/module_adapter_ipc4.c

kv2019i

CI looks good now. By directly accessing module internals is a change in convention, but we seem to have consensus this is ok in this case.

src/audio/module_adapter/module_adapter.c

src/audio/module_adapter/module_adapter_ipc4.c

andyross · 2024-01-05T19:02:14Z

Fixed Grrr... sorry. Came back to check CI and realized that I'd mis-squashed the naming fixup and put it in the wrong patch. I've already torn my tree up for different work though so can't get back to fix it in the immediate term. (I'm juggling like six PRs right now, heh). Don't merge until I get the corrected version in

lyakh · 2024-01-08T09:23:05Z

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;


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..."

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. :)

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 <[email protected]>

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 <[email protected]>

kv2019i · 2024-01-09T13:12:40Z

@marcinszkudlinski ?

lgirdwood

My changes can be fixed incrementally, I'd rather get this merged and tested more as priority.

lgirdwood · 2024-01-09T13:48:05Z

src/audio/module_adapter/module_adapter_ipc4.c

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


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

lgirdwood · 2024-01-09T13:50:04Z

src/audio/module_adapter/module_adapter_ipc4.c

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


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.

lgirdwood

My changes can be fixed incrementally, I'd rather get this merged and tested more as priority.

marc-hb · 2024-01-25T20:49:40Z

Fix c3da4e3 for double-free regression was just merged in #8790

andyross requested review from ranj063, jxstelter, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners January 3, 2024 02:36

andyross requested review from cujomalainey and johnylin76 January 3, 2024 02:36

This was referenced Jan 3, 2024

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

Open

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

Closed

btian1 reviewed Jan 3, 2024

View reviewed changes

kv2019i requested a review from marcinszkudlinski January 3, 2024 09:00

kv2019i reviewed Jan 3, 2024

View reviewed changes

andyross force-pushed the module-ext-cfg branch from 595031c to 8331325 Compare January 3, 2024 21:56

andyross force-pushed the module-ext-cfg branch 2 times, most recently from d1e425c to 8672299 Compare January 3, 2024 23:07

marcinszkudlinski reviewed Jan 4, 2024

View reviewed changes

marcinszkudlinski mentioned this pull request Jan 4, 2024

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

Merged

2 tasks

ranj063 reviewed Jan 4, 2024

View reviewed changes

src/ipc/ipc4/helper.c Outdated Show resolved Hide resolved

cujomalainey reviewed Jan 4, 2024

View reviewed changes

src/audio/module_adapter/module_adapter_ipc4.c Outdated Show resolved Hide resolved

kv2019i approved these changes Jan 5, 2024

View reviewed changes

src/audio/module_adapter/module_adapter.c Show resolved Hide resolved

src/audio/module_adapter/module_adapter_ipc4.c Outdated Show resolved Hide resolved

andyross force-pushed the module-ext-cfg branch from 8672299 to a17c4e1 Compare January 5, 2024 18:06

andyross force-pushed the module-ext-cfg branch from a17c4e1 to e0f068e Compare January 5, 2024 23:37

lyakh reviewed Jan 8, 2024

View reviewed changes

andyross force-pushed the module-ext-cfg branch from e0f068e to 877ef51 Compare January 8, 2024 23:49

ranj063 approved these changes Jan 9, 2024

View reviewed changes

jxstelter approved these changes Jan 9, 2024

View reviewed changes

lgirdwood approved these changes Jan 9, 2024

View reviewed changes

marcinszkudlinski approved these changes Jan 9, 2024

View reviewed changes

lgirdwood merged commit 4bafdee into thesofproject:main Jan 9, 2024
43 of 44 checks passed

marcinszkudlinski mentioned this pull request Jan 10, 2024

Google aec sink src dp #8722

Merged

marcinszkudlinski mentioned this pull request Feb 7, 2024

ipc4: fix a wrong variable #8838

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better handling of module base_config_ext data #8685

Better handling of module base_config_ext data #8685

andyross commented Jan 3, 2024

andyross commented Jan 3, 2024

kv2019i left a comment

kv2019i Jan 3, 2024

kv2019i Jan 3, 2024

andyross Jan 3, 2024

lgirdwood Jan 9, 2024 •

edited by andyross

Loading

kv2019i Jan 3, 2024

ranj063 Jan 3, 2024

andyross Jan 3, 2024

ranj063 Jan 4, 2024 •

edited

Loading

andyross Jan 5, 2024

andyross commented Jan 3, 2024

marcinszkudlinski left a comment

marcinszkudlinski Jan 4, 2024

andyross Jan 5, 2024

kv2019i left a comment

andyross commented Jan 5, 2024 •

edited

Loading

lyakh Jan 8, 2024

andyross Jan 8, 2024

kv2019i commented Jan 9, 2024

lgirdwood left a comment

lgirdwood Jan 9, 2024

lgirdwood Jan 9, 2024 •

edited by andyross

Loading

lgirdwood left a comment

marc-hb commented Jan 25, 2024

Better handling of module base_config_ext data #8685

Better handling of module base_config_ext data #8685

Conversation

andyross commented Jan 3, 2024

andyross commented Jan 3, 2024

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

lgirdwood Jan 9, 2024 • edited by andyross Loading

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

ranj063 Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyross commented Jan 3, 2024

marcinszkudlinski left a 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

andyross commented Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i commented Jan 9, 2024

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood Jan 9, 2024 • edited by andyross Loading

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

marc-hb commented Jan 25, 2024

lgirdwood Jan 9, 2024 •

edited by andyross

Loading

ranj063 Jan 4, 2024 •

edited

Loading

andyross commented Jan 5, 2024 •

edited

Loading

lgirdwood Jan 9, 2024 •

edited by andyross

Loading