Re: [PATCH 27/35] ASoC: Intel: Skylake: Define platform descriptors

[Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>]

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@xxxxxxxxx>
> > > ---
> > >   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@xxxxxxxxxxxxxxxx
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel