Re: [PATCH 27/35] ASoC: Intel: Skylake: Define platform descriptors - Pierre-Louis Bossart

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Cezary Rojewski <cezary.rojewski@intel.com>, alsa-devel@alsa-project.org
Cc: broonie@kernel.org, tiwai@suse.com, lgirdwood@gmail.com
Subject: Re: [PATCH 27/35] ASoC: Intel: Skylake: Define platform descriptors
Date: Mon, 26 Aug 2019 12:13:38 -0500	[thread overview]
Message-ID: <12a90456-b8a0-adc3-755a-cfee3124f177@linux.intel.com> (raw)
In-Reply-To: <29c6bb0d-c236-a6c7-9a59-efb7ec42e145@intel.com>

On 8/24/19 5:51 AM, Cezary Rojewski wrote:
> On 2019-08-23 21:50, Pierre-Louis Bossart wrote:
>>
>>
>> On 8/22/19 2:04 PM, Cezary Rojewski wrote:
>>> Make use of sst_pdata and declare platform descriptors for all existing
>>> cAVS platforms. Each carries information about base_fw filename,
>>> platform specific operations and boards supported.
>>
>> if you use a constant base_fw name that cannot be made board-specific 
>> for specific usages, you will restrict the ability to deal with quirks 
>> and custom cases.
>>
>> real-life example: not so long ago there were two SST firmwares for 
>> 'regular' solutions and ultra-low-latency ones, so by having a single 
>> name for all APL-based platforms you will generate issues that don't 
>> exist today, or you will force users to patch something in the core.
>>
> 
> I did not bring up ULL case for a reason. Guess Pandora box is to be 
> opened.. so be it.
> 
> ULL stands for Ultra Low Latency and it can be described by the following:
> - exists only for APL based platforms (more like single platform/ model 
> though)
> - in consequence, binary isn't present on any other FW branch and any 
> other platform apart from APL
> - its existence is tied to hardware.. eh.. let's call it a "limitation"
> - number of actual vendors is too Ultra Low..
> - has limited functionality and validation
> - is not the recommended FW for end users in any case
> - binary is not going to be upstreamed
> - reference board is not going to be upstreamed
> - generic (so called main FW) and ULL share the board ACPI ID and thus 
> require kernel .config to be modified -or- blacklist.conf with be updated
> - shares topology filename with generic (main) FW so user still has to 
> modify his /lib/firmware. Topology names are currently NHLT-based, built 
> from NHLT header data and platform id which are BIOS/ ABL and platform 
> specific respectively
> (...)

I would describe your answer as 'whatabout-ism'. Yes there are plenty of 
ways to screw-up, none of them is a justification for assuming that a 
single filename will work for everyone.

There are also plenty of good reasons to use a different fw and topology 
file name. Taking this capability away essentially corners users into 
non-upstreamed custom versions.

> TLDR:
> There is total of 0 people sitting in front of their monitors who are 
> consciously going to make use of ULL firmware.
> Any user that is going to, will have to play with their kconfig, 
> blacklist and replace existing topology file.

that's where you are making too many assumptions, if quirks and dynamic 
detection capabilities are provided then it's possible to have a single 
kernel build that will deal with multiple configurations.

> This is normally done by titanic build-bot which, among billion other 
> things, ensures /lib/firmware looks like it should given the configuration.
> 
> -
> 
> So, one could have provided a nice choice-box within menuconfig to 
> ensure only one board can be chosen.
> When one does it, one realizes both generic and ULL firmwares are not 
> actually tied to any specific board and with more boards (usecases) and 
> more kconfigs code gets bloated.
> 
> Moving further, guarding apl_desc with #if-else depending on some global 
> generic-vs-ULL configuration which would adjust said descriptors with 
> proper FW filename actually seems like a better solution..
> 
> ..and then kBOOM comes in and actual design pattern!
> Board should have been stated tplg_filename, not the fw_filename. Said 
> topology file contains manifest which tells host what libraries to load. 
> And thus, we clear the mist and see that one single field (which is 
> currently missing in snd_soc_acpi_mach) and some clever topology 
> manifest make it all happen: platform-board conflicts cease to exist.

I am not going to argue further. I've spent a lot of time making sure 
the same kernel build can be used across multiple platforms, if you want 
to stick to static custom configurations I am not interested in 
debating. I just hope your team has enough support folks to deal with 
all these configurations.

> 
> Czarek
> 
>>>
>>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>>> ---
>>>   sound/soc/intel/skylake/bxt-sst.c |  4 ++--
>>>   sound/soc/intel/skylake/cnl-sst.c |  4 ++--
>>>   sound/soc/intel/skylake/skl-sst.c |  4 ++--
>>>   sound/soc/intel/skylake/skl.c     | 38 ++++++++++++++++++++++++++++++-
>>>   sound/soc/intel/skylake/skl.h     |  3 +++
>>>   5 files changed, 46 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/sound/soc/intel/skylake/bxt-sst.c 
>>> b/sound/soc/intel/skylake/bxt-sst.c
>>> index a547fb84eee9..06da822790a5 100644
>>> --- a/sound/soc/intel/skylake/bxt-sst.c
>>> +++ b/sound/soc/intel/skylake/bxt-sst.c
>>> @@ -531,7 +531,7 @@ static const struct skl_dsp_fw_ops bxt_fw_ops = {
>>>       .load_library = bxt_load_library,
>>>   };
>>> -static struct sst_ops skl_ops = {
>>> +struct sst_ops apl_sst_ops = {
>>>       .irq_handler = skl_dsp_sst_interrupt,
>>>       .thread_fn = skl_dsp_irq_thread_handler,
>>>       .write = sst_shim32_write,
>>> @@ -542,7 +542,7 @@ static struct sst_ops skl_ops = {
>>>   };
>>>   static struct sst_pdata skl_dev = {
>>> -    .ops = &skl_ops,
>>> +    .ops = &apl_sst_ops,
>>>   };
>>>   int bxt_sst_dsp_init(struct skl_dev *skl, const char *fw_name)
>>> diff --git a/sound/soc/intel/skylake/cnl-sst.c 
>>> b/sound/soc/intel/skylake/cnl-sst.c
>>> index 5be0a8eb154d..c4dbf6655097 100644
>>> --- a/sound/soc/intel/skylake/cnl-sst.c
>>> +++ b/sound/soc/intel/skylake/cnl-sst.c
>>> @@ -408,7 +408,7 @@ static int cnl_ipc_init(struct device *dev, 
>>> struct skl_dev *cnl)
>>>       return 0;
>>>   }
>>> -static struct sst_ops cnl_ops = {
>>> +struct sst_ops cnl_sst_ops = {
>>>       .irq_handler = cnl_dsp_sst_interrupt,
>>>       .thread_fn = cnl_dsp_irq_thread_handler,
>>>       .write = sst_shim32_write,
>>> @@ -419,7 +419,7 @@ static struct sst_ops cnl_ops = {
>>>   };
>>>   static struct sst_pdata cnl_dev = {
>>> -    .ops = &cnl_ops,
>>> +    .ops = &cnl_sst_ops,
>>>   };
>>>   int cnl_sst_dsp_init(struct skl_dev *cnl, const char *fw_name)
>>> diff --git a/sound/soc/intel/skylake/skl-sst.c 
>>> b/sound/soc/intel/skylake/skl-sst.c
>>> index 8ae7fe73534e..122c07290440 100644
>>> --- a/sound/soc/intel/skylake/skl-sst.c
>>> +++ b/sound/soc/intel/skylake/skl-sst.c
>>> @@ -503,7 +503,7 @@ static const struct skl_dsp_fw_ops skl_fw_ops = {
>>>       .unload_mod = skl_unload_module,
>>>   };
>>> -static struct sst_ops skl_ops = {
>>> +struct sst_ops skl_sst_ops = {
>>>       .irq_handler = skl_dsp_sst_interrupt,
>>>       .write = sst_shim32_write,
>>>       .read = sst_shim32_read,
>>> @@ -513,7 +513,7 @@ static struct sst_ops skl_ops = {
>>>   };
>>>   static struct sst_pdata skl_dev = {
>>> -    .ops = &skl_ops,
>>> +    .ops = &skl_sst_ops,
>>>   };
>>>   int skl_sst_dsp_init(struct skl_dev *skl, const char *fw_name)
>>> diff --git a/sound/soc/intel/skylake/skl.c 
>>> b/sound/soc/intel/skylake/skl.c
>>> index 54e1f957121d..d6d099aba834 100644
>>> --- a/sound/soc/intel/skylake/skl.c
>>> +++ b/sound/soc/intel/skylake/skl.c
>>> @@ -27,6 +27,7 @@
>>>   #include <sound/hda_i915.h>
>>>   #include <sound/hda_codec.h>
>>>   #include <sound/intel-nhlt.h>
>>> +#include "../common/sst-dsp.h"
>>>   #include "skl.h"
>>>   #include "skl-sst-dsp.h"
>>>   #include "skl-sst-ipc.h"
>>> @@ -1063,7 +1064,6 @@ static int skl_probe(struct pci_dev *pci,
>>>       pci_set_drvdata(skl->pci, bus);
>>> -
>>>       err = skl_find_machine(skl, (void *)pci_id->driver_data);
>>>       if (err < 0) {
>>>           dev_err(bus->dev, "skl_find_machine failed with err: %d\n", 
>>> err);
>>> @@ -1153,6 +1153,42 @@ static void skl_remove(struct pci_dev *pci)
>>>       dev_set_drvdata(&pci->dev, NULL);
>>>   }
>>> +static struct sst_pdata skl_desc = {
>>> +    .fw_name = "intel/dsp_fw_release.bin",
>>> +    .ops = &skl_sst_ops,
>>> +    .boards = snd_soc_acpi_intel_skl_machines,
>>> +};
>>> +
>>> +static struct sst_pdata kbl_desc = {
>>> +    .fw_name = "intel/dsp_fw_kbl.bin",
>>> +    .ops = &skl_sst_ops,
>>> +    .boards = snd_soc_acpi_intel_kbl_machines,
>>> +};
>>> +
>>> +static struct sst_pdata apl_desc = {
>>> +    .fw_name = "intel/dsp_fw_bxtn.bin",
>>> +    .ops = &apl_sst_ops,
>>> +    .boards = snd_soc_acpi_intel_bxt_machines,
>>> +};
>>> +
>>> +static struct sst_pdata glk_desc = {
>>> +    .fw_name = "intel/dsp_fw_glk.bin",
>>> +    .ops = &apl_sst_ops,
>>> +    .boards = snd_soc_acpi_intel_glk_machines,
>>> +};
>>> +
>>> +static struct sst_pdata cnl_desc = {
>>> +    .fw_name = "intel/dsp_fw_cnl.bin",
>>> +    .ops = &cnl_sst_ops,
>>> +    .boards = snd_soc_acpi_intel_cnl_machines,
>>> +};
>>> +
>>> +static struct sst_pdata icl_desc = {
>>> +    .fw_name = "intel/dsp_fw_icl.bin",
>>> +    .ops = &cnl_sst_ops,
>>> +    .boards = snd_soc_acpi_intel_icl_machines,
>>> +};
>>> +
>>>   /* PCI IDs */
>>>   static const struct pci_device_id skl_ids[] = {
>>>   #if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKL)
>>> diff --git a/sound/soc/intel/skylake/skl.h 
>>> b/sound/soc/intel/skylake/skl.h
>>> index 9f5aa53df9f8..2f2b5a141abf 100644
>>> --- a/sound/soc/intel/skylake/skl.h
>>> +++ b/sound/soc/intel/skylake/skl.h
>>> @@ -42,6 +42,9 @@
>>>   #define AZX_REG_VS_EM2_L1SEN        BIT(13)
>>>   struct skl_debug;
>>> +extern struct sst_ops skl_sst_ops;
>>> +extern struct sst_ops apl_sst_ops;
>>> +extern struct sst_ops cnl_sst_ops;
>>>   struct skl_astate_param {
>>>       u32 kcps;
>>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel