Add support for dynamic loading of module libraries for IPC4 #3720

ranj063 · 2022-06-25T19:14:47Z

No description provided.

ranj063 · 2022-06-25T19:30:08Z

@aiChaoSONG @libinyang FYI. Please note that the module libraries are expected to be in the same folder as the topology folder with this PR

aiChaoSONG · 2022-06-27T01:59:29Z

sound/soc/sof/ipc4-loader.c

For basefw: man4_module_entry.id = 0...num_fw_modules_in_base_fw-1,
for the first library loaded: man4_module_entry.id = 0x1000 + 0...num_fw_modules_in_the_library-1
for the second library loaded: man4_module_entry.id = 0x2000 + 0...num_fw_modules_in_the_library-1
...
@libinyang found this from pytest. module_id = 0x1000 * libid + module_index_in_the_lib

I think you are merging modules from all libraries, this is easy to implement, but I am thinking something like this, I think it is module-unload friendly.

@aiChaoSONG I'd argue there is value in keeping the modules separate per library. When would we have a library that contains more than one module? If we're tying the widget UUID to the library name, this can never really work

I did some test for you reference. There are two modules for some modules

rander@cml-s:/media/xxx/adsp_fw_parser$ ./adsp_fw_parser ~/fw_libs/tgl_maxx_audio_pro.bin | grep uuid "uuid": "52983D04-2414-88B4-A2A2-C1397E13B022", "uuid": "FAACC8CC-B365-4964-B4B8-BD4DEB18D922", rander@cml-s:/media/xxx/adsp_fw_parser$ ./adsp_fw_parser ~/fw_libs/cml_maxx_audio_pro.bin | grep uuid "uuid": "52983D04-2414-88B4-A2A2-C1397E13B022", "uuid": "FAACC8CC-B365-4964-B4B8-BD4DEB18D922",

I don't think this can work in a Linux context, the topology will make references to the UUID only, so how would we know how to get the library tgl_maxx_audio_pro.bin ?

I guess we could check if a library contains another module and not try to request the firmware for that additional UUID, but that's over-engineered and borderline nonsensical.

I think you are merging modules from all libraries, this is easy to implement, but I am thinking something like this, I think it is module-unload friendly.

module unload is not supported to the best of my knowledge @aiChaoSONG?

That's why it's important to have a one page design document btw...we could talk for hours on what 'loadable' means.

On option might be (and probably ugly) is to use symlinks when a binary file contains multiple modules with different UUID?

ls -al 52983D04-2414-88B4-A2A2-C1397E13B022.bin -> tgl_maxx_audio_pro.bin FAACC8CC-B365-4964-B4B8-BD4DEB18D922.bin -> tgl_maxx_audio_pro.bin tgl_maxx_audio_pro.bin

and load only the requested module from the loaded firmware file?

or we load all modules form the file for simplicity.

we can have a config file or environment variable (different platform may has different library) to set library path and load these libraries at first boot up. On windows platform, there is a library path defined in registry and driver loads all libraries in boot-up stage

plbossart

Looks like a promising start @ranj063

I suggest we completely remove the notion of '3rd party'. It's the usage in practice but that's not a design requirement. We could perfectly support open-source modules that are not in the base firmware. The fact that they are 3rd party is out of scope/irrelevant.

Also there are a couple of error flow handling issues. Not sure what happens if loading a library is not supported but the topology mentions such libraries.

sound/soc/sof/ipc4-priv.h

sound/soc/sof/sof-priv.h

sound/soc/sof/ipc4-loader.c

include/sound/sof/ipc4/header.h

sound/soc/sof/intel/hda-loader.c

plbossart · 2022-06-27T15:50:47Z

sound/soc/sof/ipc4-loader.c

Humm, I don't get how this works. Is this not an error if the load_library is not supported at the chip level but explicitly referenced in the topology?

Better to fail than keep going IMHO.

@plbossart IPC3 has no module library support. If I return an error here, I'd be forced to add a stub in the IPC3 ops.

We may be talking on different levels here.

My first point what that we could test if (sof_ops(sdev->load_library)) earlier and avoid requesting stuff that we know will not be supported, i.e. move the test to line 287.

The second point is what to do if the topology requests something that cannot be supported. That's an error, no?

@plbossart IPC3 has no module library support. If I return an error here, I'd be forced to add a stub in the IPC3 ops.

sof_ipc4_load_library will never be called by IPC3, right?

ignore my comment please. I confused the comment to be for the load_library call in topology.c

sound/soc/sof/ipc4-loader.c

sound/soc/sof/loader.c

sound/soc/sof/sof-priv.h

sound/soc/sof/ipc4-loader.c

ujfalusi · 2022-06-28T13:14:44Z

sound/soc/sof/ipc4-loader.c

On option might be (and probably ugly) is to use symlinks when a binary file contains multiple modules with different UUID?

ls -al 52983D04-2414-88B4-A2A2-C1397E13B022.bin -> tgl_maxx_audio_pro.bin FAACC8CC-B365-4964-B4B8-BD4DEB18D922.bin -> tgl_maxx_audio_pro.bin tgl_maxx_audio_pro.bin

and load only the requested module from the loaded firmware file?

ujfalusi · 2022-06-28T13:21:24Z

sound/soc/sof/intel/hda-loader.c

the reply is only going to be received when the module is loaded by the DMA or they go in parallel? Is there a race here? If the module is 'big' we might have a timeout or we stop the channel before the whole module is sent down?

@ujfalusi I'd assume the reply would be recieved after the module library is loaded. And if the module is big and takes too long and we don't receive the reply, the time will timeout.

ujfalusi · 2022-06-28T13:25:40Z

sound/soc/sof/ipc4-loader.c

no_parse = false -> parse, I would reverse this flag.

ujfalusi · 2022-06-28T13:41:03Z

sound/soc/sof/ipc4-loader.c

or we load all modules form the file for simplicity.

sound/soc/sof/topology.c

ujfalusi · 2022-06-28T13:52:59Z

sound/soc/sof/intel/hda-loader.c

Can this be done in higher level?
Adding a new flag hda flag to indicate that we have cold booted (or imr booted) and skip the sof_ops(sdev)->load_library in Intel arch code if there is no need for the reload?

load_library happens during topology parsing and it will be not done during resume. So we need an explicit way to do this either during resume or here inpost_fw_run

The callback should be validated, this will Oops with IPC3 after a firmware crash or when waking up from hibernation, no?

bardliao · 2022-06-29T06:00:16Z

sound/soc/sof/intel/hda-loader.c

Should we move those IPC4 stuff to a new helper function in ipc4.c?

do you mean the entire load_library function? and this is HDA specific no? how can I move to the common code outside the intel directory?

This is highly HDA specific message and sequence, let's add a comment and think about the abstraction if the need ever arise

Set the FW state to complete right after boot is complete. This enables sending IPC's in the post_fw_run op. This will be needed to support reloading 3rd party module libraries after firmware boot. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

sound/soc/sof/ipc4-loader.c

plbossart

This is a significant piece of code @ranj063, I added a bunch of comments to help with code clarity - I can't pretend I understood all the sequences.

sound/soc/sof/loader.c

sound/soc/sof/ipc4-loader.c

sound/soc/sof/ipc3-loader.c

sound/soc/sof/ipc4-loader.c

sound/soc/sof/sof-priv.h

plbossart · 2022-07-21T20:27:07Z

sound/soc/sof/ipc4-loader.c

mean FW cannot support any more libraries. Would you prefer EINVAL?

sound/soc/sof/ipc4-loader.c

sound/soc/sof/topology.c

sound/soc/sof/ipc4-loader.c

Introduce a new structure snd_sof_module_library_info to save 3rd party module library info. Add two new fields to struct sof_ipc4_fw_data, max_fw_libs and module_lib_info. max_fw_libs is populated when querying the FW configuration and we allocate memory for module_lib_info based on max_fw_libs. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Modify the signature of the parse_ext_manifest op in struct sof_ipc_fw_loader_ops to pass the pointer to the firmware along with the firmware library index. This is needed to reuse the same op for parsing the extended manifest for module firmware libraries. Along with the above change, also split the parse_ext_manifest for IPC4 into 2 parts. During firmware boot, we only need the firmware offset. So the first call to parse_ext_manifest returns without parsing the module information. Once we've queried the maximum number of libraries supported by the FW after successful boot, we use that information to allocate memory for as many modules as those in the base FW and the max 3rd party modules supported by the firmware. As the manifest is parsed, the module UUID's are saved to the new field, module_uuids in struct sof_ipc4_fw_data. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Add a new op in struct snd_sof_dsp_ops to load module libraries and define the op for IPC4. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Set the load_library op for MTL. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Set the load_library DSP op for APL. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Set the load_library DSP op for TGL and IPC4. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Set the load_library op for CNL and IPC4. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Add a new load_library op to struct sof_ipc_fw_loader_ops to support loading 3rd party module libraries. This op is called during widget parsing to load the module firmware library. Also define and set the load_library op for IPC4 fw_loader ops. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Add a new op to struct sof_ipc_fw_loader_ops, reload_libraries, to support reloading the libraries. For HDA platforms, this is called during the post_fw_run op after the base FW is cold booted with the exception of the first boot. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

plbossart

I only have mostly nit-pick comments, but the main question I would have is

"how was this tested"?

we need to have a sof-test or topology that will start checking this code.

plbossart · 2022-07-22T16:38:36Z

sound/soc/sof/sof-priv.h

+	const struct firmware *fw;
+	size_t fw_offset;
+	char *name;
+	u32 id;


I am so predictable I know: what does this 'id' represent? Who allocates it and why is it needed since we have UUIDs for libraries.

It is needed by the firmware and it must be unique number starting from 1.

You missed my point @ujfalusi : We use 'id' everywhere, it's just awful. Can we find a better name that represents the function of that variable?

We can use 'index' instead or 'lib_id' as the id number is for the library itself.

plbossart · 2022-07-22T16:39:48Z

sound/soc/sof/intel/hda-loader.c

no idea, a comment wouldn't hurt :-)

plbossart · 2022-07-22T16:47:20Z

sound/soc/sof/topology.c

+
+	/* load module library if needed */
+	if (sdev->ipc->ops->fw_loader->load_library)
+		return sdev->ipc->ops->fw_loader->load_library(sdev, swidget);


I still find it confusing that we have both

sof_ops(sdev)->load_library
sdev->ipc->ops->fw_loader->load_library(sdev, swidget);

Given 2 choices, half of engineers will get it 'wrong'

And the error handling is incorrect as well, this section should be like this:

/* load module library if needed */ if (sdev->ipc->ops->fw_loader->load_library) { ret = sdev->ipc->ops->fw_loader->load_library(sdev, swidget); if (ret) { dev_err(scomp->dev, "library loading for widget %s failed\n", swidget->widget->name); kfree(swidget->private); kfree(swidget->tuples); kfree(swidget); return ret; } } w->dobj.private = swidget; list_add(&swidget->list, &sdev->widget_list);

ranj063 · 2022-07-22T21:16:10Z

I only have mostly nit-pick comments, but the main question I would have is

"how was this tested"?

we need to have a sof-test or topology that will start checking this code.

@plbossart I tested this by loading the wov module library and by loading I mean just simply test the loading of the library with the LOAD_LIBRARY IPC and also reloading with suspend/resume. But it has been been used by @libinyang and @aiChaoSONG for their smartamp and wov implementations. But I agree that maybe we should add a test topology to test the feature

ujfalusi

I need to go through it again, but I have some layering concerns and logic change ideas.

ujfalusi · 2022-08-12T10:16:14Z

sound/soc/sof/sof-priv.h

+/**
+ * struct snd_sof_module_library_info: 3rd party module library information
+ * @fw: Pointer to the 3rd party module firmware
+ * @fw_offset: Offset of the firmware within the firmware file


Offset of the module, modules?

ujfalusi · 2022-08-12T10:29:05Z

sound/soc/sof/sof-priv.h


+/**
+ * struct snd_sof_module_library_info: 3rd party module library information
+ * @fw: Pointer to the 3rd party module firmware


firmware pointer of the external library?

ujfalusi · 2022-08-12T10:29:30Z

sound/soc/sof/sof-priv.h

+ * struct snd_sof_module_library_info: 3rd party module library information
+ * @fw: Pointer to the 3rd party module firmware
+ * @fw_offset: Offset of the firmware within the firmware file
+ * @name: 3rd party module firmware name


name of the external library?

ujfalusi · 2022-08-12T10:31:35Z

sound/soc/sof/sof-priv.h

+	const struct firmware *fw;
+	size_t fw_offset;
+	char *name;
+	u32 id;


We can use 'index' instead or 'lib_id' as the id number is for the library itself.

ujfalusi · 2022-08-12T11:23:27Z

sound/soc/sof/sof-priv.h

 	int (*validate)(struct snd_sof_dev *sdev);
-	size_t (*parse_ext_manifest)(struct snd_sof_dev *sdev);
+	size_t (*parse_ext_manifest)(struct snd_sof_dev *sdev, const struct firmware *fw,
+				     u32 lib_index, u32 flags);


The lib_index and the flags are only for IPC4 internal use and should not be exposed to upper, generic layer.
ipc_ops->fw_loader->parse_ext_manifest() is only called with a static set of parameters, never with lib_index != 0 nor without the SOF_FW_PARSE_MANIFEST_PRE_BOOT flag.

ujfalusi · 2022-08-12T11:32:54Z

sound/soc/sof/intel/hda-loader.c

This is highly HDA specific message and sequence, let's add a comment and think about the abstraction if the need ever arise

ujfalusi · 2022-08-12T11:38:54Z

sound/soc/sof/ipc4-loader.c

+	if (sof_ops(sdev)->load_library) {
+		dev_err(sdev->dev, "Loading module libraries not supported\n");
+		return -EINVAL;
+	}


I would do this test in the ipc.c, if the fw_loader->load_library is set then check the sof_ops one as well and fail early?

ujfalusi · 2022-08-12T11:39:50Z

sound/soc/sof/ipc4-loader.c

+	int ret, i;
+
+	/* nothing to do if widget UUID is not set */
+	if (!guid_is_null(&swidget->uuid))


The other way around?

if (guid_is_null(&swidget->uuid)) return 0;

and check against the referenced widget_uuid right away.

ujfalusi · 2022-08-12T11:53:08Z

sound/soc/sof/intel/hda-loader.c

The callback should be validated, this will Oops with IPC3 after a firmware crash or when waking up from hibernation, no?

ujfalusi · 2022-08-12T11:55:40Z

sound/soc/sof/ipc4-loader.c

+	if (sof_ops(sdev)->load_library) {
+		dev_err(sdev->dev, "Loading module libraries not supported\n");
+		return -EINVAL;
+	}


Same here, I would do the validation during probe: if fw_loader reload or load is supported, then sof_ops load_library must be.
Also, both reload_libraries and load_library of fw_loader must be present if I'm not mistaken.

ujfalusi · 2022-08-15T09:23:46Z

sound/soc/sof/ipc4-loader.c

+		return 0;
+
+	/* check if the platform supports load_library */
+	if (sof_ops(sdev)->load_library) {


Hrm, how this could work? The check is reversed, it should be if (!sof_ops(sdev)->load_library)

plbossart · 2022-09-12T18:37:32Z

replaced by #3826

ranj063 requested review from RanderWang, ajitkupandey, bardliao, dbaluta, juimonen, kuanhsuncheng, kv2019i, lgirdwood, libinyang, lyakh, plbossart, singalsu, ujfalusi and yaochunhung as code owners June 25, 2022 19:14

ranj063 force-pushed the topic/ipc4_lib_load branch 3 times, most recently from 00e61b5 to 400e3bc Compare June 26, 2022 17:55

aiChaoSONG reviewed Jun 27, 2022

View reviewed changes

plbossart requested changes Jun 27, 2022

View reviewed changes

ujfalusi requested changes Jun 28, 2022

View reviewed changes

bardliao reviewed Jun 29, 2022

View reviewed changes

ujfalusi self-requested a review July 15, 2022 05:17

ranj063 requested review from aiChaoSONG, bardliao and plbossart July 20, 2022 22:00

ranj063 force-pushed the topic/ipc4_lib_load branch from 400e3bc to 9592a6b Compare July 20, 2022 22:34

RanderWang reviewed Jul 21, 2022

View reviewed changes

sound/soc/sof/ipc4-loader.c Outdated Show resolved Hide resolved

ranj063 force-pushed the topic/ipc4_lib_load branch from 9592a6b to 42b9f60 Compare July 21, 2022 13:55

plbossart requested changes Jul 21, 2022

View reviewed changes

ranj063 requested review from RanderWang and plbossart July 21, 2022 23:20

ranj063 added 9 commits July 21, 2022 16:56

ASoC: SOF: Add new DSP op for module library loading

775cb46

Add a new op in struct snd_sof_dsp_ops to load module libraries and define the op for IPC4. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ASoC: SOF: Intel: MTL: set the load_library op

9ff992b

Set the load_library op for MTL. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ASoC: SOF: Intel: APL: set the load_library op

4bbbe9a

Set the load_library DSP op for APL. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ASoC: SOF: Intel: TGL: set the load_library op

9d62fff

Set the load_library DSP op for TGL and IPC4. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ASoC: SOF: Intel: CNL: set the load_library op

8348e34

Set the load_library op for CNL and IPC4. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the topic/ipc4_lib_load branch from 42b9f60 to fc391aa Compare July 21, 2022 23:58

ranj063 changed the title ~~Add support for dynamic loading of 3rd party module libraries for IPC4~~ Add support for dynamic loading of module libraries for IPC4 Jul 21, 2022

plbossart reviewed Jul 22, 2022

View reviewed changes

ujfalusi requested changes Aug 12, 2022

View reviewed changes

ujfalusi reviewed Aug 15, 2022

View reviewed changes

ujfalusi mentioned this pull request Aug 18, 2022

ASoC: SOF: ipc4/Intel: Support for loading external libraries #3826

Merged

plbossart closed this Sep 12, 2022

ranj063 deleted the topic/ipc4_lib_load branch September 29, 2022 20:26

Add support for dynamic loading of module libraries for IPC4 #3720

Add support for dynamic loading of module libraries for IPC4 #3720

Uh oh!

Conversation

ranj063 commented Jun 25, 2022

Uh oh!

ranj063 commented Jun 25, 2022

Uh oh!

aiChaoSONG Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ujfalusi Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aiChaoSONG Jun 27, 2022 •

edited

Loading

ujfalusi Jun 28, 2022 •

edited

Loading