mbox series

[00/19] ASoC: SOF: Intel/IPC4: Support for external firmware libraries

Message ID 20221018120916.19820-1-peter.ujfalusi@linux.intel.com
Headers show
Series ASoC: SOF: Intel/IPC4: Support for external firmware libraries | expand

Message

Peter Ujfalusi Oct. 18, 2022, 12:08 p.m. UTC
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.

Regards,
Peter
---
Peter Ujfalusi (18):
  ASoC: SOF: Introduce container struct for SOF firmware
  ASoC: SOF: amd: Use the basefw firmware container directly
  ASoC: SOF: Intel: hda-loader: Use the basefw firmware container
    directly
  ASoC: SOF: Intel: hda-loader-skl: Use the basefw firmware container
    directly
  ASoC: SOF: Drop the firmware and fw_offset from snd_sof_pdata
  ASoC: SOF: ipc: ops: Add support for optional init and exit callbacks
  ASoC: SOF: ipc4-loader: Save the maximum number of libraries supported
  ASoC: SOF: ipc4: Convert the firmware handling (loader) to library
    convention
  ASoC: SOF: IPC4: Add helper for looking up module by UUID
  ASoC: SOF: Add path definition for external firmware libraries
  ASoC: SOF: Intel: Set the default firmware library path for IPC4
  ASoC: SOF: ipc4: Define platform dependent library loading callback
  ASoC: SOF: Intel: hda: Add flag to indicate that the firmware is IMR
    booted
  ASoC: SOF: Intel: Add ipc4 library loading implementation
  ASoC: SOF: loader: Add support for IPC dependent post firmware boot
    ops
  ASoC: SOF: ipc4: Stop using the query_fw_configuration fw_loader ops
  ASoC: SOF: loader: Remove the query_fw_configuration ops
  ASoC: SOF: ipc4-loader: Support for loading external libraries

Ranjani Sridharan (1):
  ASoC: SOF: loader: Set complete state before post_fw_run op

 include/sound/sof.h                  |  10 +-
 include/sound/sof/ipc4/header.h      |   4 +
 sound/soc/sof/amd/acp-loader.c       |   6 +-
 sound/soc/sof/intel/apl.c            |   3 +
 sound/soc/sof/intel/cnl.c            |   3 +
 sound/soc/sof/intel/hda-loader-skl.c |   7 +-
 sound/soc/sof/intel/hda-loader.c     |  83 +++++++++-
 sound/soc/sof/intel/hda.h            |   4 +
 sound/soc/sof/intel/icl.c            |   3 +
 sound/soc/sof/intel/mtl.c            |   3 +
 sound/soc/sof/intel/pci-apl.c        |   6 +
 sound/soc/sof/intel/pci-cnl.c        |   9 ++
 sound/soc/sof/intel/pci-icl.c        |   6 +
 sound/soc/sof/intel/pci-mtl.c        |   3 +
 sound/soc/sof/intel/pci-tgl.c        |  21 +++
 sound/soc/sof/intel/tgl.c            |   3 +
 sound/soc/sof/ipc.c                  |   6 +
 sound/soc/sof/ipc3-loader.c          |  26 ++-
 sound/soc/sof/ipc4-loader.c          | 233 ++++++++++++++++++++++++---
 sound/soc/sof/ipc4-priv.h            |  65 ++++++--
 sound/soc/sof/ipc4-topology.c        |  17 +-
 sound/soc/sof/ipc4.c                 |  41 +++++
 sound/soc/sof/loader.c               |  25 ++-
 sound/soc/sof/sof-pci-dev.c          |  26 +++
 sound/soc/sof/sof-priv.h             |  27 +++-
 25 files changed, 540 insertions(+), 100 deletions(-)

Comments

Amadeusz Sławiński Oct. 18, 2022, 12:38 p.m. UTC | #1
On 10/18/2022 2:09 PM, Peter Ujfalusi wrote:
> 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>
> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Chao Song <chao.song@intel.com>
> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> ---
>   sound/soc/sof/intel/pci-apl.c |  6 ++++++
>   sound/soc/sof/intel/pci-cnl.c |  9 +++++++++
>   sound/soc/sof/intel/pci-icl.c |  6 ++++++
>   sound/soc/sof/intel/pci-mtl.c |  3 +++
>   sound/soc/sof/intel/pci-tgl.c | 21 +++++++++++++++++++++
>   5 files changed, 45 insertions(+)
> 
> diff --git a/sound/soc/sof/intel/pci-apl.c b/sound/soc/sof/intel/pci-apl.c
> index 998e219011f0..69279dcc92dc 100644
> --- a/sound/soc/sof/intel/pci-apl.c
> +++ b/sound/soc/sof/intel/pci-apl.c
> @@ -33,6 +33,9 @@ static const struct sof_dev_desc bxt_desc = {
>   		[SOF_IPC] = "intel/sof",
>   		[SOF_INTEL_IPC4] = "intel/avs/apl",
>   	},
> +	.default_lib_path = {
> +		[SOF_INTEL_IPC4] = "intel/avs-lib/apl",
> +	},
>   	.default_tplg_path = {
>   		[SOF_IPC] = "intel/sof-tplg",
>   		[SOF_INTEL_IPC4] = "intel/avs-tplg",
> @@ -61,6 +64,9 @@ static const struct sof_dev_desc glk_desc = {
>   		[SOF_IPC] = "intel/sof",
>   		[SOF_INTEL_IPC4] = "intel/avs/glk",
>   	},
> +	.default_lib_path = {
> +		[SOF_INTEL_IPC4] = "intel/avs-lib/glk",
> +	},
>   	.default_tplg_path = {
>   		[SOF_IPC] = "intel/sof-tplg",
>   		[SOF_INTEL_IPC4] = "intel/avs-tplg",
> diff --git a/sound/soc/sof/intel/pci-cnl.c b/sound/soc/sof/intel/pci-cnl.c
> index c797356f7028..8db3f8d15b55 100644
> --- a/sound/soc/sof/intel/pci-cnl.c
> +++ b/sound/soc/sof/intel/pci-cnl.c
> @@ -34,6 +34,9 @@ static const struct sof_dev_desc cnl_desc = {
>   		[SOF_IPC] = "intel/sof",
>   		[SOF_INTEL_IPC4] = "intel/avs/cnl",
>   	},
> +	.default_lib_path = {
> +		[SOF_INTEL_IPC4] = "intel/avs-lib/cnl",
> +	},
>   	.default_tplg_path = {
>   		[SOF_IPC] = "intel/sof-tplg",
>   		[SOF_INTEL_IPC4] = "intel/avs-tplg",
> @@ -62,6 +65,9 @@ static const struct sof_dev_desc cfl_desc = {
>   		[SOF_IPC] = "intel/sof",
>   		[SOF_INTEL_IPC4] = "intel/avs/cnl",
>   	},
> +	.default_lib_path = {
> +		[SOF_INTEL_IPC4] = "intel/avs-lib/cnl",
> +	},
>   	.default_tplg_path = {
>   		[SOF_IPC] = "intel/sof-tplg",
>   		[SOF_INTEL_IPC4] = "intel/avs-tplg",
> @@ -91,6 +97,9 @@ static const struct sof_dev_desc cml_desc = {
>   		[SOF_IPC] = "intel/sof",
>   		[SOF_INTEL_IPC4] = "intel/avs/cnl",
>   	},
> +	.default_lib_path = {
> +		[SOF_INTEL_IPC4] = "intel/avs-lib/cnl",
> +	},
>   	.default_tplg_path = {
>   		[SOF_IPC] = "intel/sof-tplg",
>   		[SOF_INTEL_IPC4] = "intel/avs-tplg",
> diff --git a/sound/soc/sof/intel/pci-icl.c b/sound/soc/sof/intel/pci-icl.c
> index 48f24f8ace26..d6cf75e357db 100644
> --- a/sound/soc/sof/intel/pci-icl.c
> +++ b/sound/soc/sof/intel/pci-icl.c
> @@ -34,6 +34,9 @@ static const struct sof_dev_desc icl_desc = {
>   		[SOF_IPC] = "intel/sof",
>   		[SOF_INTEL_IPC4] = "intel/avs/icl",
>   	},
> +	.default_lib_path = {
> +		[SOF_INTEL_IPC4] = "intel/avs-lib/icl",
> +	},
>   	.default_tplg_path = {
>   		[SOF_IPC] = "intel/sof-tplg",
>   		[SOF_INTEL_IPC4] = "intel/avs-tplg",
> @@ -62,6 +65,9 @@ static const struct sof_dev_desc jsl_desc = {
>   		[SOF_IPC] = "intel/sof",
>   		[SOF_INTEL_IPC4] = "intel/avs/jsl",
>   	},
> +	.default_lib_path = {
> +		[SOF_INTEL_IPC4] = "intel/avs-lib/jsl",
> +	},
>   	.default_tplg_path = {
>   		[SOF_IPC] = "intel/sof-tplg",
>   		[SOF_INTEL_IPC4] = "intel/avs-tplg",
> diff --git a/sound/soc/sof/intel/pci-mtl.c b/sound/soc/sof/intel/pci-mtl.c
> index 899b00d53d64..84445daf33af 100644
> --- a/sound/soc/sof/intel/pci-mtl.c
> +++ b/sound/soc/sof/intel/pci-mtl.c
> @@ -34,6 +34,9 @@ static const struct sof_dev_desc mtl_desc = {
>   	.default_fw_path = {
>   		[SOF_INTEL_IPC4] = "intel/sof-ipc4/mtl",
>   	},
> +	.default_lib_path = {
> +		[SOF_INTEL_IPC4] = "intel/sof-ipc4-lib/mtl",
> +	},
>   	.default_tplg_path = {
>   		[SOF_INTEL_IPC4] = "intel/sof-ace-tplg",
>   	},
> diff --git a/sound/soc/sof/intel/pci-tgl.c b/sound/soc/sof/intel/pci-tgl.c
> index 2d63cc236a68..f9cbf3ad85b3 100644
> --- a/sound/soc/sof/intel/pci-tgl.c
> +++ b/sound/soc/sof/intel/pci-tgl.c
> @@ -34,6 +34,9 @@ static const struct sof_dev_desc tgl_desc = {
>   		[SOF_IPC] = "intel/sof",
>   		[SOF_INTEL_IPC4] = "intel/avs/tgl",
>   	},
> +	.default_lib_path = {
> +		[SOF_INTEL_IPC4] = "intel/avs-lib/tgl",
> +	},
>   	.default_tplg_path = {
>   		[SOF_IPC] = "intel/sof-tplg",
>   		[SOF_INTEL_IPC4] = "intel/avs-tplg",
> @@ -62,6 +65,9 @@ static const struct sof_dev_desc tglh_desc = {
>   		[SOF_IPC] = "intel/sof",
>   		[SOF_INTEL_IPC4] = "intel/avs/tgl-h",
>   	},
> +	.default_lib_path = {
> +		[SOF_INTEL_IPC4] = "intel/avs-lib/tgl-h",
> +	},
>   	.default_tplg_path = {
>   		[SOF_IPC] = "intel/sof-tplg",
>   		[SOF_INTEL_IPC4] = "intel/avs-tplg",
> @@ -90,6 +96,9 @@ static const struct sof_dev_desc ehl_desc = {
>   		[SOF_IPC] = "intel/sof",
>   		[SOF_INTEL_IPC4] = "intel/avs/ehl",
>   	},
> +	.default_lib_path = {
> +		[SOF_INTEL_IPC4] = "intel/avs-lib/ehl",
> +	},
>   	.default_tplg_path = {
>   		[SOF_IPC] = "intel/sof-tplg",
>   		[SOF_INTEL_IPC4] = "intel/avs-tplg",
> @@ -118,6 +127,9 @@ static const struct sof_dev_desc adls_desc = {
>   		[SOF_IPC] = "intel/sof",
>   		[SOF_INTEL_IPC4] = "intel/avs/adl-s",
>   	},
> +	.default_lib_path = {
> +		[SOF_INTEL_IPC4] = "intel/avs-lib/adl-s",
> +	},
>   	.default_tplg_path = {
>   		[SOF_IPC] = "intel/sof-tplg",
>   		[SOF_INTEL_IPC4] = "intel/avs-tplg",
> @@ -146,6 +158,9 @@ static const struct sof_dev_desc adl_desc = {
>   		[SOF_IPC] = "intel/sof",
>   		[SOF_INTEL_IPC4] = "intel/avs/adl",
>   	},
> +	.default_lib_path = {
> +		[SOF_INTEL_IPC4] = "intel/avs-lib/adl",
> +	},
>   	.default_tplg_path = {
>   		[SOF_IPC] = "intel/sof-tplg",
>   		[SOF_INTEL_IPC4] = "intel/avs-tplg",
> @@ -174,6 +189,9 @@ static const struct sof_dev_desc rpls_desc = {
>   		[SOF_IPC] = "intel/sof",
>   		[SOF_INTEL_IPC4] = "intel/avs/rpl-s",
>   	},
> +	.default_lib_path = {
> +		[SOF_INTEL_IPC4] = "intel/avs-lib/rpl-s",
> +	},
>   	.default_tplg_path = {
>   		[SOF_IPC] = "intel/sof-tplg",
>   		[SOF_INTEL_IPC4] = "intel/avs-tplg",
> @@ -202,6 +220,9 @@ static const struct sof_dev_desc rpl_desc = {
>   		[SOF_IPC] = "intel/sof",
>   		[SOF_INTEL_IPC4] = "intel/avs/rpl",
>   	},
> +	.default_lib_path = {
> +		[SOF_INTEL_IPC4] = "intel/avs-lib/rpl",
> +	},
>   	.default_tplg_path = {
>   		[SOF_IPC] = "intel/sof-tplg",
>   		[SOF_INTEL_IPC4] = "intel/avs-tplg",


I'm not sure that I understand the rationale here, can't libraries be 
kept in the same directory as FW, as far as I know they are version 
locked to FW anyway. In avs driver when we decided on intel/avs/platform 
path we planned to keep FW related libaries there...

Also adding Czarek to CC.
Peter Ujfalusi Oct. 18, 2022, 1:49 p.m. UTC | #2
On 18/10/2022 15:38, Amadeusz Sławiński wrote:
>> @@ -174,6 +189,9 @@ static const struct sof_dev_desc rpls_desc = {
>>           [SOF_IPC] = "intel/sof",
>>           [SOF_INTEL_IPC4] = "intel/avs/rpl-s",
>>       },
>> +    .default_lib_path = {
>> +        [SOF_INTEL_IPC4] = "intel/avs-lib/rpl-s",
>> +    },
>>       .default_tplg_path = {
>>           [SOF_IPC] = "intel/sof-tplg",
>>           [SOF_INTEL_IPC4] = "intel/avs-tplg",
>> @@ -202,6 +220,9 @@ static const struct sof_dev_desc rpl_desc = {
>>           [SOF_IPC] = "intel/sof",
>>           [SOF_INTEL_IPC4] = "intel/avs/rpl",
>>       },
>> +    .default_lib_path = {
>> +        [SOF_INTEL_IPC4] = "intel/avs-lib/rpl",
>> +    },
>>       .default_tplg_path = {
>>           [SOF_IPC] = "intel/sof-tplg",
>>           [SOF_INTEL_IPC4] = "intel/avs-tplg",
> 
> 
> I'm not sure that I understand the rationale here, can't libraries be
> kept in the same directory as FW, as far as I know they are version
> locked to FW anyway.

During the internal review we arrived to this setup, to keep the
libraries separate from the basefw binary.
One of the reason is that SOF project is not likely not going to release
external libraries, they are mostly vendor/product locked.
To make the life easier for the vendors (and distributions, packagers)
we concluded that it is better to keep them separate.

The option is there to specify custom path as well in case it is needed.

> In avs driver when we decided on intel/avs/platform
> path we planned to keep FW related libaries there...

My initial approach was this as well, but after a long debate it got
revised.

> Also adding Czarek to CC.
Cezary Rojewski Oct. 18, 2022, 5:18 p.m. UTC | #3
On 2022-10-18 6:37 PM, Pierre-Louis Bossart wrote:
> On 10/18/22 10:46, Cezary Rojewski wrote:
>> On 2022-10-18 3:49 PM, Péter Ujfalusi wrote:

...

>>> My initial approach was this as well, but after a long debate it got
>>> revised.
>>
>> Amadeo: have my bow..
>> Czarek: ..and my axe..
> 
> I don't understand what bow and axe have to do with this discussion.
> Let's say civil, shall we?

That's a common LOTR reference :-)


I'll address the rest of the feedback tomorrow. Thanks for the input though!

Czarek
Cezary Rojewski Oct. 19, 2022, 9:51 a.m. UTC | #4
On 2022-10-18 6:37 PM, Pierre-Louis Bossart wrote:
> On 10/18/22 10:46, Cezary Rojewski wrote:

...

> We should not debate on this mailing list what can or cannot be
> released, not make any distinctions between Intel and others. The
> library handling mechanism is generic, who provides the libraries is
> irrelevant.

No problem, leaving this out of the discussion.

...

> You're assuming that it's the same exact set of binary libraries for all
> skews based on the same SOC.

In majority of cases since Broadwell, that's the reality though. While 
theory may say otherwise, we were (and still are) releasing 
generation-wide builds e.g.: SKL-based firmware package, TGL-based 
firmware package.

...

> That's not necessarily a valid assumption, it's perfectly possible that
> a specific OEM decides to allocate more budget for a specific feature
> and less for others, resulting in libraries that are recompiled,
> optimized or configured differently. The UUID is a weak notion here, as
> measured by the same UUID being used for different DSP generations.
> Nothing prevents someone from generating a slightly different library
> exposed with the same UUID.
> 
> We didn't want to restrict our partners and gave them with the ability
> to put both the base firmware and the libraries in different directories
> and overload the default path should they wish to do so. They could
> decide to point to the same directory if they wanted to. That's not our
> decision.
> 
> If you look at all recent evolutions, we initially introduced different
> paths for firmware, then topology, then firmware and topology names. The
> logic of adding more flexibility for library path follow the pattern of
> trying to avoid making assumptions we have to undo at a later point.

Thanks for the elaborate input. The evolution sound good, and is 
perfectly reasonable. My only feedback is - should we put everything 
under /intel directory? If all the paths can be customized, then the 
parent directory needs not to be the same for every firmware package 
regardless of its origin. It's counterintuitive, is it not?


Regards,
Czarek
Cezary Rojewski Oct. 19, 2022, 11:58 a.m. UTC | #5
On 2022-10-19 1:16 PM, Péter Ujfalusi wrote:
> at the moment:
> # ls -al /lib/firmware/intel/ | wc -l
> 108
> 
> We might have 2 sets of binaries per platform, one using product key,
> other using community key.
> 
> If we dump everything in one directory (/lib/firmware/intel/), things
> will go out of hand pretty easily which can be somehow handled with
> complex file naming. This is only for the basefw, then we have the
> libraries (however they are sourced) with again two sets of keys, platforms.
> 
> Surely it can be done, but historically SOF prefers to use directories
> instead of pre/mid/post-fixing patterns of file names.

The concern is understandable. We haven't said that firmware files 
should be put directly under intel/ though.

> Also note that SOF is looking for a module UUID when trying to load a
> library we don't track arbitrary file names (see cover letter).

Neither 'dsp_fw_' nor 'dsp_lib_' prefix is arbitrary. A library may 
consist of more than one module, each with unique UUID. In general, 
'library' does not equal 'module'. Now, when speaking of modules, 
firmware-loading procedure that searches for file with certain UUID in 
its name is part of other drivers too and I haven't questioned that - 
it's perfectly fine and we're making use of the method ourselves.


Regards,
Czarek