[PATCH 00/11] Improve SOF support for Steam Deck OLED
This patch series is a continuation of [1] to provide fixes and improvements to SOF drivers targeting the Vangogh platform, as found on the Valve's Steam Deck OLED. The previous series only handled the legacy ACP drivers. [1]: https://lore.kernel.org/all/20231209203229.878730-1-cristian.ciocaltea@colla... Cristian Ciocaltea (11): ASoC: amd: acp: Drop redundant initialization of machine driver data ASoC: amd: acp: Make use of existing *_CODEC_DAI macros ASoC: amd: acp: Add missing error handling in sof-mach ASoC: amd: acp: Update MODULE_DESCRIPTION for sof-mach ASoC: SOF: amd: Fix memory leak in amd_sof_acp_probe() ASoC: SOF: amd: Optimize quirk for Valve Galileo ASoC: SOF: core: Skip firmware test for undefined fw_name ASoC: SOF: amd: Override default fw name for Valve Galileo ASoC: SOF: amd: Compute file paths on firmware load ASoC: amd: acp: Use correct DAI link ID for BT codec ASoC: SOF: topology: Add new DAI type entry for SOF_DAI_AMD_BT sound/soc/amd/acp/acp-mach-common.c | 6 +++--- sound/soc/amd/acp/acp-mach.h | 2 +- sound/soc/amd/acp/acp-sof-mach.c | 26 +++++++---------------- sound/soc/sof/amd/acp-loader.c | 32 +++++++++++++++++++++++------ sound/soc/sof/amd/acp.c | 30 ++++++++++++++++----------- sound/soc/sof/amd/vangogh.c | 8 +++++++- sound/soc/sof/fw-file-profile.c | 3 +++ sound/soc/sof/topology.c | 1 + 8 files changed, 66 insertions(+), 42 deletions(-) -- 2.43.0
Simplify driver data configuration by removing redundant initialization of members in static structs. Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/amd/acp/acp-sof-mach.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/sound/soc/amd/acp/acp-sof-mach.c b/sound/soc/amd/acp/acp-sof-mach.c index 2a9fd3275e42..1d313fcb5f2d 100644 --- a/sound/soc/amd/acp/acp-sof-mach.c +++ b/sound/soc/amd/acp/acp-sof-mach.c @@ -28,7 +28,6 @@ static struct acp_card_drvdata sof_rt5682_rt1019_data = { .hs_codec_id = RT5682, .amp_codec_id = RT1019, .dmic_codec_id = DMIC, - .tdm_mode = false, }; static struct acp_card_drvdata sof_rt5682_max_data = { @@ -38,7 +37,6 @@ static struct acp_card_drvdata sof_rt5682_max_data = { .hs_codec_id = RT5682, .amp_codec_id = MAX98360A, .dmic_codec_id = DMIC, - .tdm_mode = false, }; static struct acp_card_drvdata sof_rt5682s_rt1019_data = { @@ -48,7 +46,6 @@ static struct acp_card_drvdata sof_rt5682s_rt1019_data = { .hs_codec_id = RT5682S, .amp_codec_id = RT1019, .dmic_codec_id = DMIC, - .tdm_mode = false, }; static struct acp_card_drvdata sof_rt5682s_max_data = { @@ -58,7 +55,6 @@ static struct acp_card_drvdata sof_rt5682s_max_data = { .hs_codec_id = RT5682S, .amp_codec_id = MAX98360A, .dmic_codec_id = DMIC, - .tdm_mode = false, }; static struct acp_card_drvdata sof_nau8825_data = { @@ -69,7 +65,6 @@ static struct acp_card_drvdata sof_nau8825_data = { .amp_codec_id = MAX98360A, .dmic_codec_id = DMIC, .soc_mclk = true, - .tdm_mode = false, }; static struct acp_card_drvdata sof_rt5682s_hs_rt1019_data = { @@ -80,20 +75,15 @@ static struct acp_card_drvdata sof_rt5682s_hs_rt1019_data = { .amp_codec_id = RT1019, .dmic_codec_id = DMIC, .soc_mclk = true, - .tdm_mode = false, }; static struct acp_card_drvdata sof_nau8821_max98388_data = { .hs_cpu_id = I2S_SP, .amp_cpu_id = I2S_HS, .bt_cpu_id = I2S_BT, - .dmic_cpu_id = NONE, .hs_codec_id = NAU8821, .amp_codec_id = MAX98388, - .bt_codec_id = NONE, - .dmic_codec_id = NONE, .soc_mclk = true, - .tdm_mode = false, }; static int acp_sof_probe(struct platform_device *pdev) -- 2.43.0
The generic ACP machine driver provides macros for NAU88221 and MAX98388 codec DAI names, but in places it is still using directly the related strings. For consistency, replace all strings with the equivalent macros. Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/amd/acp/acp-mach-common.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sound/soc/amd/acp/acp-mach-common.c b/sound/soc/amd/acp/acp-mach-common.c index c90ec3419247..346f7514c81a 100644 --- a/sound/soc/amd/acp/acp-mach-common.c +++ b/sound/soc/amd/acp/acp-mach-common.c @@ -821,8 +821,8 @@ static const struct snd_soc_ops acp_card_maxim_ops = { }; SND_SOC_DAILINK_DEF(max98388, - DAILINK_COMP_ARRAY(COMP_CODEC("i2c-ADS8388:00", "max98388-aif1"), - COMP_CODEC("i2c-ADS8388:01", "max98388-aif1"))); + DAILINK_COMP_ARRAY(COMP_CODEC("i2c-ADS8388:00", MAX98388_CODEC_DAI), + COMP_CODEC("i2c-ADS8388:01", MAX98388_CODEC_DAI))); static const struct snd_kcontrol_new max98388_controls[] = { SOC_DAPM_PIN_SWITCH("Left Spk"), @@ -1273,7 +1273,7 @@ static const struct snd_soc_ops acp_8821_ops = { SND_SOC_DAILINK_DEF(nau8821, DAILINK_COMP_ARRAY(COMP_CODEC("i2c-NVTN2020:00", - "nau8821-hifi"))); + NAU8821_CODEC_DAI))); /* Declare DMIC codec components */ SND_SOC_DAILINK_DEF(dmic_codec, -- 2.43.0
Valve's Steam Deck OLED is uniquely identified by vendor and product name (Galileo) DMI fields. Simplify the quirk by removing the unnecessary match on product family. Additionally, fix the related comment as it points to the old product variant. Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/sof/amd/acp.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c index c6f637f29847..1e9840ae8938 100644 --- a/sound/soc/sof/amd/acp.c +++ b/sound/soc/sof/amd/acp.c @@ -28,11 +28,10 @@ MODULE_PARM_DESC(enable_fw_debug, "Enable Firmware debug"); const struct dmi_system_id acp_sof_quirk_table[] = { { - /* Valve Jupiter device */ + /* Steam Deck OLED device */ .matches = { DMI_MATCH(DMI_SYS_VENDOR, "Valve"), DMI_MATCH(DMI_PRODUCT_NAME, "Galileo"), - DMI_MATCH(DMI_PRODUCT_FAMILY, "Sephiroth"), }, .driver_data = (void *)SECURED_FIRMWARE, }, -- 2.43.0
On 12/10/23 02:23, Cristian Ciocaltea wrote:
Valve's Steam Deck OLED is uniquely identified by vendor and product name (Galileo) DMI fields.
Simplify the quirk by removing the unnecessary match on product family.
Additionally, fix the related comment as it points to the old product variant.
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/sof/amd/acp.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c index c6f637f29847..1e9840ae8938 100644 --- a/sound/soc/sof/amd/acp.c +++ b/sound/soc/sof/amd/acp.c @@ -28,11 +28,10 @@ MODULE_PARM_DESC(enable_fw_debug, "Enable Firmware debug");
const struct dmi_system_id acp_sof_quirk_table[] = { { - /* Valve Jupiter device */ + /* Steam Deck OLED device */ If any changes in SOF drivers, first need to create a PR in SOF github. .matches = { DMI_MATCH(DMI_SYS_VENDOR, "Valve"), DMI_MATCH(DMI_PRODUCT_NAME, "Galileo"), - DMI_MATCH(DMI_PRODUCT_FAMILY, "Sephiroth"), }, .driver_data = (void *)SECURED_FIRMWARE, },
On 12/10/23 05:33, Venkata Prasad Potturu wrote:
On 12/10/23 02:23, Cristian Ciocaltea wrote:
Valve's Steam Deck OLED is uniquely identified by vendor and product name (Galileo) DMI fields.
Simplify the quirk by removing the unnecessary match on product family.
Additionally, fix the related comment as it points to the old product variant.
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/sof/amd/acp.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c index c6f637f29847..1e9840ae8938 100644 --- a/sound/soc/sof/amd/acp.c +++ b/sound/soc/sof/amd/acp.c @@ -28,11 +28,10 @@ MODULE_PARM_DESC(enable_fw_debug, "Enable Firmware debug"); const struct dmi_system_id acp_sof_quirk_table[] = { { - /* Valve Jupiter device */ + /* Steam Deck OLED device */ If any changes in SOF drivers, first need to create a PR in SOF github.
This is just an optimization for the driver, no need to change anything on the firmware side. The product family remains as is, but it's not really required to match the board, i.e. the previous board was "Jupiter", the next one will have a different product name.
.matches = { DMI_MATCH(DMI_SYS_VENDOR, "Valve"), DMI_MATCH(DMI_PRODUCT_NAME, "Galileo"), - DMI_MATCH(DMI_PRODUCT_FAMILY, "Sephiroth"), }, .driver_data = (void *)SECURED_FIRMWARE, },
The ACP driver for Vangogh platform uses a quirk for Valve Galileo device to setup a custom firmware loader, which neither requires nor uses the firmware file indicated via the default_fw_filename member of struct sof_dev_desc. Since commit 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing"), the provided filename gets verified and triggers a fatal error on probe: [ 7.719337] snd_sof_amd_vangogh 0000:04:00.5: enabling device (0000 -> 0002) [ 7.721486] snd_sof_amd_vangogh 0000:04:00.5: SOF firmware and/or topology file not found. [ 7.721565] snd_sof_amd_vangogh 0000:04:00.5: Supported default profiles [ 7.721569] snd_sof_amd_vangogh 0000:04:00.5: - ipc type 0 (Requested): [ 7.721573] snd_sof_amd_vangogh 0000:04:00.5: Firmware file: amd/sof/sof-vangogh.ri [ 7.721577] snd_sof_amd_vangogh 0000:04:00.5: Topology file: amd/sof-tplg/sof-vangogh-nau8821-max.tplg [ 7.721582] snd_sof_amd_vangogh 0000:04:00.5: Check if you have 'sof-firmware' package installed. [ 7.721585] snd_sof_amd_vangogh 0000:04:00.5: Optionally it can be manually downloaded from: [ 7.721589] snd_sof_amd_vangogh 0000:04:00.5: https://github.com/thesofproject/sof-bin/ [ 7.721997] snd_sof_amd_vangogh: probe of 0000:04:00.5 failed with error -2 Skip testing the default firmware by overriding fw_name in sof_vangogh_ops_init(). Fixes: d0dab6b76a9f ("ASoC: SOF: amd: Add sof support for vangogh platform") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/sof/amd/vangogh.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/sound/soc/sof/amd/vangogh.c b/sound/soc/sof/amd/vangogh.c index de15d21aa6d9..5843ff8a8b40 100644 --- a/sound/soc/sof/amd/vangogh.c +++ b/sound/soc/sof/amd/vangogh.c @@ -151,8 +151,14 @@ int sof_vangogh_ops_init(struct snd_sof_dev *sdev) sof_vangogh_ops.num_drv = ARRAY_SIZE(vangogh_sof_dai); dmi_id = dmi_first_match(acp_sof_quirk_table); - if (dmi_id && dmi_id->driver_data) + if (dmi_id && dmi_id->driver_data) { sof_vangogh_ops.load_firmware = acp_sof_load_signed_firmware; + /* + * Board doesn't use the default firmware, hence override + * its name to prevent probe error due to fw validation. + */ + sdev->pdata->ipc_file_profile_base.fw_name = ""; + } return 0; } -- 2.43.0
Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink creation for I2S BT instance") added I2S BT support in ACP common machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2: [ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version 0:0:0-7863d [ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0 Kernel ABI 3:23:0 [ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0 Kernel ABI 3:23:0 [ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id 2) not exist [ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load header: -22 [ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22 [ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22 [ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22 [ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22 [ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max) [ 8.545022] sof_mach: probe of nau8821-max failed with error -22 Move BT_BE_ID to the correct position in the enum. Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink creation for I2S BT instance") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/amd/acp/acp-mach.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h index a48546d8d407..0c18ccd29305 100644 --- a/sound/soc/amd/acp/acp-mach.h +++ b/sound/soc/amd/acp/acp-mach.h @@ -27,8 +27,8 @@ enum be_id { HEADSET_BE_ID = 0, AMP_BE_ID, - DMIC_BE_ID, BT_BE_ID, + DMIC_BE_ID, }; enum cpu_endpoints { -- 2.43.0
On 12/10/23 02:23, Cristian Ciocaltea wrote:
Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink creation for I2S BT instance") added I2S BT support in ACP common machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2:
[ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version 0:0:0-7863d [ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0 Kernel ABI 3:23:0 [ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0 Kernel ABI 3:23:0 [ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id 2) not exist [ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load header: -22 [ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22 [ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22 [ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22 [ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22 [ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max) [ 8.545022] sof_mach: probe of nau8821-max failed with error -22
Move BT_BE_ID to the correct position in the enum.
Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink creation for I2S BT instance") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/amd/acp/acp-mach.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h index a48546d8d407..0c18ccd29305 100644 --- a/sound/soc/amd/acp/acp-mach.h +++ b/sound/soc/amd/acp/acp-mach.h @@ -27,8 +27,8 @@ enum be_id { HEADSET_BE_ID = 0, AMP_BE_ID, - DMIC_BE_ID, BT_BE_ID, + DMIC_BE_ID, This will break the other platforms as this same enum used in topology to create dailink. };
enum cpu_endpoints {
On 12/10/23 05:24, Venkata Prasad Potturu wrote:
On 12/10/23 02:23, Cristian Ciocaltea wrote:
Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink creation for I2S BT instance") added I2S BT support in ACP common machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2:
[ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version 0:0:0-7863d [ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0 Kernel ABI 3:23:0 [ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0 Kernel ABI 3:23:0 [ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id 2) not exist [ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load header: -22 [ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22 [ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22 [ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22 [ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22 [ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max) [ 8.545022] sof_mach: probe of nau8821-max failed with error -22
Move BT_BE_ID to the correct position in the enum.
Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink creation for I2S BT instance") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/amd/acp/acp-mach.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h index a48546d8d407..0c18ccd29305 100644 --- a/sound/soc/amd/acp/acp-mach.h +++ b/sound/soc/amd/acp/acp-mach.h @@ -27,8 +27,8 @@ enum be_id { HEADSET_BE_ID = 0, AMP_BE_ID, - DMIC_BE_ID, BT_BE_ID, + DMIC_BE_ID, This will break the other platforms as this same enum used in topology to create dailink.
If I understand this correctly, there is no consistency across firmware regarding the IDs used for DAI link identification. What would be the suggested solution in this case? Thanks, Cristian
}; enum cpu_endpoints {
On 12/10/23 14:36, Cristian Ciocaltea wrote:
On 12/10/23 05:24, Venkata Prasad Potturu wrote:
On 12/10/23 02:23, Cristian Ciocaltea wrote:
Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink creation for I2S BT instance") added I2S BT support in ACP common machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2:
[ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version 0:0:0-7863d [ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0 Kernel ABI 3:23:0 [ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0 Kernel ABI 3:23:0 [ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id 2) not exist [ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load header: -22 [ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22 [ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22 [ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22 [ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22 [ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max) [ 8.545022] sof_mach: probe of nau8821-max failed with error -22
Move BT_BE_ID to the correct position in the enum.
Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink creation for I2S BT instance") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/amd/acp/acp-mach.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h index a48546d8d407..0c18ccd29305 100644 --- a/sound/soc/amd/acp/acp-mach.h +++ b/sound/soc/amd/acp/acp-mach.h @@ -27,8 +27,8 @@ enum be_id { HEADSET_BE_ID = 0, AMP_BE_ID, - DMIC_BE_ID, BT_BE_ID, + DMIC_BE_ID, This will break the other platforms as this same enum used in topology to create dailink. If I understand this correctly, there is no consistency across firmware regarding the IDs used for DAI link identification. What would be the suggested solution in this case?
These id values should be same in machine driver and topology file, then only dailink can create without an error. Always new be_id should add at the end only. In this case BT_BE_ID should be at the end. enum be_id { HEADSET_BE_ID = 0, AMP_BE_ID, DMIC_BE_ID, BT_BE_ID, }
Thanks, Cristian
}; enum cpu_endpoints {
On 12/10/23 12:05, Venkata Prasad Potturu wrote:
On 12/10/23 14:36, Cristian Ciocaltea wrote:
On 12/10/23 05:24, Venkata Prasad Potturu wrote:
On 12/10/23 02:23, Cristian Ciocaltea wrote:
Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink creation for I2S BT instance") added I2S BT support in ACP common machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2:
[ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version 0:0:0-7863d [ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0 Kernel ABI 3:23:0 [ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0 Kernel ABI 3:23:0 [ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id 2) not exist [ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load header: -22 [ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22 [ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22 [ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22 [ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22 [ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max) [ 8.545022] sof_mach: probe of nau8821-max failed with error -22
Move BT_BE_ID to the correct position in the enum.
Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink creation for I2S BT instance") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/amd/acp/acp-mach.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h index a48546d8d407..0c18ccd29305 100644 --- a/sound/soc/amd/acp/acp-mach.h +++ b/sound/soc/amd/acp/acp-mach.h @@ -27,8 +27,8 @@ enum be_id { HEADSET_BE_ID = 0, AMP_BE_ID, - DMIC_BE_ID, BT_BE_ID, + DMIC_BE_ID, This will break the other platforms as this same enum used in topology to create dailink. If I understand this correctly, there is no consistency across firmware regarding the IDs used for DAI link identification. What would be the suggested solution in this case?
These id values should be same in machine driver and topology file, then only dailink can create without an error.
Yes, my point was that some topology files seem to require different IDs for the same DAI link types. In this case the topology expects ID 2 for BT, but other topologies would interpret that as DMIC.
Always new be_id should add at the end only.
In this case BT_BE_ID should be at the end.
enum be_id { HEADSET_BE_ID = 0, AMP_BE_ID, DMIC_BE_ID, BT_BE_ID, }
So you are basically stating the firmware is broken and needs an update to use ID 3 for BT, and there is nothing we can do about it on driver's side. Is that correct?
Thanks, Cristian
}; enum cpu_endpoints {
On 12/10/23 16:02, Cristian Ciocaltea wrote:
On 12/10/23 12:05, Venkata Prasad Potturu wrote:
On 12/10/23 14:36, Cristian Ciocaltea wrote:
On 12/10/23 05:24, Venkata Prasad Potturu wrote:
On 12/10/23 02:23, Cristian Ciocaltea wrote:
Commit 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink creation for I2S BT instance") added I2S BT support in ACP common machine driver, but using a wrong BT_BE_ID, i.e. 3 instead of 2:
[ 7.799659] snd_sof_amd_vangogh 0000:04:00.5: Firmware info: version 0:0:0-7863d [ 7.803906] snd_sof_amd_vangogh 0000:04:00.5: Firmware: ABI 3:26:0 Kernel ABI 3:23:0 [ 7.872873] snd_sof_amd_vangogh 0000:04:00.5: Topology: ABI 3:26:0 Kernel ABI 3:23:0 [ 8.508218] sof_mach nau8821-max: ASoC: physical link acp-bt-codec (id 2) not exist [ 8.513468] sof_mach nau8821-max: ASoC: topology: could not load header: -22 [ 8.518853] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22 [ 8.524049] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22 [ 8.529230] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22 [ 8.534465] sof_mach nau8821-max: ASoC: failed to instantiate card -22 [ 8.539820] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max) [ 8.545022] sof_mach: probe of nau8821-max failed with error -22
Move BT_BE_ID to the correct position in the enum.
Fixes: 671dd2ffbd8b ("ASoC: amd: acp: Add new cpu dai and dailink creation for I2S BT instance") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/amd/acp/acp-mach.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/amd/acp/acp-mach.h b/sound/soc/amd/acp/acp-mach.h index a48546d8d407..0c18ccd29305 100644 --- a/sound/soc/amd/acp/acp-mach.h +++ b/sound/soc/amd/acp/acp-mach.h @@ -27,8 +27,8 @@ enum be_id { HEADSET_BE_ID = 0, AMP_BE_ID, - DMIC_BE_ID, BT_BE_ID, + DMIC_BE_ID, This will break the other platforms as this same enum used in topology to create dailink. If I understand this correctly, there is no consistency across firmware regarding the IDs used for DAI link identification. What would be the suggested solution in this case? These id values should be same in machine driver and topology file, then only dailink can create without an error. Yes, my point was that some topology files seem to require different IDs for the same DAI link types. In this case the topology expects ID 2 for BT, but other topologies would interpret that as DMIC.
Always new be_id should add at the end only.
In this case BT_BE_ID should be at the end.
enum be_id { HEADSET_BE_ID = 0, AMP_BE_ID, DMIC_BE_ID, BT_BE_ID, } So you are basically stating the firmware is broken and needs an update to use ID 3 for BT, and there is nothing we can do about it on driver's side. Is that correct? Yes, id 3 should be used for BT_BE_ID in topology file.
Thanks, Cristian
}; enum cpu_endpoints {
Some SOF drivers like AMD ACP do not always rely on a single static firmware file, but may require multiple files having their names dynamically computed on probe time, e.g. based on chip name. In those cases, providing an invalid default_fw_filename in their sof_dev_desc struct will prevent probing due to 'SOF firmware and/or topology file not found' error. Fix the issue by allowing drivers to omit initialization for this member (or alternatively provide a dynamic override via ipc_file_profile_base) and update sof_test_firmware_file() to verify the given profile data and skip firmware testing if either fw_path or fw_name is not defined. Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/sof/fw-file-profile.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c index 138a1ca2c4a8..e63700234df0 100644 --- a/sound/soc/sof/fw-file-profile.c +++ b/sound/soc/sof/fw-file-profile.c @@ -21,6 +21,9 @@ static int sof_test_firmware_file(struct device *dev, const u32 *magic; int ret; + if (!profile->fw_path || !profile->fw_name || !*profile->fw_name) + return 0; + fw_filename = kasprintf(GFP_KERNEL, "%s/%s", profile->fw_path, profile->fw_name); if (!fw_filename) -- 2.43.0
On 09/12/2023 22:53, Cristian Ciocaltea wrote:
Some SOF drivers like AMD ACP do not always rely on a single static firmware file, but may require multiple files having their names dynamically computed on probe time, e.g. based on chip name.
I see, AMD vangogh needs two binary files to be loaded sometimes, it is not using the default name as it hints via the sof_dev_desc ("sof-vangogh.ri"). The constructed names for the two files are just using different pattern: sof-%PLAT%.ri vs sof-%PLAT%-code.bin sof-%PLAT%-data.bin iow, instead of the combined .ri file which includes the code and data segment it has 'raw' bin files and cannot use the core for loading the firmware. What is the reason for this? an .ri file can have two 'modules' one to be written to IRAM the other to DRAM. sof_ipc3_load_fw_to_dsp()
In those cases, providing an invalid default_fw_filename in their sof_dev_desc struct will prevent probing due to 'SOF firmware and/or topology file not found' error.
Fix the issue by allowing drivers to omit initialization for this member (or alternatively provide a dynamic override via ipc_file_profile_base) and update sof_test_firmware_file() to verify the given profile data and skip firmware testing if either fw_path or fw_name is not defined.
Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/sof/fw-file-profile.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c index 138a1ca2c4a8..e63700234df0 100644 --- a/sound/soc/sof/fw-file-profile.c +++ b/sound/soc/sof/fw-file-profile.c @@ -21,6 +21,9 @@ static int sof_test_firmware_file(struct device *dev, const u32 *magic; int ret;
+ if (!profile->fw_path || !profile->fw_name || !*profile->fw_name) + return 0;
I would rather make the firmware file skipping based on custom loader use and print a dev_dbg() alongside: diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c index 138a1ca2c4a8..7b91c9551ada 100644 --- a/sound/soc/sof/fw-file-profile.c +++ b/sound/soc/sof/fw-file-profile.c @@ -89,6 +89,15 @@ static int sof_test_topology_file(struct device *dev, return ret; } +static bool sof_platform_uses_generic_loader(struct snd_sof_dev *sdev) +{ + if (sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_raw || + sdev->pdata->desc->ops->load_firmware == snd_sof_load_firmware_memcpy) + return true; + + return false; +} + static int sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev, enum sof_ipc_type ipc_type, @@ -130,7 +139,8 @@ sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev, * For default path and firmware name do a verification before * continuing further. */ - if (base_profile->fw_path || base_profile->fw_name) { + if ((base_profile->fw_path || base_profile->fw_name) && + sof_platform_uses_generic_loader(sdev)) { ret = sof_test_firmware_file(dev, out_profile, &ipc_type); if (ret) return ret; @@ -181,7 +191,8 @@ sof_file_profile_for_ipc_type(struct snd_sof_dev *sdev, out_profile->ipc_type = ipc_type; /* Test only default firmware file */ - if (!base_profile->fw_path && !base_profile->fw_name) + if ((!base_profile->fw_path && !base_profile->fw_name) && + sof_platform_uses_generic_loader(sdev)) ret = sof_test_firmware_file(dev, out_profile, NULL); if (!ret) @@ -265,7 +276,9 @@ static void sof_print_profile_info(struct snd_sof_dev *sdev, "Using fallback IPC type %d (requested type was %d)\n", profile->ipc_type, ipc_type); - dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type); + /* The firmware path only valid when generic loader is used */ + if (sof_platform_uses_generic_loader(sdev)) + dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type); dev_info(dev, " Firmware file: %s/%s\n", profile->fw_path, profile->fw_name); if (profile->fw_lib_path)
+ fw_filename = kasprintf(GFP_KERNEL, "%s/%s", profile->fw_path, profile->fw_name); if (!fw_filename)
checking for custom loader allows you to drop the next patch. I would also skip the fw path printing in case of a custom loader. -- Péter
On 14/12/2023 12:48, Péter Ujfalusi wrote:
@@ -265,7 +276,9 @@ static void sof_print_profile_info(struct snd_sof_dev *sdev, "Using fallback IPC type %d (requested type was %d)\n", profile->ipc_type, ipc_type);
- dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type); + /* The firmware path only valid when generic loader is used */ + if (sof_platform_uses_generic_loader(sdev)) + dev_info(dev, "Firmware paths/files for ipc type %d:\n", profile->ipc_type);
This is the correct section in here, sorry: - dev_info(dev, " Firmware file: %s/%s\n", profile->fw_path, profile->fw_name); + /* The firmware path only valid when generic loader is used */ + if (sof_platform_uses_generic_loader(sdev)) + dev_info(dev, " Firmware file: %s/%s\n", profile->fw_path, profile->fw_name); + if (profile->fw_lib_path) -- Péter
Commit 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing") changed the order of some operations and the firmware paths are not available anymore at snd_sof_probe() time. Precisely, fw_filename_prefix is set by sof_select_ipc_and_paths() via plat_data->fw_filename_prefix = out_profile.fw_path; but sof_init_environment() which calls this function was moved from snd_sof_device_probe() to sof_probe_continue(). Moreover, snd_sof_probe() was moved from sof_probe_continue() to sof_init_environment(), but before the call to sof_select_ipc_and_paths(). The problem here is that amd_sof_acp_probe() uses fw_filename_prefix to compute fw_code_bin and fw_data_bin paths, and because the field is not yet initialized, the paths end up containing (null): snd_sof_amd_vangogh 0000:04:00.5: Direct firmware load for (null)/sof-vangogh-code.bin failed with error -2 snd_sof_amd_vangogh 0000:04:00.5: sof signed firmware code bin is missing snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP firmware -2 snd_sof_amd_vangogh: probe of 0000:04:00.5 failed with error -2 Move usage of fw_filename_prefix right before request_firmware() calls in acp_sof_load_signed_firmware(). Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/sof/amd/acp-loader.c | 32 ++++++++++++++++++++++++++------ sound/soc/sof/amd/acp.c | 7 ++----- 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/sound/soc/sof/amd/acp-loader.c b/sound/soc/sof/amd/acp-loader.c index e05eb7a86dd4..d2d21478399e 100644 --- a/sound/soc/sof/amd/acp-loader.c +++ b/sound/soc/sof/amd/acp-loader.c @@ -267,29 +267,49 @@ int acp_sof_load_signed_firmware(struct snd_sof_dev *sdev) { struct snd_sof_pdata *plat_data = sdev->pdata; struct acp_dev_data *adata = plat_data->hw_pdata; + const char *fw_filename; int ret; - ret = request_firmware(&sdev->basefw.fw, adata->fw_code_bin, sdev->dev); + fw_filename = kasprintf(GFP_KERNEL, "%s/%s", + plat_data->fw_filename_prefix, + adata->fw_code_bin); + if (!fw_filename) + return -ENOMEM; + + ret = request_firmware(&sdev->basefw.fw, fw_filename, sdev->dev); if (ret < 0) { + kfree(fw_filename); dev_err(sdev->dev, "sof signed firmware code bin is missing\n"); return ret; } else { - dev_dbg(sdev->dev, "request_firmware %s successful\n", adata->fw_code_bin); + dev_dbg(sdev->dev, "request_firmware %s successful\n", fw_filename); } + kfree(fw_filename); + ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_IRAM, 0, - (void *)sdev->basefw.fw->data, sdev->basefw.fw->size); + (void *)sdev->basefw.fw->data, + sdev->basefw.fw->size); + + fw_filename = kasprintf(GFP_KERNEL, "%s/%s", + plat_data->fw_filename_prefix, + adata->fw_data_bin); + if (!fw_filename) + return -ENOMEM; - ret = request_firmware(&adata->fw_dbin, adata->fw_data_bin, sdev->dev); + ret = request_firmware(&adata->fw_dbin, fw_filename, sdev->dev); if (ret < 0) { + kfree(fw_filename); dev_err(sdev->dev, "sof signed firmware data bin is missing\n"); return ret; } else { - dev_dbg(sdev->dev, "request_firmware %s successful\n", adata->fw_data_bin); + dev_dbg(sdev->dev, "request_firmware %s successful\n", fw_filename); } + kfree(fw_filename); ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_DRAM, 0, - (void *)adata->fw_dbin->data, adata->fw_dbin->size); + (void *)adata->fw_dbin->data, + adata->fw_dbin->size); return ret; } EXPORT_SYMBOL_NS(acp_sof_load_signed_firmware, SND_SOC_SOF_AMD_COMMON); diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c index 1e9840ae8938..87c5c71eac68 100644 --- a/sound/soc/sof/amd/acp.c +++ b/sound/soc/sof/amd/acp.c @@ -479,7 +479,6 @@ EXPORT_SYMBOL_NS(amd_sof_acp_resume, SND_SOC_SOF_AMD_COMMON); int amd_sof_acp_probe(struct snd_sof_dev *sdev) { struct pci_dev *pci = to_pci_dev(sdev->dev); - struct snd_sof_pdata *plat_data = sdev->pdata; struct acp_dev_data *adata; const struct sof_amd_acp_desc *chip; const struct dmi_system_id *dmi_id; @@ -547,8 +546,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev) dmi_id = dmi_first_match(acp_sof_quirk_table); if (dmi_id && dmi_id->driver_data) { adata->fw_code_bin = devm_kasprintf(sdev->dev, GFP_KERNEL, - "%s/sof-%s-code.bin", - plat_data->fw_filename_prefix, + "sof-%s-code.bin", chip->name); if (!adata->fw_code_bin) { ret = -ENOMEM; @@ -556,8 +554,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev) } adata->fw_data_bin = devm_kasprintf(sdev->dev, GFP_KERNEL, - "%s/sof-%s-data.bin", - plat_data->fw_filename_prefix, + "sof-%s-data.bin", chip->name); if (!adata->fw_data_bin) { ret = -ENOMEM; -- 2.43.0
On 12/10/23 02:23, Cristian Ciocaltea wrote:
Commit 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing") changed the order of some operations and the firmware paths are not available anymore at snd_sof_probe() time.
Precisely, fw_filename_prefix is set by sof_select_ipc_and_paths() via
plat_data->fw_filename_prefix = out_profile.fw_path;
but sof_init_environment() which calls this function was moved from snd_sof_device_probe() to sof_probe_continue(). Moreover, snd_sof_probe() was moved from sof_probe_continue() to sof_init_environment(), but before the call to sof_select_ipc_and_paths().
The problem here is that amd_sof_acp_probe() uses fw_filename_prefix to compute fw_code_bin and fw_data_bin paths, and because the field is not yet initialized, the paths end up containing (null):
snd_sof_amd_vangogh 0000:04:00.5: Direct firmware load for (null)/sof-vangogh-code.bin failed with error -2 snd_sof_amd_vangogh 0000:04:00.5: sof signed firmware code bin is missing snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP firmware -2 snd_sof_amd_vangogh: probe of 0000:04:00.5 failed with error -2
Move usage of fw_filename_prefix right before request_firmware() calls in acp_sof_load_signed_firmware().
Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/sof/amd/acp-loader.c | 32 ++++++++++++++++++++++++++------ sound/soc/sof/amd/acp.c | 7 ++----- 2 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/sound/soc/sof/amd/acp-loader.c b/sound/soc/sof/amd/acp-loader.c index e05eb7a86dd4..d2d21478399e 100644 --- a/sound/soc/sof/amd/acp-loader.c +++ b/sound/soc/sof/amd/acp-loader.c @@ -267,29 +267,49 @@ int acp_sof_load_signed_firmware(struct snd_sof_dev *sdev) { struct snd_sof_pdata *plat_data = sdev->pdata; struct acp_dev_data *adata = plat_data->hw_pdata; + const char *fw_filename; int ret;
- ret = request_firmware(&sdev->basefw.fw, adata->fw_code_bin, sdev->dev); + fw_filename = kasprintf(GFP_KERNEL, "%s/%s", + plat_data->fw_filename_prefix, + adata->fw_code_bin); File path already saved in adata->fw_code_bin in amd_sof_acp_probe function. No need to get it again. + if (!fw_filename) + return -ENOMEM; + + ret = request_firmware(&sdev->basefw.fw, fw_filename, sdev->dev); if (ret < 0) { + kfree(fw_filename); dev_err(sdev->dev, "sof signed firmware code bin is missing\n"); return ret; } else { - dev_dbg(sdev->dev, "request_firmware %s successful\n", adata->fw_code_bin); + dev_dbg(sdev->dev, "request_firmware %s successful\n", fw_filename); } + kfree(fw_filename); + ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_IRAM, 0, - (void *)sdev->basefw.fw->data, sdev->basefw.fw->size); + (void *)sdev->basefw.fw->data, + sdev->basefw.fw->size); + + fw_filename = kasprintf(GFP_KERNEL, "%s/%s", + plat_data->fw_filename_prefix, + adata->fw_data_bin); + if (!fw_filename) + return -ENOMEM;
- ret = request_firmware(&adata->fw_dbin, adata->fw_data_bin, sdev->dev); + ret = request_firmware(&adata->fw_dbin, fw_filename, sdev->dev); if (ret < 0) { + kfree(fw_filename); dev_err(sdev->dev, "sof signed firmware data bin is missing\n"); return ret;
} else { - dev_dbg(sdev->dev, "request_firmware %s successful\n", adata->fw_data_bin); + dev_dbg(sdev->dev, "request_firmware %s successful\n", fw_filename); } + kfree(fw_filename);
ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_DRAM, 0, - (void *)adata->fw_dbin->data, adata->fw_dbin->size); + (void *)adata->fw_dbin->data, + adata->fw_dbin->size); return ret; } EXPORT_SYMBOL_NS(acp_sof_load_signed_firmware, SND_SOC_SOF_AMD_COMMON); diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c index 1e9840ae8938..87c5c71eac68 100644 --- a/sound/soc/sof/amd/acp.c +++ b/sound/soc/sof/amd/acp.c @@ -479,7 +479,6 @@ EXPORT_SYMBOL_NS(amd_sof_acp_resume, SND_SOC_SOF_AMD_COMMON); int amd_sof_acp_probe(struct snd_sof_dev *sdev) { struct pci_dev *pci = to_pci_dev(sdev->dev); - struct snd_sof_pdata *plat_data = sdev->pdata; struct acp_dev_data *adata; const struct sof_amd_acp_desc *chip; const struct dmi_system_id *dmi_id; @@ -547,8 +546,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev) dmi_id = dmi_first_match(acp_sof_quirk_table); if (dmi_id && dmi_id->driver_data) { adata->fw_code_bin = devm_kasprintf(sdev->dev, GFP_KERNEL, - "%s/sof-%s-code.bin", - plat_data->fw_filename_prefix, + "sof-%s-code.bin", chip->name); if (!adata->fw_code_bin) { ret = -ENOMEM; @@ -556,8 +554,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev) }
adata->fw_data_bin = devm_kasprintf(sdev->dev, GFP_KERNEL, - "%s/sof-%s-data.bin", - plat_data->fw_filename_prefix, + "sof-%s-data.bin", chip->name); if (!adata->fw_data_bin) { ret = -ENOMEM;
On 12/10/23 05:50, Venkata Prasad Potturu wrote:
On 12/10/23 02:23, Cristian Ciocaltea wrote:
Commit 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing") changed the order of some operations and the firmware paths are not available anymore at snd_sof_probe() time.
Precisely, fw_filename_prefix is set by sof_select_ipc_and_paths() via
plat_data->fw_filename_prefix = out_profile.fw_path;
but sof_init_environment() which calls this function was moved from snd_sof_device_probe() to sof_probe_continue(). Moreover, snd_sof_probe() was moved from sof_probe_continue() to sof_init_environment(), but before the call to sof_select_ipc_and_paths().
The problem here is that amd_sof_acp_probe() uses fw_filename_prefix to compute fw_code_bin and fw_data_bin paths, and because the field is not yet initialized, the paths end up containing (null):
snd_sof_amd_vangogh 0000:04:00.5: Direct firmware load for (null)/sof-vangogh-code.bin failed with error -2 snd_sof_amd_vangogh 0000:04:00.5: sof signed firmware code bin is missing snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP firmware -2 snd_sof_amd_vangogh: probe of 0000:04:00.5 failed with error -2
Move usage of fw_filename_prefix right before request_firmware() calls in acp_sof_load_signed_firmware().
Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if firmware files are missing") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/sof/amd/acp-loader.c | 32 ++++++++++++++++++++++++++------ sound/soc/sof/amd/acp.c | 7 ++----- 2 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/sound/soc/sof/amd/acp-loader.c b/sound/soc/sof/amd/acp-loader.c index e05eb7a86dd4..d2d21478399e 100644 --- a/sound/soc/sof/amd/acp-loader.c +++ b/sound/soc/sof/amd/acp-loader.c @@ -267,29 +267,49 @@ int acp_sof_load_signed_firmware(struct snd_sof_dev *sdev) { struct snd_sof_pdata *plat_data = sdev->pdata; struct acp_dev_data *adata = plat_data->hw_pdata; + const char *fw_filename; int ret; - ret = request_firmware(&sdev->basefw.fw, adata->fw_code_bin, sdev->dev); + fw_filename = kasprintf(GFP_KERNEL, "%s/%s", + plat_data->fw_filename_prefix, + adata->fw_code_bin); File path already saved in adata->fw_code_bin in amd_sof_acp_probe function. No need to get it again.
As already stated in the patch description, fw_filename_prefix is not available anymore when amd_sof_acp_probe() gets invoked, and that is the root cause of ending up with (null) in the computed file path. Hence, the patch ensures amd_sof_acp_probe() computes the file name, without prefix, while acp_sof_load_signed_firmware() adds the prefix.
+ if (!fw_filename) + return -ENOMEM; + + ret = request_firmware(&sdev->basefw.fw, fw_filename, sdev->dev); if (ret < 0) { + kfree(fw_filename); dev_err(sdev->dev, "sof signed firmware code bin is missing\n"); return ret; } else { - dev_dbg(sdev->dev, "request_firmware %s successful\n", adata->fw_code_bin); + dev_dbg(sdev->dev, "request_firmware %s successful\n", fw_filename); } + kfree(fw_filename); + ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_IRAM, 0, - (void *)sdev->basefw.fw->data, sdev->basefw.fw->size); + (void *)sdev->basefw.fw->data, + sdev->basefw.fw->size); + + fw_filename = kasprintf(GFP_KERNEL, "%s/%s", + plat_data->fw_filename_prefix, + adata->fw_data_bin); + if (!fw_filename) + return -ENOMEM; - ret = request_firmware(&adata->fw_dbin, adata->fw_data_bin, sdev->dev); + ret = request_firmware(&adata->fw_dbin, fw_filename, sdev->dev); if (ret < 0) { + kfree(fw_filename); dev_err(sdev->dev, "sof signed firmware data bin is missing\n"); return ret; } else { - dev_dbg(sdev->dev, "request_firmware %s successful\n", adata->fw_data_bin); + dev_dbg(sdev->dev, "request_firmware %s successful\n", fw_filename); } + kfree(fw_filename); ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_DRAM, 0, - (void *)adata->fw_dbin->data, adata->fw_dbin->size); + (void *)adata->fw_dbin->data, + adata->fw_dbin->size); return ret; } EXPORT_SYMBOL_NS(acp_sof_load_signed_firmware, SND_SOC_SOF_AMD_COMMON); diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c index 1e9840ae8938..87c5c71eac68 100644 --- a/sound/soc/sof/amd/acp.c +++ b/sound/soc/sof/amd/acp.c @@ -479,7 +479,6 @@ EXPORT_SYMBOL_NS(amd_sof_acp_resume, SND_SOC_SOF_AMD_COMMON); int amd_sof_acp_probe(struct snd_sof_dev *sdev) { struct pci_dev *pci = to_pci_dev(sdev->dev); - struct snd_sof_pdata *plat_data = sdev->pdata; struct acp_dev_data *adata; const struct sof_amd_acp_desc *chip; const struct dmi_system_id *dmi_id; @@ -547,8 +546,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev) dmi_id = dmi_first_match(acp_sof_quirk_table); if (dmi_id && dmi_id->driver_data) { adata->fw_code_bin = devm_kasprintf(sdev->dev, GFP_KERNEL, - "%s/sof-%s-code.bin", - plat_data->fw_filename_prefix, + "sof-%s-code.bin", chip->name); if (!adata->fw_code_bin) { ret = -ENOMEM; @@ -556,8 +554,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev) } adata->fw_data_bin = devm_kasprintf(sdev->dev, GFP_KERNEL, - "%s/sof-%s-data.bin", - plat_data->fw_filename_prefix, + "sof-%s-data.bin", chip->name); if (!adata->fw_data_bin) { ret = -ENOMEM;
Commit efb931cdc4b9 ("ASoC: SOF: topology: Add support for AMD ACP DAIs") registered "ACP" name for SOF_DAI_AMD_BT DAI type. However, some boards, i.e. Steam Deck OLED, seem to require "ACPBT" for the same type: [ 467.489680] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for 0x30030000 (msg/reply size: 16/0): -22 [ 467.492775] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_route_setup: route ACPBT2.IN -> BUF5.0 failed [ 467.495839] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_set_up_all_pipelines: route set up failed [ 467.499128] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22 [ 467.502210] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22 [ 467.505289] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22 [ 467.508430] sof_mach nau8821-max: ASoC: failed to instantiate card -22 [ 467.511725] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max) [ 467.514861] sof_mach: probe of nau8821-max failed with error -22 Add "ACPBT" alias for "ACP" SOF DAI type. Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/sof/topology.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c index e3e7fbe40fa6..73bf791e7520 100644 --- a/sound/soc/sof/topology.c +++ b/sound/soc/sof/topology.c @@ -290,6 +290,7 @@ static const struct sof_dai_types sof_dais[] = { {"SAI", SOF_DAI_IMX_SAI}, {"ESAI", SOF_DAI_IMX_ESAI}, {"ACP", SOF_DAI_AMD_BT}, + {"ACPBT", SOF_DAI_AMD_BT}, {"ACPSP", SOF_DAI_AMD_SP}, {"ACPDMIC", SOF_DAI_AMD_DMIC}, {"ACPHS", SOF_DAI_AMD_HS}, -- 2.43.0
On 12/10/23 02:23, Cristian Ciocaltea wrote:
Commit efb931cdc4b9 ("ASoC: SOF: topology: Add support for AMD ACP DAIs") registered "ACP" name for SOF_DAI_AMD_BT DAI type. However, some boards, i.e. Steam Deck OLED, seem to require "ACPBT" for the same type:
[ 467.489680] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for 0x30030000 (msg/reply size: 16/0): -22 [ 467.492775] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_route_setup: route ACPBT2.IN -> BUF5.0 failed [ 467.495839] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_set_up_all_pipelines: route set up failed [ 467.499128] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22 [ 467.502210] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22 [ 467.505289] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22 [ 467.508430] sof_mach nau8821-max: ASoC: failed to instantiate card -22 [ 467.511725] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max) [ 467.514861] sof_mach: probe of nau8821-max failed with error -22
Add "ACPBT" alias for "ACP" SOF DAI type.
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/sof/topology.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c index e3e7fbe40fa6..73bf791e7520 100644 --- a/sound/soc/sof/topology.c +++ b/sound/soc/sof/topology.c @@ -290,6 +290,7 @@ static const struct sof_dai_types sof_dais[] = { {"SAI", SOF_DAI_IMX_SAI}, {"ESAI", SOF_DAI_IMX_ESAI}, {"ACP", SOF_DAI_AMD_BT}, + {"ACPBT", SOF_DAI_AMD_BT}, No need to create the alias name, we can directly modify ACP to ACPBT as ACP is not using anywhere. {"ACPSP", SOF_DAI_AMD_SP}, {"ACPDMIC", SOF_DAI_AMD_DMIC}, {"ACPHS", SOF_DAI_AMD_HS},
On 12/10/23 05:30, Venkata Prasad Potturu wrote:
On 12/10/23 02:23, Cristian Ciocaltea wrote:
Commit efb931cdc4b9 ("ASoC: SOF: topology: Add support for AMD ACP DAIs") registered "ACP" name for SOF_DAI_AMD_BT DAI type. However, some boards, i.e. Steam Deck OLED, seem to require "ACPBT" for the same type:
[ 467.489680] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for 0x30030000 (msg/reply size: 16/0): -22 [ 467.492775] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_route_setup: route ACPBT2.IN -> BUF5.0 failed [ 467.495839] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_set_up_all_pipelines: route set up failed [ 467.499128] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22 [ 467.502210] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22 [ 467.505289] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22 [ 467.508430] sof_mach nau8821-max: ASoC: failed to instantiate card -22 [ 467.511725] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max) [ 467.514861] sof_mach: probe of nau8821-max failed with error -22
Add "ACPBT" alias for "ACP" SOF DAI type.
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/sof/topology.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c index e3e7fbe40fa6..73bf791e7520 100644 --- a/sound/soc/sof/topology.c +++ b/sound/soc/sof/topology.c @@ -290,6 +290,7 @@ static const struct sof_dai_types sof_dais[] = { {"SAI", SOF_DAI_IMX_SAI}, {"ESAI", SOF_DAI_IMX_ESAI}, {"ACP", SOF_DAI_AMD_BT}, + {"ACPBT", SOF_DAI_AMD_BT}, No need to create the alias name, we can directly modify ACP to ACPBT as ACP is not using anywhere.
Great, thanks, will do in v2.
{"ACPSP", SOF_DAI_AMD_SP}, {"ACPDMIC", SOF_DAI_AMD_DMIC}, {"ACPHS", SOF_DAI_AMD_HS},
On 12/10/23 05:30, Venkata Prasad Potturu wrote:
On 12/10/23 02:23, Cristian Ciocaltea wrote:
Commit efb931cdc4b9 ("ASoC: SOF: topology: Add support for AMD ACP DAIs") registered "ACP" name for SOF_DAI_AMD_BT DAI type. However, some boards, i.e. Steam Deck OLED, seem to require "ACPBT" for the same type:
[ 467.489680] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for 0x30030000 (msg/reply size: 16/0): -22 [ 467.492775] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_route_setup: route ACPBT2.IN -> BUF5.0 failed [ 467.495839] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_set_up_all_pipelines: route set up failed [ 467.499128] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22 [ 467.502210] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22 [ 467.505289] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22 [ 467.508430] sof_mach nau8821-max: ASoC: failed to instantiate card -22 [ 467.511725] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max) [ 467.514861] sof_mach: probe of nau8821-max failed with error -22
Add "ACPBT" alias for "ACP" SOF DAI type.
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/sof/topology.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c index e3e7fbe40fa6..73bf791e7520 100644 --- a/sound/soc/sof/topology.c +++ b/sound/soc/sof/topology.c @@ -290,6 +290,7 @@ static const struct sof_dai_types sof_dais[] = { {"SAI", SOF_DAI_IMX_SAI}, {"ESAI", SOF_DAI_IMX_ESAI}, {"ACP", SOF_DAI_AMD_BT}, + {"ACPBT", SOF_DAI_AMD_BT}, No need to create the alias name, we can directly modify ACP to ACPBT as ACP is not using anywhere. Great, thanks, will do in v2. This should send to SOF git repo for rewiew, once SOF reviewers approved
On 12/10/23 14:38, Cristian Ciocaltea wrote: this, again need to send to broonie git. All the changes in sound/soc/sof/ path should go to SOF git.
{"ACPSP", SOF_DAI_AMD_SP}, {"ACPDMIC", SOF_DAI_AMD_DMIC}, {"ACPHS", SOF_DAI_AMD_HS},
On 12/10/23 11:51, Venkata Prasad Potturu wrote:
On 12/10/23 05:30, Venkata Prasad Potturu wrote:
On 12/10/23 02:23, Cristian Ciocaltea wrote:
Commit efb931cdc4b9 ("ASoC: SOF: topology: Add support for AMD ACP DAIs") registered "ACP" name for SOF_DAI_AMD_BT DAI type. However, some boards, i.e. Steam Deck OLED, seem to require "ACPBT" for the same type:
[ 467.489680] snd_sof_amd_vangogh 0000:04:00.5: ipc tx error for 0x30030000 (msg/reply size: 16/0): -22 [ 467.492775] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_route_setup: route ACPBT2.IN -> BUF5.0 failed [ 467.495839] snd_sof_amd_vangogh 0000:04:00.5: sof_ipc3_set_up_all_pipelines: route set up failed [ 467.499128] snd_sof_amd_vangogh 0000:04:00.5: error: tplg component load failed -22 [ 467.502210] snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP topology -22 [ 467.505289] snd_sof_amd_vangogh 0000:04:00.5: ASoC: error at snd_soc_component_probe on 0000:04:00.5: -22 [ 467.508430] sof_mach nau8821-max: ASoC: failed to instantiate card -22 [ 467.511725] sof_mach nau8821-max: error -EINVAL: Failed to register card(sof-nau8821-max) [ 467.514861] sof_mach: probe of nau8821-max failed with error -22
Add "ACPBT" alias for "ACP" SOF DAI type.
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/sof/topology.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c index e3e7fbe40fa6..73bf791e7520 100644 --- a/sound/soc/sof/topology.c +++ b/sound/soc/sof/topology.c @@ -290,6 +290,7 @@ static const struct sof_dai_types sof_dais[] = { {"SAI", SOF_DAI_IMX_SAI}, {"ESAI", SOF_DAI_IMX_ESAI}, {"ACP", SOF_DAI_AMD_BT}, + {"ACPBT", SOF_DAI_AMD_BT}, No need to create the alias name, we can directly modify ACP to ACPBT as ACP is not using anywhere. Great, thanks, will do in v2. This should send to SOF git repo for rewiew, once SOF reviewers approved
On 12/10/23 14:38, Cristian Ciocaltea wrote: this, again need to send to broonie git. All the changes in sound/soc/sof/ path should go to SOF git.
Unfortunately I'm not familiar with the SOF dev workflow. So it's not enough to have this patch cc-ed to sound-open-firmware@alsa-project.org?
{"ACPSP", SOF_DAI_AMD_SP}, {"ACPDMIC", SOF_DAI_AMD_DMIC}, {"ACPHS", SOF_DAI_AMD_HS},
The current MODULE_DESCRIPTION relates to a Chrome board, as that was what the driver initially supported. Nonetheless, it has since progressed incrementally and evolved into a more comprehensive machine driver. Hence, update MODULE_DESCRIPTION to better reflect this. Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/amd/acp/acp-sof-mach.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sound/soc/amd/acp/acp-sof-mach.c b/sound/soc/amd/acp/acp-sof-mach.c index 6f0ca23638af..19ff4fe5b1ea 100644 --- a/sound/soc/amd/acp/acp-sof-mach.c +++ b/sound/soc/amd/acp/acp-sof-mach.c @@ -166,7 +166,7 @@ static struct platform_driver acp_asoc_audio = { module_platform_driver(acp_asoc_audio); MODULE_IMPORT_NS(SND_SOC_AMD_MACH); -MODULE_DESCRIPTION("ACP chrome SOF audio support"); +MODULE_DESCRIPTION("ACP SOF Machine Driver"); MODULE_ALIAS("platform:rt5682-rt1019"); MODULE_ALIAS("platform:rt5682-max"); MODULE_ALIAS("platform:rt5682s-max"); -- 2.43.0
Handle potential acp_sofdsp_dai_links_create() errors in ACP SOF machine driver's probe function. Additionally, switch to dev_err_probe(). Fixes: 9f84940f5004 ("ASoC: amd: acp: Add SOF audio support on Chrome board") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/amd/acp/acp-sof-mach.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/sound/soc/amd/acp/acp-sof-mach.c b/sound/soc/amd/acp/acp-sof-mach.c index 1d313fcb5f2d..6f0ca23638af 100644 --- a/sound/soc/amd/acp/acp-sof-mach.c +++ b/sound/soc/amd/acp/acp-sof-mach.c @@ -112,16 +112,14 @@ static int acp_sof_probe(struct platform_device *pdev) if (dmi_id && dmi_id->driver_data) acp_card_drvdata->tdm_mode = dmi_id->driver_data; - acp_sofdsp_dai_links_create(card); + ret = acp_sofdsp_dai_links_create(card); + if (ret) + return dev_err_probe(&pdev->dev, ret, "Failed to create DAI links\n"); ret = devm_snd_soc_register_card(&pdev->dev, card); - if (ret) { - dev_err(&pdev->dev, - "devm_snd_soc_register_card(%s) failed: %d\n", - card->name, ret); - return ret; - } - + if (ret) + return dev_err_probe(&pdev->dev, ret, + "Failed to register card(%s)\n", card->name); return 0; } -- 2.43.0
On 2023/12/09, Cristian Ciocaltea wrote:
Handle potential acp_sofdsp_dai_links_create() errors in ACP SOF machine driver's probe function. Additionally, switch to dev_err_probe().
Fixes: 9f84940f5004 ("ASoC: amd: acp: Add SOF audio support on Chrome board") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/amd/acp/acp-sof-mach.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/sound/soc/amd/acp/acp-sof-mach.c b/sound/soc/amd/acp/acp-sof-mach.c index 1d313fcb5f2d..6f0ca23638af 100644 --- a/sound/soc/amd/acp/acp-sof-mach.c +++ b/sound/soc/amd/acp/acp-sof-mach.c @@ -112,16 +112,14 @@ static int acp_sof_probe(struct platform_device *pdev) if (dmi_id && dmi_id->driver_data) acp_card_drvdata->tdm_mode = dmi_id->driver_data;
- acp_sofdsp_dai_links_create(card); + ret = acp_sofdsp_dai_links_create(card); + if (ret) + return dev_err_probe(&pdev->dev, ret, "Failed to create DAI links\n");
ret = devm_snd_soc_register_card(&pdev->dev, card); - if (ret) { - dev_err(&pdev->dev, - "devm_snd_soc_register_card(%s) failed: %d\n", - card->name, ret); - return ret; - } - + if (ret)
Do we need to undo acp_sofdsp_dai_links_create() in here? If not, please add a trivial note in the commit message. With that the series is: Reviewed-by: Emil Velikov <emil.velikov@collabora.com> HTH o/ -Emil
On 12/11/23 15:31, Emil Velikov wrote:
On 2023/12/09, Cristian Ciocaltea wrote:
Handle potential acp_sofdsp_dai_links_create() errors in ACP SOF machine driver's probe function. Additionally, switch to dev_err_probe().
Fixes: 9f84940f5004 ("ASoC: amd: acp: Add SOF audio support on Chrome board") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/amd/acp/acp-sof-mach.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/sound/soc/amd/acp/acp-sof-mach.c b/sound/soc/amd/acp/acp-sof-mach.c index 1d313fcb5f2d..6f0ca23638af 100644 --- a/sound/soc/amd/acp/acp-sof-mach.c +++ b/sound/soc/amd/acp/acp-sof-mach.c @@ -112,16 +112,14 @@ static int acp_sof_probe(struct platform_device *pdev) if (dmi_id && dmi_id->driver_data) acp_card_drvdata->tdm_mode = dmi_id->driver_data;
- acp_sofdsp_dai_links_create(card); + ret = acp_sofdsp_dai_links_create(card); + if (ret) + return dev_err_probe(&pdev->dev, ret, "Failed to create DAI links\n");
ret = devm_snd_soc_register_card(&pdev->dev, card); - if (ret) { - dev_err(&pdev->dev, - "devm_snd_soc_register_card(%s) failed: %d\n", - card->name, ret); - return ret; - } - + if (ret)
Do we need to undo acp_sofdsp_dai_links_create() in here? If not, please add a trivial note in the commit message.
No need to undo, will update the commit as suggested.
With that the series is: Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
Thanks for reviewing, Cristian
HTH o/ -Emil
Driver uses kasprintf() to initialize fw_{code,data}_bin members of struct acp_dev_data, but kfree() is never called to deallocate the memory, which results in a memory leak. Fix the issue by switching to devm_kasprintf(). Additionally, ensure the allocation was successful by checking the pointer validity. Fixes: f7da88003c53 ("ASoC: SOF: amd: Enable signed firmware image loading for Vangogh platform") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- sound/soc/sof/amd/acp.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c index 603ea5fc0d0d..c6f637f29847 100644 --- a/sound/soc/sof/amd/acp.c +++ b/sound/soc/sof/amd/acp.c @@ -547,17 +547,27 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev) adata->signed_fw_image = false; dmi_id = dmi_first_match(acp_sof_quirk_table); if (dmi_id && dmi_id->driver_data) { - adata->fw_code_bin = kasprintf(GFP_KERNEL, "%s/sof-%s-code.bin", - plat_data->fw_filename_prefix, - chip->name); - adata->fw_data_bin = kasprintf(GFP_KERNEL, "%s/sof-%s-data.bin", - plat_data->fw_filename_prefix, - chip->name); - adata->signed_fw_image = dmi_id->driver_data; + adata->fw_code_bin = devm_kasprintf(sdev->dev, GFP_KERNEL, + "%s/sof-%s-code.bin", + plat_data->fw_filename_prefix, + chip->name); + if (!adata->fw_code_bin) { + ret = -ENOMEM; + goto free_ipc_irq; + } + + adata->fw_data_bin = devm_kasprintf(sdev->dev, GFP_KERNEL, + "%s/sof-%s-data.bin", + plat_data->fw_filename_prefix, + chip->name); + if (!adata->fw_data_bin) { + ret = -ENOMEM; + goto free_ipc_irq; + } - dev_dbg(sdev->dev, "fw_code_bin:%s, fw_data_bin:%s\n", adata->fw_code_bin, - adata->fw_data_bin); + adata->signed_fw_image = dmi_id->driver_data; } + adata->enable_fw_debug = enable_fw_debug; acp_memory_init(sdev); -- 2.43.0
participants (4)
-
Cristian Ciocaltea -
Emil Velikov -
Péter Ujfalusi -
Venkata Prasad Potturu