ASoC: SOF: file layout profile handling with fallback mechanism #4309

ujfalusi · 2023-04-19T12:05:02Z

Hi,

replaces #4308

This PR contains #4301 as the first 10 patch, the topmost two patch is the new one:
ASoC: SOF: Add library function for constructing loadable file paths, names
ASoC: SOF: sof-pci-dev: Use sof_create_file_profile() to create the file paths

Instead of cleaning up the current path/filename definitions in sof_dev_desc, this PR will introduce (the last two patch) a library file which can set/craft the paths, names used by the driver to load firmware files.
The entry to the library is the sof_create_file_profile() function which can handle overrides and also fallback to other IPC types in case the defaults are used (no override for firmware path or name).

Only sof-pci-dev.c is converted to use it atm as we only have multiple IPC types supported only with PCI devices.

kv2019i

A few quick comments. I'll try to test this still a bit before casting my final vote.

kv2019i · 2023-04-19T12:34:56Z

sound/soc/sof/fw-file-profile.c

+	if (path_override->tplg_name)
+		out_profile->tplg_name = path_override->tplg_name;
+
+	out_profile->ipc_type = ipc_type;


I guess on remaining complication is that if e.g. default IPC type is 4 for a platform, but the override passes path to a IPC 3 FW, the test would be skipped (due to override) and but FW load would fail (kernel configured to IPC4 but FW is actually IPC3). I believe this is the typical case for many Chromebooks, so a pretty common case.

Not really a problem with this PR, but basicly limits the usefulness. If we keep IPC3 as the default for all platforms (that support multiple IPCs), then IPC3 will always be chosen as that's always available.

Well, it is the user who is asking for it, right? It will fail later.
If we change the IPC type based on what the override points to then we would need to verify that the tplg path is aligned, so we would need to look for a matching tplg default path if it is not overridden, and that will open other door.

This is why I decided to not verify in case the fw path or/and name is overridden, it is something we cannot handle in a clean and sane way.

@kv2019i, to the specific scenario: I don't have a good answer. I don't know how that can be handled apart from requiring user to specify the IPC type alongside the fw path/name change to make sure they are aligned.

If you have a good algorithm to follow to cover that case, I'm all ears, but it is a bit of a wack-a-mole game with almost unlimited holes.

One thing might work:
If a fw path/name override is asked we would first test it and.take the IPC type from the file and proceed further to gather other paths.
We could warn if the IPC types differ, but not fail.

I think this can close the loophole.

kv2019i · 2023-04-19T12:35:42Z

sound/soc/sof/fw-file-profile.c

+				desc->default_fw_filename[i]);
+	}
+
+	dev_err(dev, "Check if you have 'sof-firmware' package installed.\n");


Minor nit: not sure how stable "sof-firmware" is as a pointer. In many RPM-based distros, package name is alsa-sof-firmware. Thus we just referred to sof-bin in the kernel error in the past.

I think the user should try to find a distro way first before reaching out to wget ;)

@ujfalusi Yup, we could add a "If you are seeing this message, you are looking too close." disclaimer here. ;)

plbossart

not fully getting the picture @ujfalusi

plbossart · 2023-04-19T15:39:36Z

sound/soc/sof/fw-file-profile.c

+	/* firmware path */
+	if (path_override->fw_path) {
+		out_profile->fw_path = path_override->fw_path;
+	} else if (path_override->fw_path_postfix) {


is there any incompatibility between fw_path and fw_path_postfix? I can recall any of this.

Yes, if the path is overridden then it is the full path, the postfix is ignored.
The postfix is the 'community' in Intel platforms.
If the user sets the path to "/home/developer/work/my_firmware" then that should be used, no?

plbossart · 2023-04-19T15:47:20Z

sound/soc/sof/fw-file-profile.c

+		if (i == ipc_type)
+			marker = "Requested";
+		else
+			marker = "Fallback";


we would need to verify which IPC_TYPE is supported for a given platforms. Some only have IPC3, some only IPC4.

Oh, true, I missed it from here.

plbossart · 2023-04-19T15:49:53Z

sound/soc/sof/fw-file-profile.c

+		if (i == ipc_type)
+			marker = "Requested";
+		else
+			marker = "Fallback";


the other problem here is that for IPC4 on TGL the default is the 'IPC4 tester', not cSOF.

static const struct sof_dev_desc tgl_desc = { .default_fw_path = { [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", }, .default_fw_filename = { [SOF_IPC] = "sof-tgl.ri", [SOF_INTEL_IPC4] = "dsp_basefw.bin", },

Sure, that is a problem, and the now closed PR would have solved that issue, but since that did not received well, I have dropped that to follow the suggestion to not support multiple profiles per IPC types.

We have one set of paths per IPC type and that can only be changed with a change similar to what was introduced by #4308

ujfalusi · 2023-04-20T14:17:38Z

Changes since v1:

Mostly rewritten, now the whole process is moved to core, sof-acpi/of/pci-dev is only saving the override strings, they don't do much.
Based on the saved parameters, the core will call sof_create_ipc_file_profile() to gather the paths and to have the firmware file existence verified before proceeding further.
We still support fallback from default fw locations
We support IPC type adjustment in case of a custom FW path/name is asked.

ujfalusi · 2023-04-21T10:38:02Z

Changes since v2:

free up possibly allocated fw_path if default path with postfix is used and the firmware is not present.

dbaluta · 2023-04-21T11:11:38Z

What is the purpose of this feature? To handle paths/etc in a single place?

Add a struct sof_loadable_file_profile which can be filled by platforms (sof-acpi-dev.c, sof-of-dev.c and sof-acpi-dev.c) to be able to use common, generic code to handle path customization. Signed-off-by: Peter Ujfalusi <[email protected]>

Store the default IPC type and the firmware and topology path overrides to ipc_file_profile_base Signed-off-by: Peter Ujfalusi <[email protected]>

Store the default IPC type and the overrides to ipc_file_profile_base Signed-off-by: Peter Ujfalusi <[email protected]>

Use the information stored in ipc_file_profile_base by platforms to construct the paths, filenames that are going to be used to load the firmware and topology files. The sof_create_ipc_file_profile() function will perform firmware file presence checking and it can also fall back to other supported IPC types in case the requested file for the type is not present. If the firmware path or name is overridden by a module parameter, we will check the file's IPC type and adjust the all other paths accordingly. Signed-off-by: Peter Ujfalusi <[email protected]>

The core is now using the information from ipc_file_profile_base to create the paths for the loadable files, no need to set it in here anymore. Signed-off-by: Peter Ujfalusi <[email protected]>

ujfalusi · 2023-04-21T11:14:13Z

Changes since v3:

Fix fallback handling in case of default paths and ipc type is 1

ujfalusi · 2023-04-21T11:36:11Z

What is the purpose of this feature? To handle paths/etc in a single place?

Yes, this series is doing that with the additional bonus of handling fallback path in case the default firmware is not in place - fallback to different IPC type, that is.

This is just on part of the puzzle I'm trying to solve, which is to be able to verify both firmware and topology file existence and if any of them is missing, try other ipc type.
We have IPC3 used by distributions and if we want to provide a path for users to move to IPC4 we need to make sure that their system can fall back to IPC3 in case the files are not in place.

I could have tried to aim only for sof-pci-dev, but I think this feature can be useful for all of us in the long run and the code is not in hot path, so a bit of logic would not hurt performance, but let us use common ways to craft the paths (if they are let to their defaults).

Does this answer your question?

dbaluta · 2023-04-21T11:40:32Z

@ujfalusi thanks this answers the question and now things are more clear. Ideally this explanation should be placed in one of the commit messages that gets kept in git history.

Also, thanks for going the generic way with pci/of/acpi.

ujfalusi · 2023-04-27T06:22:54Z

I'm going to replace this PR with a new one which will be able to do the topology file existence check also

ujfalusi · 2023-04-27T12:15:59Z

Replaced by take 3 at #4331, closing

ujfalusi requested review from ajitkupandey, bhiregoudar, sunilkumardommati, vijendarmukunda, libinyang, bardliao, lyakh, RanderWang, juimonen, ranj063, dbaluta, lgirdwood, plbossart, kv2019i, kuanhsuncheng and yaochunhung as code owners April 19, 2023 12:05

kv2019i reviewed Apr 19, 2023

View reviewed changes

plbossart reviewed Apr 19, 2023

View reviewed changes

ujfalusi force-pushed the peter/sof/pr/file_layout_profile_with_fallback_01 branch from 1d005c7 to 9f86975 Compare April 20, 2023 14:14

ujfalusi force-pushed the peter/sof/pr/file_layout_profile_with_fallback_01 branch from 9f86975 to 03cdad1 Compare April 21, 2023 10:36

ujfalusi force-pushed the peter/sof/pr/file_layout_profile_with_fallback_01 branch from 03cdad1 to 09dc400 Compare April 21, 2023 11:13

ujfalusi added 6 commits April 21, 2023 14:14

ASoC: SOF: sof-acpi-dev: Save the default IPC type and path overrides

af602bd

Store the default IPC type and the firmware and topology path overrides to ipc_file_profile_base Signed-off-by: Peter Ujfalusi <[email protected]>

ASoC: SOF: sof-of-dev: Save the default IPC type and path overrides

c53c7ea

Store the default IPC type and the firmware and topology path overrides to ipc_file_profile_base Signed-off-by: Peter Ujfalusi <[email protected]>

ASoC: SOF: sof-pci-dev: Save the default IPC type and path overrides

c36cda3

Store the default IPC type and the overrides to ipc_file_profile_base Signed-off-by: Peter Ujfalusi <[email protected]>

ASoC: SOF: sof-acpi-dev: Rely on core to create the file paths

42a127a

The core is now using the information from ipc_file_profile_base to create the paths for the loadable files, no need to set it in here anymore. Signed-off-by: Peter Ujfalusi <[email protected]>

ujfalusi added 2 commits April 21, 2023 14:14

ASoC: SOF: sof-of-dev: Rely on core to create the file paths

a255636

The core is now using the information from ipc_file_profile_base to create the paths for the loadable files, no need to set it in here anymore. Signed-off-by: Peter Ujfalusi <[email protected]>

ASoC: SOF: sof-pci-dev: Rely on core to create the file paths

09dc400

The core is now using the information from ipc_file_profile_base to create the paths for the loadable files, no need to set it in here anymore. Signed-off-by: Peter Ujfalusi <[email protected]>

ujfalusi mentioned this pull request Apr 27, 2023

ASoC: SOF: file layout profile handling with fallback mechanism (take 3) #4331

Merged

ujfalusi closed this Apr 27, 2023

ujfalusi deleted the peter/sof/pr/file_layout_profile_with_fallback_01 branch December 13, 2024 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASoC: SOF: file layout profile handling with fallback mechanism #4309

ASoC: SOF: file layout profile handling with fallback mechanism #4309

ujfalusi commented Apr 19, 2023

kv2019i left a comment

kv2019i Apr 19, 2023

ujfalusi Apr 19, 2023

ujfalusi Apr 19, 2023

ujfalusi Apr 19, 2023

kv2019i Apr 19, 2023

ujfalusi Apr 19, 2023

kv2019i Apr 19, 2023

plbossart left a comment

plbossart Apr 19, 2023

ujfalusi Apr 19, 2023

plbossart Apr 19, 2023

ujfalusi Apr 19, 2023

plbossart Apr 19, 2023

ujfalusi Apr 19, 2023

ujfalusi commented Apr 20, 2023

ujfalusi commented Apr 21, 2023

dbaluta commented Apr 21, 2023

ujfalusi commented Apr 21, 2023

ujfalusi commented Apr 21, 2023

dbaluta commented Apr 21, 2023 •

edited

Loading

ujfalusi commented Apr 27, 2023

ujfalusi commented Apr 27, 2023

ASoC: SOF: file layout profile handling with fallback mechanism #4309

ASoC: SOF: file layout profile handling with fallback mechanism #4309

Conversation

ujfalusi commented Apr 19, 2023

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ujfalusi commented Apr 20, 2023

ujfalusi commented Apr 21, 2023

dbaluta commented Apr 21, 2023

ujfalusi commented Apr 21, 2023

ujfalusi commented Apr 21, 2023

dbaluta commented Apr 21, 2023 • edited Loading

ujfalusi commented Apr 27, 2023

ujfalusi commented Apr 27, 2023

dbaluta commented Apr 21, 2023 •

edited

Loading