| Message ID | 20221018120916.19820-1-peter.ujfalusi@linux.intel.com |
|---|---|
| Headers | show |
| Series | ASoC: SOF: Intel/IPC4: Support for external firmware libraries | expand |
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.
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.
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
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
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
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(-)