Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <6e52c5a2-24d5-422a-9a40-a0053729c98e@linux.intel.com>
Date:   Thu, 14 Dec 2023 12:48:13 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 07/11] ASoC: SOF: core: Skip firmware test for undefined
 fw_name
Content-Language: en-US
To:     Cristian Ciocaltea <cristian.ciocaltea@collabora.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        Bard Liao <yung-chuan.liao@linux.intel.com>,
        Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
        Daniel Baluta <daniel.baluta@nxp.com>,
        Kai Vehmanen <kai.vehmanen@linux.intel.com>,
        Venkata Prasad Potturu <venkataprasad.potturu@amd.com>,
        Alper Nebi Yasak <alpernebiyasak@gmail.com>,
        Syed Saba Kareem <Syed.SabaKareem@amd.com>,
        Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
        Marian Postevca <posteuca@mutex.one>,
        Vijendar Mukunda <Vijendar.Mukunda@amd.com>,
        V sujith kumar Reddy <Vsujithkumar.Reddy@amd.com>,
        Mastan Katragadda <Mastan.Katragadda@amd.com>,
        Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>
Cc:     linux-sound@vger.kernel.org, linux-kernel@vger.kernel.org,
        sound-open-firmware@alsa-project.org, kernel@collabora.com
References: <20231209205351.880797-1-cristian.ciocaltea@collabora.com>
 <20231209205351.880797-8-cristian.ciocaltea@collabora.com>
From:   =?UTF-8?Q?P=C3=A9ter_Ujfalusi?= <peter.ujfalusi@linux.intel.com>
In-Reply-To: <20231209205351.880797-8-cristian.ciocaltea@collabora.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


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