Dne 02. 10. 19 v 18:00 Pierre-Louis Bossart napsal(a):
> Thanks Jaroslav, see comments below.
>
> No real objections on the concept, but we should fold NODSP/LEGACY
> together, deal with all the PCI IDs handled by SOF and deal with the
> cases where we want the SKL driver (SKL/KBL/APL Chromebooks with DMICs)
Thank you for your comment. It was just a first shot to resolve the detection
issue properly.
>> diff --git a/include/sound/intel-dsp-config.h b/include/sound/intel-dsp-config.h
>> new file mode 100644
>> index 000000000000..700f86282a83
>> --- /dev/null
>> +++ b/include/sound/intel-dsp-config.h
>> @@ -0,0 +1,35 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * intel-dsp-config.h - Intel DSP config
>> + *
>> + * Copyright (c) 2019 Jaroslav Kysela <perex@xxxxxxxx>
>> + */
>> +
>> +#ifndef __INTEL_DSP_CONFIG_H__
>> +#define __INTEL_DSP_CONFIG_H__
>> +
>> +struct pci_dev;
>> +
>> +enum {
>> + SND_INTEL_DSP_DRIVER_ANY = 0,
>> + SND_INTEL_DSP_DRIVER_NODSP,
>> + SND_INTEL_DSP_DRIVER_LEGACY,
>
> not sure what 'LEGACY' means here? This is really the same as NODSP.
>
>> + SND_INTEL_DSP_DRIVER_SST,
>> + SND_INTEL_DSP_DRIVER_SOF,
>> + SND_INTEL_DSP_DRIVER_LAST = SND_INTEL_DSP_DRIVER_SOF
>> +};
>
>> +static int dsp_driver;
>> +
>> +module_param(dsp_driver, int, 0444);
>> +MODULE_PARM_DESC(dsp_driver, "Force the DSP driver for Intel DSP (0=auto, 1=noDSP, 2=legacy, 3=SST, 4=SOF)");
>
> Same here, noDSP==legacy.
I just had a good feeling to add another state where we know that the DSP is
not present. But yes, for the driver selection it's an extra state and
everything will fall-back to the legacy hda driver.
>> +
>> +static const u16 sof_skl_table[] = {
>
> we should remove the reference to SKL, this is historical and there are
> already 2 generations that have little to do with SKL.
>
>> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_COMETLAKE_LP)
>> + 0x02c8, /* Cometlake-LP */
>> +#endif
>> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_COMETLAKE_H)
>> + 0x06c8, /* Cometlake-H */
>> +#endif
>> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_GEMINILAKE)
>> + 0x3198, /* Geminilake */
>> +#endif
>> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_APOLLOLAKE)
>> + 0x5a98, /* Broxton-P (Appololake) */
>> +#endif
>> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_CANNONLAKE)
>> + 0x9dc8, /* Cannonlake */
>> +#endif
>> +#if IS_ENABLED(CONFIG_SND_SOC_SOF_COFFEELAKE)
>> + 0xa348, /* Coffelake */
>> +#endif
>
> What about all the other PCI IDs that SOF handles, e.g. TigerLake, etc?
There is no PCI ID clash, only one driver will call the DSP probe and
SND_INTEL_DSP_DRIVER_ANY will be returned in this case.
> Also how do you deal with SKL/KBL cases with DMICs? They would need to
> use the SST driver (for Chromebooks mainly)
As I noted in the comment, we can add dmi quirks on top. I just do not have
enough information - can I take the hints from the pull request (your code)
you already mentioned?
> Even for APL, the 'official' driver is still SST for Chromebooks. SOF
> should work but there will probably be missing firmware/topology files.
I can rework this part of course. I'll send v2 patch.
>> +};
>> +
>> +static int snd_intel_dsp_check_device(u16 device, const u16 *table, u32 len)
>> +{
>> + for (; len > 0; len--, table++) {
>> + if (*table == device)
>> + return 1;
>> + }
>> + return 0;
>> +}
>> +
>> +static int snd_intel_dsp_check_dmic(struct pci_dev *pci)
>> +{
>> + struct nhlt_acpi_table *nhlt;
>> + int ret = 0;
>> +
>> + if (snd_intel_dsp_check_device(pci->device, sof_skl_table, ARRAY_SIZE(sof_skl_table))) {
>> + nhlt = intel_nhlt_init(&pci->dev);
>> + if (nhlt) {
>> + if (intel_nhlt_get_dmic_geo(&pci->dev, nhlt))
>> + ret = 1;
>> + intel_nhlt_free(nhlt);
>> + }
>> + }
>> + return ret;
>> +}
>> +
>> +int snd_intel_dsp_driver_probe(struct pci_dev *pci)
>> +{
>> + if (dsp_driver > 0 && dsp_driver <= SND_INTEL_DSP_DRIVER_LAST)
>> + return dsp_driver;
>> +
>> + /* Intel vendor only */
>> + if (snd_BUG_ON(pci->vendor != 0x8086))
>> + return SND_INTEL_DSP_DRIVER_ANY;
>> +
>> + /*
>> + * detect DSP by checking class/subclass/prog-id information
>> + * class=04 subclass 03 prog-if 00: no DSP, use legacy driver
>> + * class=04 subclass 01 prog-if 00: DSP is present
>> + * (and may be required e.g. for DMIC or SSP support)
>> + * class=04 subclass 03 prog-if 80: use DSP or legacy mode
>> + */
>> + if (pci->class == 0x040300)
>> + return SND_INTEL_DSP_DRIVER_NODSP;
>> + if (pci->class != 0x040100 && pci->class != 0x040380) {
>> + dev_err(&pci->dev, "Unknown PCI class/subclass/prog-if information (0x%06x) found, selecting HDA legacy driver\n", pci->class);
>> + return SND_INTEL_DSP_DRIVER_LEGACY;
>
> Again these two cases are the same. The DSP is not enabled so the
> HDaudio legacy driver needs to be used.
>
>> + }
>> +
>> + dev_info(&pci->dev, "DSP detected with PCI class/subclass/prog-if info 0x%06x\n", pci->class);
>> +
>> + /* DMIC check for Skylake+ */
>> + if (snd_intel_dsp_check_dmic(pci)) {
>> + dev_info(&pci->dev, "Digital mics found on Skylake+ platform, using SOF driver\n");
>> + return SND_INTEL_DSP_DRIVER_SOF;
>
> That's not fully correct, see comment above on Chromebooks.
And it's really visible now at least :-) I welcome any hints what's wrong.
>> + }
>> +
>> + return SND_INTEL_DSP_DRIVER_ANY;
>> +}
>> +
>> +EXPORT_SYMBOL_GPL(snd_intel_dsp_driver_probe);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("Intel DSP config driver");
>> diff --git a/sound/hda/intel-nhlt.c b/sound/hda/intel-nhlt.c
>> index daede96f28ee..097ff6c10099 100644
>> --- a/sound/hda/intel-nhlt.c
>> +++ b/sound/hda/intel-nhlt.c
>> @@ -102,6 +102,3 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt)
>> return dmic_geo;
>> }
>> EXPORT_SYMBOL_GPL(intel_nhlt_get_dmic_geo);
>> -
>> -MODULE_LICENSE("GPL v2");
>> -MODULE_DESCRIPTION("Intel NHLT driver");
>> diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
>> index dae47a45b2b8..bd48335d09d7 100644
>> --- a/sound/pci/hda/Kconfig
>> +++ b/sound/pci/hda/Kconfig
>> @@ -12,7 +12,7 @@ config SND_HDA_INTEL
>> tristate "HD Audio PCI"
>> depends on SND_PCI
>> select SND_HDA
>> - select SND_INTEL_NHLT if ACPI
>> + select SND_INTEL_DSP_CONFIG
>> help
>> Say Y here to include support for Intel "High Definition
>> Audio" (Azalia) and its compatible devices.
>> @@ -23,15 +23,6 @@ config SND_HDA_INTEL
>> To compile this driver as a module, choose M here: the module
>> will be called snd-hda-intel.
>>
>> -config SND_HDA_INTEL_DETECT_DMIC
>> - bool "DMIC detection and probe abort"
>> - depends on SND_HDA_INTEL
>> - help
>> - Say Y to detect digital microphones on SKL+ devices. DMICs
>> - cannot be handled by the HDaudio legacy driver and are
>> - currently only supported by the SOF driver.
>> - If unsure say N.
>> -
>> config SND_HDA_TEGRA
>> tristate "NVIDIA Tegra HD Audio"
>> depends on ARCH_TEGRA
>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
>> index 240f4ca76391..c76c2deea47c 100644
>> --- a/sound/pci/hda/hda_intel.c
>> +++ b/sound/pci/hda/hda_intel.c
>> @@ -46,7 +46,7 @@
>> #include <sound/initval.h>
>> #include <sound/hdaudio.h>
>> #include <sound/hda_i915.h>
>> -#include <sound/intel-nhlt.h>
>> +#include <sound/intel-dsp-config.h>
>> #include <linux/vgaarb.h>
>> #include <linux/vga_switcheroo.h>
>> #include <linux/firmware.h>
>> @@ -124,7 +124,7 @@ static char *patch[SNDRV_CARDS];
>> static bool beep_mode[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] =
>> CONFIG_SND_HDA_INPUT_BEEP_MODE};
>> #endif
>> -static bool dmic_detect = IS_ENABLED(CONFIG_SND_HDA_INTEL_DETECT_DMIC);
>> +static bool no_dsp_driver;
>>
>> module_param_array(index, int, NULL, 0444);
>> MODULE_PARM_DESC(index, "Index value for Intel HD audio interface.");
>> @@ -159,8 +159,8 @@ module_param_array(beep_mode, bool, NULL, 0444);
>> MODULE_PARM_DESC(beep_mode, "Select HDA Beep registration mode "
>> "(0=off, 1=on) (default=1).");
>> #endif
>> -module_param(dmic_detect, bool, 0444);
>> -MODULE_PARM_DESC(dmic_detect, "DMIC detect on SKL+ platforms");
>> +module_param(no_dsp_driver, bool, 0444);
>> +MODULE_PARM_DESC(no_dsp_driver, "Do not return from probe when another DSP driver claims device");
>
> maybe: "force this driver to be used and bypass DSP driver selection"
>
>>
>> #ifdef CONFIG_PM
>> static int param_set_xint(const char *val, const struct kernel_param *kp);
>> @@ -2020,25 +2020,6 @@ static const struct hda_controller_ops pci_hda_ops = {
>> .position_check = azx_position_check,
>> };
>>
>> -static int azx_check_dmic(struct pci_dev *pci, struct azx *chip)
>> -{
>> - struct nhlt_acpi_table *nhlt;
>> - int ret = 0;
>> -
>> - if (chip->driver_type == AZX_DRIVER_SKL &&
>> - pci->class != 0x040300) {
>> - nhlt = intel_nhlt_init(&pci->dev);
>> - if (nhlt) {
>> - if (intel_nhlt_get_dmic_geo(&pci->dev, nhlt)) {
>> - ret = -ENODEV;
>> - dev_info(&pci->dev, "Digital mics found on Skylake+ platform, aborting probe\n");
>> - }
>> - intel_nhlt_free(nhlt);
>> - }
>> - }
>> - return ret;
>> -}
>> -
>> static int azx_probe(struct pci_dev *pci,
>> const struct pci_device_id *pci_id)
>> {
>> @@ -2056,6 +2037,17 @@ static int azx_probe(struct pci_dev *pci,
>> return -ENOENT;
>> }
>>
>> + /*
>> + * stop probe if another Intel's DSP driver should be activated
>> + */
>> + if (!no_dsp_driver) {
>> + err = snd_intel_dsp_driver_probe(pci);
>> + if (err != SND_INTEL_DSP_DRIVER_ANY &&
>> + err != SND_INTEL_DSP_DRIVER_NODSP &&
>> + err != SND_INTEL_DSP_DRIVER_LEGACY)
>
> merge these last two
>
>> + return -ENODEV;
>> + }
>> +
>
> [snip]
>
>> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
>> index 141dbbf975ac..58ba3e9469ba 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 <sound/intel-dsp-config.h>
>> #include "skl.h"
>> #include "skl-sst-dsp.h"
>> #include "skl-sst-ipc.h"
>> @@ -987,22 +988,10 @@ static int skl_probe(struct pci_dev *pci,
>>
>> switch (skl_pci_binding) {
>> case SND_SKL_PCI_BIND_AUTO:
>> - /*
>> - * detect DSP by checking class/subclass/prog-id information
>> - * class=04 subclass 03 prog-if 00: no DSP, use legacy driver
>> - * class=04 subclass 01 prog-if 00: DSP is present
>> - * (and may be required e.g. for DMIC or SSP support)
>> - * class=04 subclass 03 prog-if 80: use DSP or legacy mode
>> - */
>> - if (pci->class == 0x040300) {
>> - dev_info(&pci->dev, "The DSP is not enabled on this platform, aborting probe\n");
>> + err = snd_intel_dsp_driver_probe(pci);
>> + if (err != SND_INTEL_DSP_DRIVER_ANY &&
>> + err != SND_INTEL_DSP_DRIVER_SST)
>
> I didn't see a case where SST was selected?
>
>> return -ENODEV;
>> - }
>> - if (pci->class != 0x040100 && pci->class != 0x040380) {
>> - dev_err(&pci->dev, "Unknown PCI class/subclass/prog-if information (0x%06x) found, aborting probe\n", pci->class);
>> - return -ENODEV;
>> - }
>> - dev_info(&pci->dev, "DSP detected with PCI class/subclass/prog-if info 0x%06x\n", pci->class);
>> break;
>> case SND_SKL_PCI_BIND_LEGACY:
>> dev_info(&pci->dev, "Module parameter forced binding with HDaudio legacy, aborting probe\n");
>
> Do we still need this skl_pci_binding now? I think it's remaining code
> from the time when we only used the PCI class, which led to breaking
> Linus' laptop (and others)...
>
>> diff --git a/sound/soc/sof/intel/Kconfig b/sound/soc/sof/intel/Kconfig
>> index 479ba249e219..8a5d5c0f95f2 100644
>> --- a/sound/soc/sof/intel/Kconfig
>> +++ b/sound/soc/sof/intel/Kconfig
>> @@ -286,7 +286,7 @@ config SND_SOC_SOF_HDA
>> tristate
>> select SND_HDA_EXT_CORE if SND_SOC_SOF_HDA_LINK
>> select SND_SOC_HDAC_HDA if SND_SOC_SOF_HDA_AUDIO_CODEC
>> - select SND_INTEL_NHLT if ACPI
>> + select SND_INTEL_DSP_CONFIG
>> help
>> This option is not user-selectable but automagically handled by
>> 'select' statements at a higher level
>> diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c
>> index d66412a77873..3a9e0e2a150d 100644
>> --- a/sound/soc/sof/sof-pci-dev.c
>> +++ b/sound/soc/sof/sof-pci-dev.c
>> @@ -12,6 +12,7 @@
>> #include <linux/module.h>
>> #include <linux/pci.h>
>> #include <linux/pm_runtime.h>
>> +#include <sound/intel-dsp-config.h>
>> #include <sound/soc-acpi.h>
>> #include <sound/soc-acpi-intel-match.h>
>> #include <sound/sof.h>
>> @@ -277,6 +278,11 @@ static int sof_pci_probe(struct pci_dev *pci,
>> const struct snd_sof_dsp_ops *ops;
>> int ret;
>>
>> + ret = snd_intel_dsp_driver_probe(pci);
>> + if (ret != SND_INTEL_DSP_DRIVER_ANY &&
>> + ret != SND_INTEL_DSP_DRIVER_SOF)
>> + return -ENODEV;
>> +
>> dev_dbg(&pci->dev, "PCI DSP detected");
>>
>> /* get ops for platform */
>>
--
Jaroslav Kysela <perex@xxxxxxxx>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
