Skip to content

Conversation

@ujfalusi
Copy link
Collaborator

Hi,

This PR contains #4301 as the first 10 patch, the new content is starting at
ASoC: SOF: pci-cnl: Correct the firmware filename for cfl and cml

Currently we have a set of string arrays to define the default paths and firmware filename for a platform (in struct sof_dev_desc) per IPC type - so we can have one set of strings for a given IPC type.
The fact that these strings are defined in the kernel, it is posing restriction on the file placement in filesystem level and prevent backwards, forwards compatibility in case a new layout is required or decided.
IPC3 has a standardized and stable filesystem layout among all vendors and platforms:

firmware path:		<vendor>/sof</fw_path_postfix>
firmware name:		sof-<platform>.ri
topology path:		<vendor>/sof-tplg/

note: fw_path_postfix: used by Intel for community key signed binaries.

IPC4 at the moment have two Intel layouts and no generic vendor neutral one. Moving these to a generic one would require interlocked update of firmware and kernel, which is realistically not a possibility in most cases.

This set proposes a solution which will allow multiple filesystem layout per IPC type, all generated and defined in core for a standardized presentation.
Device descriptions will include only a vendor name and platform name, for example:

diff --git a/sound/soc/sof/amd/pci-rmb.c b/sound/soc/sof/amd/pci-rmb.c
index 5cae889f9b35..a64e135b0420 100644
--- a/sound/soc/sof/amd/pci-rmb.c
+++ b/sound/soc/sof/amd/pci-rmb.c
@@ -47,15 +47,8 @@ static const struct sof_dev_desc rembrandt_desc = {
 	.chip_info		= &rembrandt_chip_info,
 	.ipc_supported_mask     = BIT(SOF_IPC_TYPE_3),
 	.ipc_default            = SOF_IPC_TYPE_3,
-	.default_fw_path	= {
-		[SOF_IPC_TYPE_3] = "amd/sof",
-	},
-	.default_tplg_path	= {
-		[SOF_IPC_TYPE_3] = "amd/sof-tplg",
-	},
-	.default_fw_filename	= {
-		[SOF_IPC_TYPE_3] = "sof-rmb.ri",
-	},
+	.vendor			= "amd",
+	.platform		= "rmb",
 	.nocodec_tplg_filename	= "sof-acp.tplg",
 	.ops			= &sof_rembrandt_ops,
 	.ops_init		= sof_rembrandt_ops_init,

or

diff --git a/sound/soc/sof/intel/pci-tgl.c b/sound/soc/sof/intel/pci-tgl.c
index 613119ec37a2..d9c2d6868ed6 100644
--- a/sound/soc/sof/intel/pci-tgl.c
+++ b/sound/soc/sof/intel/pci-tgl.c
@@ -19,6 +19,8 @@
 /* platform specific devices */
 #include "hda.h"
 
+static const char vendor_string[] = "intel";
+
 static const struct sof_dev_desc tgl_desc = {
 	.machines               = snd_soc_acpi_intel_tgl_machines,
 	.alt_machines		= snd_soc_acpi_intel_tgl_sdw_machines,
@@ -31,21 +33,8 @@ static const struct sof_dev_desc tgl_desc = {
 	.ipc_supported_mask	= BIT(SOF_IPC_TYPE_3) | BIT(SOF_IPC_TYPE_4),
 	.ipc_default		= SOF_IPC_TYPE_3,
 	.dspless_mode_supported	= true,		/* Only supported for HDaudio */
-	.default_fw_path = {
-		[SOF_IPC_TYPE_3] = "intel/sof",
-		[SOF_IPC_TYPE_4] = "intel/avs/tgl",
-	},
-	.default_lib_path = {
-		[SOF_IPC_TYPE_4] = "intel/avs-lib/tgl",
-	},
-	.default_tplg_path = {
-		[SOF_IPC_TYPE_3] = "intel/sof-tplg",
-		[SOF_IPC_TYPE_4] = "intel/avs-tplg",
-	},
-	.default_fw_filename = {
-		[SOF_IPC_TYPE_3] = "sof-tgl.ri",
-		[SOF_IPC_TYPE_4] = "dsp_basefw.bin",
-	},
+	.vendor			= vendor_string,
+	.platform		= "tgl",
 	.nocodec_tplg_filename = "sof-tgl-nocodec.tplg",
 	.ops = &sof_tgl_ops,
 	.ops_init = sof_tgl_ops_init,

The strings are going to be created by the core.

With this change the kernel can support multiple filesystem layout per IPC type and can discover the one which is deployed on the system.
We can also fall back to other IPC type in case the requested one not found on the filesystem (we can only check for the firmware image at this point).

@ujfalusi
Copy link
Collaborator Author

Looks like CML_HEL_RT5682 cannot find the firmware, other CML devices can , but this one is using community key. strange

@ujfalusi
Copy link
Collaborator Author

On the CI machine:

ls -al /lib/firmware/intel/sof/community/ | grep sof-cml.ri
lrwxrwxrwx 1 root root   34 Apr 18 10:03 sof-cml.ri -> /lib/firmware/intel/sof/sof-cnl.ri

on a arch machine:

ls -al /lib/firmware/intel/sof-v2.2.4/community | grep sof-cml.ri
lrwxrwxrwx 1 root root     10  7. 1. 23:17 sof-cml.ri -> sof-cnl.ri

so CI is using custom layout for this machine.

@ujfalusi
Copy link
Collaborator Author

I can fix this by forcing similar symlinking for IPC4, so link cml and cfl directories to cnl to make CI happy and probably other custom deployments out there?

Is this acceptable solution?

ujfalusi added 11 commits April 18, 2023 17:14
Change the enum names for the IPC types to be more descriptive and drop
tying the IPC4 to Intel SoCs.

Add defines to avoid build breakage while the related code is
modified to use the new enum names.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Clarify the description of the ipc_type module parameter and drop the Intel
CAVS in favor of IPC4.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Drop the Intel from the IPC type Kconfig option

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Use the new SOF_IPC_TYPE_3, SOF_IPC_TYPE_4 in core code.

No functional changes, just renaming.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Use the new SOF_IPC_TYPE_3 in core code.

No functional changes, just renaming.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Use the new SOF_IPC_TYPE_3 in core code.

No functional changes, just renaming.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Use the new SOF_IPC_TYPE_3, SOF_IPC_TYPE_4 in core code.

No functional changes, just renaming.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Use the new SOF_IPC_TYPE_4 in core code.

No functional changes, just renaming.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Use the new SOF_IPC_TYPE_3 in core code.

No functional changes, just renaming.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
The SOF stack now uses the generic names for the IPC type, the defines can
be dropped.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
All three platform described by pci-cnl.c (CNL, CFL and CML) uses the same
firmware image (sof-cnl.ri).
Packaging is handling the different file names by using symbolic links:
sof-cfl.ri -> sof-cnl.ri
sof-cml.ri -> sof-cnl.ri

For IPC4 the firmware and lib path should be:
intel/avs/cfl -> intel/avs/cnl
intel/avs-lib/cfl -> intel/avs-lib/cnl

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
ujfalusi added 16 commits April 18, 2023 17:14
The firmware file (and libraries for IPC4) and topology file location is
different for IPC versions which works well currently but it also places
constraints on file system layout changes since the default paths are
wired in the kernel.
IPC4 complicates this single set of paths per IPC implementations since on
Intel platforms it should be possible to use reference firmware or SOF,
both implementing IPC4.
With the current implementation it is also problematic to implement
seamless fallback to different firmware file layout for the same IPC type
and to have means to use other IPC versions in case the default/requested
one has not firmware file available.

With the proposed firmware profile layout machine descriptors will provide
two identification strings: name of the vendor (amd, imx, intel, mediatek),
name of the platform (apl, tgl, rmb, imx8, mt8188, etc).

From these strings the paths and firmware file name is constructed in a
standardized way.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
The purpose of the library is to craft the paths and firmware file name
for the IPC version.
After creating the paths we will try to open the default firmware image
ad check it's magic number to make sure it is matching with the expected
IPC type.

The sof_create_default_fw_layout_profile() will try to first create the
strings for the requested IPC type.
If no layout profile was found for the IPC type then we will try to probe
other IPC types as a fallback.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Set the vendor and platform names in sof_dev_desc, which will be used to
create the default firmware layout profile paths and firmware name.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Set the vendor and platform names in sof_dev_desc, which will be used to
create the default firmware layout profile paths and firmware name.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Set the vendor and platform names in sof_dev_desc, which will be used to
create the default firmware layout profile paths and firmware name.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…escriptors

Set the vendor and platform names in sof_dev_desc, which will be used to
create the default firmware layout profile paths and firmware name.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…tors

Set the vendor and platform names in sof_dev_desc, which will be used to
create the default firmware layout profile paths and firmware name.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…rofile

Create the default firmware layout profile based on the vendor and platform
strings in descriptors and use the default profile for paths and firmware
name.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…file

Create the default firmware layout profile based on the vendor and platform
strings in descriptors and use the default profile for paths and firmware
name.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…ofile

Create the default firmware layout profile based on the vendor and platform
strings in descriptors and use the default profile for paths and firmware
name.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…descriptor

The core no longer uses the default_fw_path, default_tplg_path and
default_fw_filename.
The paths and filename is created based on the vendor and platform strings.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…descriptor

The core no longer uses the default_fw_path, default_tplg_path and
default_fw_filename.
The paths and filename is created based on the vendor and platform strings.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…m descriptor

The core no longer uses the default_fw_path, default_tplg_path and
default_fw_filename.
The paths and filename is created based on the vendor and platform strings.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…ename from descriptor

The core no longer uses the default_fw_path, default_tplg_path and
default_fw_filename.
The paths and filename is created based on the vendor and platform strings.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…from descriptor

The core no longer uses the default_fw_path, default_tplg_path and
default_fw_filename.
The paths and filename is created based on the vendor and platform strings.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…re name

The default_fw_path, default_tplg_path, default_lib_path and
default_fw_filename are no longer used, they can be removed from
sof_dev_desc.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
@ujfalusi
Copy link
Collaborator Author

Changes since v1:

  • align the IPC4 path names with existing IPC3 standard for CFL, CML in pci-cnl.c

@ujfalusi ujfalusi force-pushed the peter/sof/pr/firmware_layout_profile_01 branch from 168ad26 to cbcdda8 Compare April 18, 2023 14:19
@plbossart
Copy link
Member

plbossart commented Apr 18, 2023

IPC4 at the moment have two Intel layouts and no generic vendor neutral one.

Why is this a problem?

a) the first Intel layout was defined by our colleagues with the 'firmware tester'. it is what it is.
b) the second layout was defined by popular vote in our own team. It is what it is. If we want to change it, we need a popular voice again.
c) we have no plan to enforce any naming or restrictions on other vendors. The naming is the problem at each vendor level, depending on if and how they plan to release binaries.

Can we please go back to the problem statement?

  1. what are we trying to achieve?
  2. what is broken?
  3. what does the suggested solution bring?
  4. how does this help the community and other vendors?

Edit
5) how does this leave our partners using Intel platform the freedom to use their own layout based on overlays? There will be multiple firmwares/libraries generated for the same SoC, so we can't have a one-size-fits-all layout. That was the basis for the 'default_path' than can be overridden at will. It was not planned to be a generic solution, only one that would be followed by Intel only for sof-bin releases. When sof-bin is not used, there is no convention required.

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like @plbossart 's comments need to be addressed first. If we drop the idea of generic lookup path, I guess a minimal version of fw-file-layout.c that would just try the different IPC fw+tplg entries in order (if they are defined), would still be possible. E.g. for platforms where both IPC3 and IPC4 entries are defined, kernel would try to use IPC4 first, check fw+tplg and if available, use those, and if not, fallback to IPC3.

.ipc_default = SOF_IPC_TYPE_3,
.default_fw_path = {
[SOF_IPC] = "imx/sof",
[SOF_IPC_TYPE_3] = "imx/sof",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the PR could be kept smaller by accepting that SOF_IPC continues to be used to refer to 3.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is more intuitive for the first look to have SOF_IPC_TYPE_3 than SOF_IPC
IPC4 is SOF_IPC, it is IPC type used by SOF.

if (i == ipc_type || !(desc->ipc_supported_mask & BIT(i)))
continue;

ret = fw_layout_for_ipc_type(dev, i, desc, fw_path_postfix,
Copy link
Collaborator

@kv2019i kv2019i Apr 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is the magic bit that allows to choose IPC major version based on available installed FW.

UPDATE: i.e. this would allow us to add SOF2.6 binaries to sof-bin release for distributions, and when this kernel hits a system with SOF2.6 installed, a portion of systems would move to use IPC4 and the new SOF2.6 release while older platforms would continue to use stable SOF2.2 binaries.

Copy link
Collaborator Author

@ujfalusi ujfalusi Apr 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we could change the default IPC type to IPC4 for everything in pci-tgl.c and provide only selected platform's IPC4 firmware (where we have good coverage of topologies). That would work fine.
This would also allow the possibility (if ever needed) to change the location of the SOF firmware things to
/lib/firmware/sof/< vendor >/< ipc_type >/.../
if the profile is described in kernel.

But looks like this is an overshot, so I'll try to scale it back.

@ujfalusi ujfalusi force-pushed the peter/sof/pr/firmware_layout_profile_01 branch 2 times, most recently from ad525ae to cbcdda8 Compare April 19, 2023 11:52
@ujfalusi
Copy link
Collaborator Author

Closing, completely new approach will be pushed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants