ASoC: Intel: Skylake: Add alternative topology binary name
diff mbox series

Message ID 20200325122230.12172-1-mateusz.gorski@linux.intel.com
State New
Headers show
Series
  • ASoC: Intel: Skylake: Add alternative topology binary name
Related show

Commit Message

Gorski, Mateusz March 25, 2020, 12:22 p.m. UTC
This commit adds alternative topology binary file name based on used
machine driver and fallback to use this name after failed attempt to
load topology file with name based on NHLT.

Signed-off-by: Mateusz Gorski <mateusz.gorski@linux.intel.com>
---
 sound/soc/intel/skylake/skl-topology.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

Comments

Pierre-Louis Bossart March 25, 2020, 2:33 p.m. UTC | #1
On 3/25/20 7:22 AM, Mateusz Gorski wrote:
> This commit adds alternative topology binary file name based on used
> machine driver and fallback to use this name after failed attempt to
> load topology file with name based on NHLT.
> 
> Signed-off-by: Mateusz Gorski <mateusz.gorski@linux.intel.com>
> ---
>   sound/soc/intel/skylake/skl-topology.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
> index 69cd7a81bf2a..344b06df0e15 100644
> --- a/sound/soc/intel/skylake/skl-topology.c
> +++ b/sound/soc/intel/skylake/skl-topology.c
> @@ -3565,8 +3565,20 @@ int skl_tplg_init(struct snd_soc_component *component, struct hdac_bus *bus)
>   
>   	ret = request_firmware(&fw, skl->tplg_name, bus->dev);
>   	if (ret < 0) {
> -		dev_info(bus->dev, "tplg fw %s load failed with %d, falling back to dfw_sst.bin",
> -				skl->tplg_name, ret);
> +		char alt_tplg_name[64];
> +
> +		snprintf(alt_tplg_name, sizeof(alt_tplg_name), "%s-tplg.bin",
> +				skl->i2s_dev->name);

That's progress but is this complete?

skl->i2s_dev->name is the name of the machine driver, I don't see the 
part where this is modified to deal with the number of dmics?

In your topology patches, the names are: hda_dsp_noDMIC hda_dsp_DMIC_2ch 
hda_dsp_DMIC_4ch

How would the relevant file be found based on the number of DMICs on the 
platform? I must be missing something here?

> +		dev_info(bus->dev, "tplg fw %s load failed with %d, trying alternative tplg name %s",
> +				skl->tplg_name, ret, alt_tplg_name);
> +
> +		ret = request_firmware(&fw, alt_tplg_name, bus->dev);
> +		if (!ret)
> +			goto component_load;
> +
> +		dev_info(bus->dev, "tplg %s failed with %d, falling back to dfw_sst.bin",
> +				alt_tplg_name, ret);
> +
>   		ret = request_firmware(&fw, "dfw_sst.bin", bus->dev);
>   		if (ret < 0) {
>   			dev_err(bus->dev, "Fallback tplg fw %s load failed with %d\n",
> @@ -3575,6 +3587,9 @@ int skl_tplg_init(struct snd_soc_component *component, struct hdac_bus *bus)
>   		}
>   	}
>   
> +component_load:
> +
> +
>   	/*
>   	 * The complete tplg for SKL is loaded as index 0, we don't use
>   	 * any other index
>
Gorski, Mateusz March 26, 2020, 3 p.m. UTC | #2
> That's progress but is this complete?
>
> skl->i2s_dev->name is the name of the machine driver, I don't see the 
> part where this is modified to deal with the number of dmics?
>
> In your topology patches, the names are: hda_dsp_noDMIC 
> hda_dsp_DMIC_2ch hda_dsp_DMIC_4ch
>
> How would the relevant file be found based on the number of DMICs on 
> the platform? I must be missing something here?
>

The intention of this patch was not to deal with the different DMIC 
configurations problem. It only simplifies the topology binary 
selection. As you mentioned in one of previous mails, currently there 
are two options:

- name based on NHLT, which is pretty complicated, especially for end 
user, and is also depending on things like OEM name so the same topology 
will need to be renamed multiple times different devices (additionally, 
there are laptops on the market that do not have NHLT table at all)

- dfw_sst.bin, which is only a fallback binary name and should not be 
actually used because it could be misleading for users/distro intergrators

This change adds the third option, which is, in my opinion, the right 
way to deal with this problem. This name is simpler, does not depend onĀ  
existence of NHLT, and makes life easier for users/distros.


And as for the mentioned DMIC confguration problem - I am doing a 
research to find the simplest way to deal with this.


Mateusz
Pierre-Louis Bossart March 26, 2020, 3:26 p.m. UTC | #3
On 3/26/20 10:00 AM, Gorski, Mateusz wrote:
> 
>> That's progress but is this complete?
>>
>> skl->i2s_dev->name is the name of the machine driver, I don't see the 
>> part where this is modified to deal with the number of dmics?
>>
>> In your topology patches, the names are: hda_dsp_noDMIC 
>> hda_dsp_DMIC_2ch hda_dsp_DMIC_4ch
>>
>> How would the relevant file be found based on the number of DMICs on 
>> the platform? I must be missing something here?
>>
> 
> The intention of this patch was not to deal with the different DMIC 
> configurations problem. It only simplifies the topology binary 
> selection. As you mentioned in one of previous mails, currently there 
> are two options:
> 
> - name based on NHLT, which is pretty complicated, especially for end 
> user, and is also depending on things like OEM name so the same topology 
> will need to be renamed multiple times different devices (additionally, 
> there are laptops on the market that do not have NHLT table at all)
> 
> - dfw_sst.bin, which is only a fallback binary name and should not be 
> actually used because it could be misleading for users/distro intergrators
> 
> This change adds the third option, which is, in my opinion, the right 
> way to deal with this problem. This name is simpler, does not depend on 
> existence of NHLT, and makes life easier for users/distros.

Right, and that's fine to avoid the NHLT-name and dfw_sst.bin, no issue 
here. The point is to go one step further and require ZERO configuration 
from users.

> And as for the mentioned DMIC confguration problem - I am doing a 
> research to find the simplest way to deal with this.

Just append the number of mics detected to the topology file name? 
Asking users to copy/symlink hda_dsp_DMIC_2ch.tplg as hda-dsp.tplg 
doesn't really help, you can make things simpler.
Gorski, Mateusz April 2, 2020, 2:40 p.m. UTC | #4
>>
>> The intention of this patch was not to deal with the different DMIC 
>> configurations problem. It only simplifies the topology binary 
>> selection. As you mentioned in one of previous mails, currently there 
>> are two options:
>>
>> - name based on NHLT, which is pretty complicated, especially for end 
>> user, and is also depending on things like OEM name so the same 
>> topology will need to be renamed multiple times different devices 
>> (additionally, there are laptops on the market that do not have NHLT 
>> table at all)
>>
>> - dfw_sst.bin, which is only a fallback binary name and should not be 
>> actually used because it could be misleading for users/distro 
>> intergrators
>>
>> This change adds the third option, which is, in my opinion, the right 
>> way to deal with this problem. This name is simpler, does not depend 
>> on existence of NHLT, and makes life easier for users/distros.
>
> Right, and that's fine to avoid the NHLT-name and dfw_sst.bin, no 
> issue here. The point is to go one step further and require ZERO 
> configuration from users.
>
>> And as for the mentioned DMIC confguration problem - I am doing a 
>> research to find the simplest way to deal with this.
>
> Just append the number of mics detected to the topology file name? 
> Asking users to copy/symlink hda_dsp_DMIC_2ch.tplg as hda-dsp.tplg 
> doesn't really help, you can make things simpler.


Sent another patch to add path multiconfiguration feature to the driver. 
This way we will have one topology binary for multiple DMIC 
configurations, so we do not need to select different file according to 
its name. Also new version of topology conf was provided, the one with 
multiple configs.


Thanks,
Mateusz

Patch
diff mbox series

diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index 69cd7a81bf2a..344b06df0e15 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -3565,8 +3565,20 @@  int skl_tplg_init(struct snd_soc_component *component, struct hdac_bus *bus)
 
 	ret = request_firmware(&fw, skl->tplg_name, bus->dev);
 	if (ret < 0) {
-		dev_info(bus->dev, "tplg fw %s load failed with %d, falling back to dfw_sst.bin",
-				skl->tplg_name, ret);
+		char alt_tplg_name[64];
+
+		snprintf(alt_tplg_name, sizeof(alt_tplg_name), "%s-tplg.bin",
+				skl->i2s_dev->name);
+		dev_info(bus->dev, "tplg fw %s load failed with %d, trying alternative tplg name %s",
+				skl->tplg_name, ret, alt_tplg_name);
+
+		ret = request_firmware(&fw, alt_tplg_name, bus->dev);
+		if (!ret)
+			goto component_load;
+
+		dev_info(bus->dev, "tplg %s failed with %d, falling back to dfw_sst.bin",
+				alt_tplg_name, ret);
+
 		ret = request_firmware(&fw, "dfw_sst.bin", bus->dev);
 		if (ret < 0) {
 			dev_err(bus->dev, "Fallback tplg fw %s load failed with %d\n",
@@ -3575,6 +3587,9 @@  int skl_tplg_init(struct snd_soc_component *component, struct hdac_bus *bus)
 		}
 	}
 
+component_load:
+
+
 	/*
 	 * The complete tplg for SKL is loaded as index 0, we don't use
 	 * any other index