Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ujfalusi wants to merge 8 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7b63390
ASoC: SOF: Add placeholder for platform IPC type and path overrides
ujfalusi Apr 20, 2023
af602bd
ASoC: SOF: sof-acpi-dev: Save the default IPC type and path overrides
ujfalusi Apr 20, 2023
c53c7ea
ASoC: SOF: sof-of-dev: Save the default IPC type and path overrides
ujfalusi Apr 20, 2023
c36cda3
ASoC: SOF: sof-pci-dev: Save the default IPC type and path overrides
ujfalusi Apr 20, 2023
afe6537
ASoC: SOF: core: firmware, topology path setup to core with validation
ujfalusi Apr 20, 2023
42a127a
ASoC: SOF: sof-acpi-dev: Rely on core to create the file paths
ujfalusi Apr 20, 2023
a255636
ASoC: SOF: sof-of-dev: Rely on core to create the file paths
ujfalusi Apr 20, 2023
09dc400
ASoC: SOF: sof-pci-dev: Rely on core to create the file paths
ujfalusi Apr 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions include/sound/sof.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,17 @@ enum sof_ipc_type {
SOF_IPC_TYPE_COUNT
};

struct sof_loadable_file_profile {
enum sof_ipc_type ipc_type;

const char *fw_path;
const char *fw_name;
const char *fw_lib_path;
const char *fw_path_postfix;
const char *tplg_path;
const char *tplg_name;
};

/*
* SOF Platform data.
*/
Expand All @@ -78,6 +89,9 @@ struct snd_sof_pdata {
/* descriptor */
const struct sof_dev_desc *desc;

/* platform's preferred IPC type and path overrides */
struct sof_loadable_file_profile ipc_file_profile_base;

/* firmware and topology filenames */
const char *fw_filename_prefix;
const char *fw_filename;
Expand Down
3 changes: 2 additions & 1 deletion sound/soc/sof/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
# SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)

snd-sof-objs := core.o ops.o loader.o ipc.o pcm.o pm.o debug.o topology.o\
control.o trace.o iomem-utils.o sof-audio.o stream-ipc.o
control.o trace.o iomem-utils.o sof-audio.o stream-ipc.o\
fw-file-profile.o

# IPC implementations
ifneq ($(CONFIG_SND_SOC_SOF_IPC3),)
Expand Down
38 changes: 32 additions & 6 deletions sound/soc/sof/core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,35 @@ static void sof_probe_work(struct work_struct *work)
}
}

static int sof_select_ipc_and_paths(struct snd_sof_dev *sdev)
{
struct snd_sof_pdata *plat_data = sdev->pdata;
struct sof_loadable_file_profile *base_profile = &plat_data->ipc_file_profile_base;
struct sof_loadable_file_profile out_profile;
int ret;

/* check IPC support */
if (!(BIT(base_profile->ipc_type) & plat_data->desc->ipc_supported_mask)) {
dev_err(sdev->dev,
"ipc_type %d is not supported on this platform, mask is %#x\n",
base_profile->ipc_type, plat_data->desc->ipc_supported_mask);
return -EINVAL;
}

ret = sof_create_ipc_file_profile(sdev, base_profile, &out_profile);
if (ret)
return ret;

plat_data->ipc_type = out_profile.ipc_type;
plat_data->fw_filename = out_profile.fw_name;
plat_data->fw_filename_prefix = out_profile.fw_path;
plat_data->fw_lib_prefix = out_profile.fw_lib_path;
plat_data->tplg_filename_prefix = out_profile.tplg_path;
plat_data->tplg_filename = out_profile.tplg_name;

return 0;
}

int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data)
{
struct snd_sof_dev *sdev;
Expand Down Expand Up @@ -380,12 +409,9 @@ int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data)
}
}

/* check IPC support */
if (!(BIT(plat_data->ipc_type) & plat_data->desc->ipc_supported_mask)) {
dev_err(dev, "ipc_type %d is not supported on this platform, mask is %#x\n",
plat_data->ipc_type, plat_data->desc->ipc_supported_mask);
return -EINVAL;
}
ret = sof_select_ipc_and_paths(sdev);
if (ret)
return ret;

/* init ops, if necessary */
ret = sof_ops_init(sdev);
Expand Down
293 changes: 293 additions & 0 deletions sound/soc/sof/fw-file-profile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,293 @@
// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
//
// This file is provided under a dual BSD/GPLv2 license. When using or
// redistributing this file, you may do so under either license.
//
// Copyright(c) 2023 Intel Corporation. All rights reserved.
//

#include <linux/firmware.h>
#include <sound/sof.h>
#include <sound/sof/ext_manifest4.h>
#include "sof-priv.h"

static int sof_test_file_profile(struct device *dev,
struct sof_loadable_file_profile *profile,
enum sof_ipc_type *ipc_type_to_adjust)
{
enum sof_ipc_type fw_ipc_type;
const struct firmware *fw;
const char *fw_filename;
const u32 *magic;
int ret;

fw_filename = kasprintf(GFP_KERNEL, "%s/%s", profile->fw_path,
profile->fw_name);
if (!fw_filename)
return -ENOMEM;

ret = firmware_request_nowarn(&fw, fw_filename, dev);
if (ret < 0) {
kfree(fw_filename);
return ret;
}

/* firmware file exists, check the magic number */
magic = (const u32 *)fw->data;
switch (*magic) {
case SOF_EXT_MAN_MAGIC_NUMBER:
fw_ipc_type = SOF_IPC;
break;
case SOF_EXT_MAN4_MAGIC_NUMBER:
fw_ipc_type = SOF_INTEL_IPC4;
break;
default:
dev_err(dev, "Invalid firmware magic: %#x\n", *magic);
ret = -EINVAL;
goto out;
}

if (ipc_type_to_adjust) {
*ipc_type_to_adjust = fw_ipc_type;
} else if (fw_ipc_type != profile->ipc_type) {
dev_err(dev,
"ipc type mismatch between %s and expected: %d vs %d\n",
fw_filename, fw_ipc_type, profile->ipc_type);
ret = -EINVAL;
}
out:
release_firmware(fw);
kfree(fw_filename);

return ret;
}

static int
sof_file_profile_for_ipc_type(struct device *dev,
enum sof_ipc_type ipc_type,
const struct sof_dev_desc *desc,
struct sof_loadable_file_profile *base_profile,
struct sof_loadable_file_profile *out_profile)
{
bool fw_path_allocated = false;
bool fw_lib_path_allocated = false;
int ret = 0;

/* firmware path */
if (base_profile->fw_path) {
out_profile->fw_path = base_profile->fw_path;
} else if (base_profile->fw_path_postfix) {
out_profile->fw_path = devm_kasprintf(dev, GFP_KERNEL, "%s/%s",
desc->default_fw_path[ipc_type],
base_profile->fw_path_postfix);
if (!out_profile->fw_path)
return -ENOMEM;

fw_path_allocated = true;
} else {
out_profile->fw_path = desc->default_fw_path[ipc_type];
}

/* firmware filename */
if (base_profile->fw_name)
out_profile->fw_name = base_profile->fw_name;
else
out_profile->fw_name = desc->default_fw_filename[ipc_type];

/*
* Check the custom firmware path/filename and adjust the ipc_type to
* match with the existing file for the remaining path configuration.
*
* For default path and firmware name do a verification before
* continuing further.
*/
if (base_profile->fw_path || base_profile->fw_name) {
ret = sof_test_file_profile(dev, out_profile, &ipc_type);
if (ret)
return ret;

if (!(desc->ipc_supported_mask & BIT(ipc_type))) {
dev_err(dev, "Unsupported IPC type %d needed by %s/%s\n",
ipc_type, base_profile->fw_path,
base_profile->fw_name);
return -EINVAL;
}
}

/* firmware library path */
if (base_profile->fw_lib_path) {
out_profile->fw_lib_path = base_profile->fw_lib_path;
} else if (desc->default_lib_path[ipc_type]) {
if (base_profile->fw_path_postfix) {
out_profile->fw_lib_path = devm_kasprintf(dev,
GFP_KERNEL, "%s/%s",
desc->default_lib_path[ipc_type],
base_profile->fw_path_postfix);
if (!out_profile->fw_lib_path) {
ret = -ENOMEM;
goto out;
}

fw_lib_path_allocated = true;
} else {
out_profile->fw_lib_path = desc->default_lib_path[ipc_type];
}
}

if (base_profile->fw_path_postfix)
out_profile->fw_path_postfix = base_profile->fw_path_postfix;

/* topology path */
if (base_profile->tplg_path)
out_profile->tplg_path = base_profile->tplg_path;
else
out_profile->tplg_path = desc->default_tplg_path[ipc_type];

/* topology name */
if (base_profile->tplg_name)
out_profile->tplg_name = base_profile->tplg_name;

out_profile->ipc_type = ipc_type;
Copy link
Collaborator

@kv2019i kv2019i Apr 19, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@ujfalusi ujfalusi Apr 19, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@ujfalusi ujfalusi Apr 19, 2023

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator Author

@ujfalusi ujfalusi Apr 19, 2023

Choose a reason for hiding this comment

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

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.


/* Test only default firmware file */
if (!base_profile->fw_path && !base_profile->fw_name)
ret = sof_test_file_profile(dev, out_profile, NULL);

out:
if (ret) {
/* Free up path strings created with devm_kasprintf */
if (fw_path_allocated)
devm_kfree(dev, out_profile->fw_path);
if (fw_lib_path_allocated)
devm_kfree(dev, out_profile->fw_lib_path);

memset(out_profile, 0, sizeof(*out_profile));
}

return ret;
}

static void
sof_missing_firmware_notification(struct device *dev,
enum sof_ipc_type ipc_type,
const struct sof_dev_desc *desc,
struct sof_loadable_file_profile *base_profile)
{
char *marker;
int i;

dev_err(dev, "No SOF firmware file was found.\n");

for (i = 0; i < SOF_IPC_TYPE_COUNT; i++) {
if (!(desc->ipc_supported_mask & BIT(i)))
continue;

if (i == ipc_type)
marker = "Requested";
else
marker = "Fallback";
Copy link
Member

@plbossart plbossart Apr 19, 2023

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@ujfalusi ujfalusi Apr 19, 2023

Choose a reason for hiding this comment

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

Oh, true, I missed it from here.

Copy link
Member

@plbossart plbossart Apr 19, 2023

Choose a reason for hiding this comment

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

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",
	},

Copy link
Collaborator Author

@ujfalusi ujfalusi Apr 19, 2023

Choose a reason for hiding this comment

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

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


if (base_profile->fw_path_postfix)
dev_err(dev, " %s file: %s/%s/%s\n", marker,
desc->default_fw_path[i],
base_profile->fw_path_postfix,
desc->default_fw_filename[i]);
else
dev_err(dev, " %s file: %s/%s\n", marker,
desc->default_fw_path[i],
desc->default_fw_filename[i]);
}

dev_err(dev, "Check if you have 'sof-firmware' package installed.\n");
Copy link
Collaborator

@kv2019i kv2019i Apr 19, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@ujfalusi ujfalusi Apr 19, 2023

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@kv2019i kv2019i Apr 19, 2023

Choose a reason for hiding this comment

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

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

dev_err(dev, "Optionally it can be manually downloaded from:\n");
dev_err(dev, " https://github.com/thesofproject/sof-bin/\n");
}

static void sof_print_profile_info(struct device *dev,
enum sof_ipc_type ipc_type,
struct sof_loadable_file_profile *profile)
{
if (ipc_type != profile->ipc_type)
dev_info(dev,
"Using fallback IPC type %d (requested type was %d)\n",
profile->ipc_type, ipc_type);

dev_dbg(dev, "Loadable file profile%s for ipc type %d:\n",
ipc_type != profile->ipc_type ? " (fallback)" : "",
profile->ipc_type);

dev_dbg(dev, " Firmware file: %s/%s\n", profile->fw_path, profile->fw_name);
if (profile->fw_lib_path)
dev_dbg(dev, " Firmware lib path: %s\n", profile->fw_lib_path);
dev_dbg(dev, " Topology path: %s\n", profile->tplg_path);
if (profile->tplg_name)
dev_dbg(dev, " Topology name: %s\n", profile->tplg_name);
}

int sof_create_ipc_file_profile(struct snd_sof_dev *sdev,
struct sof_loadable_file_profile *base_profile,
struct sof_loadable_file_profile *out_profile)
{
const struct sof_dev_desc *desc = sdev->pdata->desc;
struct device *dev = sdev->dev;
int ret = -ENOENT;
int i;

memset(out_profile, 0, sizeof(*out_profile));

if (base_profile->fw_path)
dev_dbg(dev, "Module parameter used, changed fw path to %s\n",
base_profile->fw_path);
else if (base_profile->fw_path_postfix)
dev_dbg(dev, "Path postfix appended to default fw path: %s\n",
base_profile->fw_path_postfix);

if (base_profile->fw_lib_path)
dev_dbg(dev, "Module parameter used, changed fw_lib path to %s\n",
base_profile->fw_lib_path);
else if (base_profile->fw_path_postfix)
dev_dbg(dev, "Path postfix appended to default fw_lib path: %s\n",
base_profile->fw_path_postfix);

if (base_profile->fw_name)
dev_dbg(dev, "Module parameter used, changed fw filename to %s\n",
base_profile->fw_name);

if (base_profile->tplg_path)
dev_dbg(dev, "Module parameter used, changed tplg path to %s\n",
base_profile->tplg_path);

if (base_profile->tplg_name)
dev_dbg(dev, "Module parameter used, changed tplg name to %s\n",
base_profile->tplg_name);

ret = sof_file_profile_for_ipc_type(dev, base_profile->ipc_type, desc,
base_profile, out_profile);
if (!ret)
goto out;

/*
* No firmware file was found for the requested IPC type, check all
* IPC types as fallback
*/
for (i = 0; i < SOF_IPC_TYPE_COUNT; i++) {
if (i == base_profile->ipc_type ||
!(desc->ipc_supported_mask & BIT(i)))
continue;

ret = sof_file_profile_for_ipc_type(dev, i, desc, base_profile,
out_profile);
if (!ret)
break;
}

out:
if (ret)
sof_missing_firmware_notification(dev, base_profile->ipc_type,
desc, base_profile);
else
sof_print_profile_info(dev, base_profile->ipc_type, out_profile);

return ret;
}
EXPORT_SYMBOL(sof_create_ipc_file_profile);
Loading