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

ujfalusi · 2022-08-18T10:28:40Z

Hi,

In IPC4 all DSP loadable executable is a 'library' containing modules. The main or basefw is also a library which contains multiple modules.
IPC4 allows to use loadable libraries to extend the functionality of the booted basefw.

This series adds support for loading external libraries in case they are needed by the loaded topology file.

The libraries must be placed to a specific firmware directory (fw_lib_prefix), which is:
intel/avs-lib|sof-ipc4-lib/ followed by the platform name and in case of community key use a 'community' directory.
For example for upx-i11 (community key): intel/avs-lib/tgl/community is the default path.

The name of the library should be the UUID of the module it contains since the library loading is going to look for the file as <module_UUID>.bin
In case there is a need to bundle multiple modules into single library, symlinks can be used to point to the file:

module_boundle.bin
<UUID1>.bin -> module_boundle.bin
<UUID2>.bin -> module_boundle.bin
<UUID3>.bin -> module_boundle.bin

But note that in this case all modules will be loaded to the DSP since only the whole library can be loaded, not individual modules.

This PR replaces #3720 from @ranj063 and it is taking completely different route to implement the support, keeping most of the implementation IPC4 specific and not forcing artificial rules to non IPC4 users.

plbossart

Looks mostly good @ujfalusi, I added a number of nit-picks comments. My main concern is the IMR boot, this doesn't seem to be aligned with fixes we added for the base firmware for crashes and hibernate.

plbossart · 2022-08-18T11:54:12Z

sound/soc/sof/ipc4-priv.h

 	void *nhlt;
 	enum sof_ipc4_mtrace_type mtrace_type;
 	u32 mtrace_log_bytes;
+	u32 max_fw_libs;


naming nit-pick: max_libs_count would be clearer IMHO, with a 1:1 mapping with the query

OK.
It is unused atm, we might as well want to use it when loading libraries to make sure we are not trying to load more than what is supported.

plbossart · 2022-08-18T11:57:35Z

sound/soc/sof/loader.c

 			fw_filename, ret);
 	}

+	/* Platform code is still using the plat_data for firmware handling */


it seems that it's a temporary comment, until the platform code is modified? it should be made clearer for reviewers who scan patches one after the other.

plbossart · 2022-08-18T12:03:41Z

sound/soc/sof/sof-priv.h

 * @fw_tracing:	Pointer to Firmware tracing ops
 *
+ * @init:	Optional pointer for IPC related initialization
+ * @fini:	Optional pointer for IPC related cleanup


I personally have have no idea what 'fini' would stand for. We have no precedent for using 'fini'. can we find a more usual name for this callback?

If we use the ASoC convention, we have init/exit.

Yeah, it is coming from DRM, but sure, let's use exit

plbossart · 2022-08-18T12:07:45Z

sound/soc/sof/ipc4-loader.c

+	payload_offset = sof_ipc4_fw_parse_ext_man(sdev, fw_lib);
+	if (payload_offset > 0) {
+		fw_lib->sof_fw.payload_offset = payload_offset;
+		id = idr_alloc(&ipc4_data->fw_lib_idr, fw_lib, 0, 0, GFP_KERNEL);


In the kernel documentation https://www.kernel.org/doc/html/latest/core-api/idr.html#:~:text=The%20IDR%20provides%20the%20ability,is%20much%20more%20memory%2Defficient. I see this:

The IDR interface is deprecated; please use the XArray instead.

??

Oh, that's unfortunate. The IDR API is straight forward with lots of users... I'll study XArray, it would be a shame if I would need to do this by hand (list+bitfiled+lookups).

plbossart · 2022-08-18T12:10:20Z

sound/soc/sof/ipc4-priv.h


+/**
+ * struct sof_ipc4_fw_module - IPC4 module info
+ * @sof_man4_module : Module info


spurious space after _module

plbossart · 2022-08-18T12:22:17Z

sound/soc/sof/intel/pci-skl.c

 	},
+	.default_lib_path = {
+		[SOF_INTEL_IPC4] = "intel/sof-lib/kbl",
+	},


wondering if the library loading is the same for SKL/KBL, it's not the same loader at all. Maybe we should put this on the back-burner for now, it's not clear how we would test this and it's not really a priority, is it?

OK, I'll drop this and the setting of the load_library callback for SKL.

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

plbossart · 2022-08-18T12:29:58Z

sound/soc/sof/sof-priv.h

 *			configuration from the booted firmware.
 *			Executed after the first successful firmware boot.
+ * @reload_fw_addons:	Optional function pointer to reload firmware components
+ *			after the base firmware has been booted up


naming nit-pick: why are we talking about 'reloading", that would mean we've loaded those component already. Did you mean 'load_fw_addons'?

and I also don't get why we can't be explicit about libraries, I don't know what 'addons' might mean, that could mean very different things to different people.

And we also need to be clear that there is no 'unload'

Right, my idea with this was to give a point where IPC dependent post firmware boot loading, patching can be executed. Be it libraries, some blobs, tuning values, whatever.
Basically a fixed point right after firmware boot to download (if needed) anything that the firmware might need prior to continuing to configure and run.

I have chosen reload, but perhaps restore might be better? If it is load_fw_addons then this callback must be also called after the first boot, right? Without condition, after query_fw_configuration()

@ujfalusi Isn't this quite close to existing "post_fw_run()" at DSP ops level? Maybe just have same for ipc_fw_loader ops (i.e. define post_fw_run also for fw_loaded_ops...?

@kv2019i, @plbossart, yes, in a way it is pretty close to the platform's post_fw_run() with a distinct difference:
the post_fw_run() is for IPC agnostic, hardware, platform related things to do after the fw has booted.

One way we could do this I think is to:
Add generic, IPC level callback: post_fw_run() to struct sof_ipc_ops
Call that after the platform's post_fw_run() without condition (if set)
In IPC4, if it is the first run, do what we do in query_fw_configuration() otherwise reload libraries
remove the query_fw_configuration() from sof_ipc_fw_loader_ops and from IPC4.

This will somehow makes things better balanced and generic enough to be usable for purposes we are not yet seeing.

right, so it is really need to be in the fw_loader_ops. I'll do that.

plbossart · 2022-08-18T12:31:50Z

sound/soc/sof/ipc4-loader.c

+
+	/* Fix up the module ID numbers within the library */
+	for (i = 0; i < fw_lib->num_modules; i++)
+		fw_lib->modules[i].man4_module_entry.id += (SOF_IPC4_MODULE_ID_STRIDE * id);


I could not follow what this is doing? Is this a firmware requirement, or some of sort of trick?

Modules must have unique id numbers. Afaik there is no hard definition on the stride, 0x1000 is just a safe distance. Let me check, but it might be required by the protocol.

The same stride is used within the firmware for modules within a libraries.

ujfalusi · 2022-08-18T12:46:29Z

Looks mostly good @ujfalusi, I added a number of nit-picks comments. My main concern is the IMR boot, this doesn't seem to be aligned with fixes we added for the base firmware for crashes and hibernate.

The new flag booted_from_imr will going to take care of that. I have decided to not play with existing flags decisions dealing with deciding if we should boot from IMR, but with the new flag we know if the firmware was booted up from IMR or not.
This will take care of the cases when we force the cold DSP boot after either firmware crash or coming up from hibernation.

plbossart · 2022-08-18T12:51:14Z

Looks mostly good @ujfalusi, I added a number of nit-picks comments. My main concern is the IMR boot, this doesn't seem to be aligned with fixes we added for the base firmware for crashes and hibernate.

The new flag booted_from_imr will going to take care of that. I have decided to not play with existing flags decisions dealing with deciding if we should boot from IMR, but with the new flag we know if the firmware was booted up from IMR or not. This will take care of the cases when we force the cold DSP boot after either firmware crash or coming up from hibernation.

I clearly missed where this flag is reset based on higher level information (crash or hibernate).

ujfalusi · 2022-08-18T13:21:11Z

Looks mostly good @ujfalusi, I added a number of nit-picks comments. My main concern is the IMR boot, this doesn't seem to be aligned with fixes we added for the base firmware for crashes and hibernate.

The new flag booted_from_imr will going to take care of that. I have decided to not play with existing flags decisions dealing with deciding if we should boot from IMR, but with the new flag we know if the firmware was booted up from IMR or not. This will take care of the cases when we force the cold DSP boot after either firmware crash or coming up from hibernation.

I clearly missed where this flag is reset based on higher level information (crash or hibernate).

It is never cleared as such. It is set to indicate exactly how the DSP got booted up. The decision to skip IMR should not be of concern for the library loading, what matters is how the DSP booted up. We might tried to boot from IMR, but failed to do that and done a clean boot instead.

kv2019i

Good work @ujfalusi and @ranj063 ! The reworked IPC abstraction is good, this makes it easier to support another library load mechanism on the side. The series seems very clean to me and easy to read the commit messages. Deprecation of IDR seems unfortunate, that helped code readability as well. Let's see the XArray version. One minor comment inline.

kv2019i · 2022-08-19T08:24:46Z

sound/soc/sof/sof-priv.h

 *			configuration from the booted firmware.
 *			Executed after the first successful firmware boot.
+ * @reload_fw_addons:	Optional function pointer to reload firmware components
+ *			after the base firmware has been booted up


@ujfalusi Isn't this quite close to existing "post_fw_run()" at DSP ops level? Maybe just have same for ipc_fw_loader ops (i.e. define post_fw_run also for fw_loaded_ops...?

ujfalusi · 2022-08-19T12:42:29Z

Changes since v1:

Use XArray instead of the deprecated IDR for managing libraries
The path for the libraries are now aligned with the firmware path (avs-lib or sof-ipc4-lib, depending on the platform)
max_fw_libs renamed to max_libs_count and it is now used when allocating IDs in XArray
The query_fw_configuration fw_loader ops is removed and replaced by generic ipc_ops.post_fw_boot ops
Commit message expanded for the new booted_from_imr flag
fini is now exit for IPC level cleanup function
lib_id is used in place of just id
copy paste comments and typos fixed

ujfalusi · 2022-08-22T11:03:40Z

SOFCI TEST

plbossart · 2022-08-22T12:26:36Z

SOFCI TEST

sound/soc/sof/intel/hda.h

sound/soc/sof/ipc4.c

RanderWang

LGTM. Do we plan to support unload lib ?

ujfalusi · 2022-08-23T07:34:28Z

LGTM. Do we plan to support unload lib ?

At this point it is not planned. When you would need such feature you would need to load a new tplg file, so reboot or module removals.
No plan for on demand loading either at this point to load only libraries needed by the started pipeline, we load all needed libraries by the tplg all the time

ujfalusi · 2022-08-24T12:49:32Z

sound/soc/sof/ipc4-loader.c

Note: we will keep the library in the kernel list even if loading fails. But this will only happen during tplg parsing at the moment and if that fails, the probe will also fail and thus it does not really matter since the SOF stack is not initialized to be usable.

Other use cases will need more planning to cover all corner cases.

I will not leave it like this. Let me find a solution which has no holes and TODOs.

…irectly Switch to access to the firmware struct via sdev->basefw container to unblock the removal of the firmware information from plat_data. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

The SOF stack now uses the sdev->basefw to work with the SOF firmware, the information from plat_data can be dropped. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

Add support for IPC specific initialization (init) and cleanup (exit) callback. These callbacks can be used by IPC implementation to do basic initialization and cleanup. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

The firmware supports external libraries (containing modules) to be loaded runtime. The firmware configuration contains the maximum number of libraries supported, including the base firmware (which is library 0). Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

…nvention With IPC4 each DSP loadable binary is a library, which contains ext_manifest section and loadable modules. The basefw is no exception, it is always library 0 and it can contain several modules, depending on the firmware build. The current code assumes only one binary, which is the basefw and has no concept of libraries. This patch introduces the library+modules abstraction and represents the basefw as library for the IPC4 loader codebase. The basefw loading and handling is not changing, it is still done by the generic code, but it's information is cloned under the library representation. The libraries are managed via XArray to offload the list and ID management. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

Add a simple helper to walk the loaded libraries and their modules to make the ipc4-topology not aware of the underlying infrastructure and simplify the code. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

IPC4 based firmware supports dynamically loaded external libraries. The libraries will be not stored alongside of the firmware or tplg files. For intel platforms the default path will be: intel/avs-lib|sof-ipc4-lib/<platform>/ if a community key is used on the given machine then the libraries will be under 'community' directory, like it is done for the firmware itself. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

The default path for the external firmware libraries are: intel/avs-lib/<platform> or intel/sof-ipc4-lib/<platform> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

Platforms where external libraries can be supported should set the load_library callback to implement this functionality. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

…booted Dynamic loading of external libraries should not be done if the firmware was booted from IMR since in that case the libraries will be restored along with the basefw. The booted_from_imr flag is introduced and set to true if the IMR boot was successful and to false if cold booting is executed. The reason for the new flag is that guessing from existing flags, used to decide if we should try booting from IMR or not is not going to be robust as the IMR boot itself can fail and in that case a full, cold boot is executed. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

On Intel HDA platforms the library loading is done via DMA and an IPC message is also need to be sent to initiate the downloading of the new library. Co-developed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

Add support for executing IPC dependent tasks after a successful firmware boot. The new post_fw_boot ops can make the fw_loader query_fw_configuration callback redundant as IPC code can handle the first boot internally. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

Execute the configuration query from the generic post_fw_boot callback and do not set the query_fw_configuration ops to allow it's removal. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

The query_fw_configuration callback is redundant and the only user of it was converted to use the generic post_fw_boot ops. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

In case the requested module is not available among the loaded libraries, try to load it as external library. The kernel will try to load the file from <fw_lib_prefix>/<module_uuid>.bin If the file found, then the ext manifest of it is parsed, placed it under XArray and the pointer to the module is returned to the caller. Releasing the firmware will be done on ipc cleanup time. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ujfalusi · 2022-09-15T13:40:53Z

Changes since v9:

Move the devm_kfree(sdev->dev, fw_lib->modules); up in sof_ipc4_load_library_by_uuid() and add a comment as well.

sound/soc/sof/ipc4-loader.c

aiChaoSONG

I have tested the initial draft from Ranjani maybe several months ago, at that time, the module loaded at runtime seems cannot work after dsp resuming from suspend. Module reloading is handled in the PR, so I will suppose it will work. Thanks.

ujfalusi · 2022-09-19T06:39:41Z

I have tested the initial draft from Ranjani maybe several months ago, at that time, the module loaded at runtime seems cannot work after dsp resuming from suspend. Module reloading is handled in the PR, so I will suppose it will work. Thanks.

@aiChaoSONG, for the library availability after suspend: if IMR boot is used then the library/module is reloaded via that method, if the IMR boot is disabled or not supported on the platform then with this PR we are going to re-load them in the same order as they were loaded initially.

ujfalusi requested review from RanderWang, ajitkupandey, bardliao, bhiregoudar, dbaluta, juimonen, kuanhsuncheng, kv2019i, lgirdwood, libinyang, lyakh, plbossart, ranj063, sunilkumardommati and yaochunhung as code owners August 18, 2022 10:28

plbossart requested changes Aug 18, 2022

View reviewed changes

kv2019i reviewed Aug 19, 2022

View reviewed changes

ujfalusi force-pushed the peter/sof/pr/ipc4_external_lib_support_v1 branch from 7fb1358 to 4da9ae5 Compare August 19, 2022 12:35

RanderWang reviewed Aug 23, 2022

View reviewed changes

sound/soc/sof/intel/hda.h Outdated Show resolved Hide resolved

RanderWang reviewed Aug 23, 2022

View reviewed changes

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

RanderWang reviewed Aug 23, 2022

View reviewed changes

RanderWang previously approved these changes Aug 23, 2022

View reviewed changes

ujfalusi commented Aug 24, 2022

View reviewed changes

ujfalusi added 15 commits September 15, 2022 16:39

ASoC: SOF: Intel: hda-loader-skl: Use the basefw firmware container d…

a1e313a

…irectly Switch to access to the firmware struct via sdev->basefw container to unblock the removal of the firmware information from plat_data. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ASoC: SOF: Drop the firmware and fw_offset from snd_sof_pdata

7e69eec

The SOF stack now uses the sdev->basefw to work with the SOF firmware, the information from plat_data can be dropped. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ASoC: SOF: IPC4: Add helper for looking up module by UUID

323bbed

Add a simple helper to walk the loaded libraries and their modules to make the ipc4-topology not aware of the underlying infrastructure and simplify the code. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ASoC: SOF: Intel: Set the default firmware library path for IPC4

ff9aedc

The default path for the external firmware libraries are: intel/avs-lib/<platform> or intel/sof-ipc4-lib/<platform> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ASoC: SOF: ipc4: Define platform dependent library loading callback

571f447

Platforms where external libraries can be supported should set the load_library callback to implement this functionality. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ASoC: SOF: ipc4: Stop using the query_fw_configuration fw_loader ops

4966d36

Execute the configuration query from the generic post_fw_boot callback and do not set the query_fw_configuration ops to allow it's removal. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ASoC: SOF: loader: Remove the query_fw_configuration ops

2a5ce69

The query_fw_configuration callback is redundant and the only user of it was converted to use the generic post_fw_boot ops. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ujfalusi force-pushed the peter/sof/pr/ipc4_external_lib_support_v1 branch from 5970706 to 210555c Compare September 15, 2022 13:39

ranj063 approved these changes Sep 16, 2022

View reviewed changes

plbossart approved these changes Sep 16, 2022

View reviewed changes

plbossart requested review from RanderWang, aiChaoSONG, jsarha and kv2019i September 16, 2022 16:00

aiChaoSONG reviewed Sep 19, 2022

View reviewed changes

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

aiChaoSONG approved these changes Sep 19, 2022

View reviewed changes

kv2019i approved these changes Sep 19, 2022

View reviewed changes

plbossart merged commit 3cd5d99 into thesofproject:topic/sof-dev Sep 19, 2022

ujfalusi deleted the peter/sof/pr/ipc4_external_lib_support_v1 branch November 18, 2022 07:31

ujfalusi mentioned this pull request Dec 17, 2024

intel_debug: introduction: Split firmware lookup table into two for m… thesofproject/sof-docs#506

Merged

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

plbossart left a comment

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!

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!

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!

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

Uh oh!

Uh oh!

plbossart commented Aug 18, 2022

Uh oh!

ujfalusi commented Aug 18, 2022

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ujfalusi commented Aug 19, 2022

Uh oh!

ujfalusi commented Aug 22, 2022

Uh oh!

plbossart commented Aug 22, 2022

Uh oh!

Uh oh!

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

ujfalusi commented Aug 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ujfalusi commented Aug 18, 2022 •

edited

Loading

ujfalusi commented Aug 18, 2022 •

edited

Loading

aiChaoSONG left a comment •

edited

Loading