| Message ID | 20190719170610.17610-7-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA/HDA: abort probe when DMICs are detected | expand |
On Fri, 19 Jul 2019 19:06:10 +0200, Pierre-Louis Bossart wrote: > > +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_dbg(&pci->dev, "Digital mics found on Skylake+ platform, aborting probe\n"); IMO, this can be verbose, dev_info() would be suitable. Otherwise user has no idea why the module load is skipped. > @@ -2055,6 +2078,17 @@ static int azx_probe(struct pci_dev *pci, > card->private_data = chip; > hda = container_of(chip, struct hda_intel, chip); > > + /* > + * stop probe if digital microphones detected on Skylake+ platform > + * with the DSP enabled. This is an opt-in behavior defined at build > + * time or at run-time with a module parameter > + */ > + if (IS_ENABLED(CONFIG_SND_HDA_INTEL_DETECT_DMIC) || dmic_detect >= 0) { Isn't it "dmic_detect != 0" ? Otherwise passing dmic_detect=0 would be treated as positive here. thanks, Takashi
On 7/19/19 1:13 PM, Takashi Iwai wrote: > On Fri, 19 Jul 2019 19:06:10 +0200, > Pierre-Louis Bossart wrote: >> >> +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_dbg(&pci->dev, "Digital mics found on Skylake+ platform, aborting probe\n"); > > IMO, this can be verbose, dev_info() would be suitable. > Otherwise user has no idea why the module load is skipped. sure, will do. > > >> @@ -2055,6 +2078,17 @@ static int azx_probe(struct pci_dev *pci, >> card->private_data = chip; >> hda = container_of(chip, struct hda_intel, chip); >> >> + /* >> + * stop probe if digital microphones detected on Skylake+ platform >> + * with the DSP enabled. This is an opt-in behavior defined at build >> + * time or at run-time with a module parameter >> + */ >> + if (IS_ENABLED(CONFIG_SND_HDA_INTEL_DETECT_DMIC) || dmic_detect >= 0) { > > Isn't it "dmic_detect != 0" ? Otherwise passing dmic_detect=0 would > be treated as positive here. Ah, good catch. I literally copied the enable_msi example here, which relies on >= 0. if (enable_msi >= 0) { chip->msi = !!enable_msi; return; } Not sure what the intention was here. Using dmic_detect != 0 wouldn't work for the default -1 value, maybe dmic_detect > 0 is probably a better solution?
On Fri, 19 Jul 2019 20:29:10 +0200, Pierre-Louis Bossart wrote: > > > >> @@ -2055,6 +2078,17 @@ static int azx_probe(struct pci_dev *pci, > >> card->private_data = chip; > >> hda = container_of(chip, struct hda_intel, chip); > >> + /* > >> + * stop probe if digital microphones detected on Skylake+ platform > >> + * with the DSP enabled. This is an opt-in behavior defined at build > >> + * time or at run-time with a module parameter > >> + */ > >> + if (IS_ENABLED(CONFIG_SND_HDA_INTEL_DETECT_DMIC) || dmic_detect >= 0) { > > > > Isn't it "dmic_detect != 0" ? Otherwise passing dmic_detect=0 would > > be treated as positive here. > > Ah, good catch. I literally copied the enable_msi example here, which > relies on >= 0. > > if (enable_msi >= 0) { > chip->msi = !!enable_msi; > return; > } > > Not sure what the intention was here. The MSI-enablement depends on the platform. enable_msi >=0 means that user passed module option explicitly so we follow that. Otherwise, let the system chooses whether to enable MSI or not. > Using dmic_detect != 0 wouldn't work for the default -1 value, > maybe dmic_detect > 0 is probably a better solution? In your case, the logic doesn't look like the dynamically platform dependent, so it should be simpler as below: static bool dmic_detect = IS_ENABLED(CONFIG_SND_HDA_INTEL_DETECT_DMIC); module_param(dmic_detect, bool, 0444); and evaluate it if (dmic_detect) { ret = azx_check_dmic(); .... That is, if Kconfig is set, it's per default enabled, but user can still turn it off via option. Otherwise user needs to enable it via option. Takashi
On 7/19/19 1:55 PM, Takashi Iwai wrote: > On Fri, 19 Jul 2019 20:29:10 +0200, > Pierre-Louis Bossart wrote: >> >> >>>> @@ -2055,6 +2078,17 @@ static int azx_probe(struct pci_dev *pci, >>>> card->private_data = chip; >>>> hda = container_of(chip, struct hda_intel, chip); >>>> + /* >>>> + * stop probe if digital microphones detected on Skylake+ platform >>>> + * with the DSP enabled. This is an opt-in behavior defined at build >>>> + * time or at run-time with a module parameter >>>> + */ >>>> + if (IS_ENABLED(CONFIG_SND_HDA_INTEL_DETECT_DMIC) || dmic_detect >= 0) { >>> >>> Isn't it "dmic_detect != 0" ? Otherwise passing dmic_detect=0 would >>> be treated as positive here. >> >> Ah, good catch. I literally copied the enable_msi example here, which >> relies on >= 0. >> >> if (enable_msi >= 0) { >> chip->msi = !!enable_msi; >> return; >> } >> >> Not sure what the intention was here. > > The MSI-enablement depends on the platform. enable_msi >=0 means that > user passed module option explicitly so we follow that. Otherwise, > let the system chooses whether to enable MSI or not. > >> Using dmic_detect != 0 wouldn't work for the default -1 value, >> maybe dmic_detect > 0 is probably a better solution? > > In your case, the logic doesn't look like the dynamically platform > dependent, so it should be simpler as below: > > static bool dmic_detect = IS_ENABLED(CONFIG_SND_HDA_INTEL_DETECT_DMIC); > module_param(dmic_detect, bool, 0444); > > and evaluate it > > if (dmic_detect) { > ret = azx_check_dmic(); > .... > > That is, if Kconfig is set, it's per default enabled, but user can > still turn it off via option. Otherwise user needs to enable it via > option. Makes sense, thanks for the feedback. Will send a v2 shortly. Have a nice week-end.
diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index 35d934309cb2..b5966014b5f7 100644 --- a/sound/pci/hda/Kconfig +++ b/sound/pci/hda/Kconfig @@ -12,6 +12,7 @@ config SND_HDA_INTEL tristate "HD Audio PCI" depends on SND_PCI select SND_HDA + select SND_INTEL_NHLT if ACPI help Say Y here to include support for Intel "High Definition Audio" (Azalia) and its compatible devices. @@ -22,6 +23,15 @@ 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 cb8b0945547c..15244a06f634 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -46,6 +46,7 @@ #include <sound/initval.h> #include <sound/hdaudio.h> #include <sound/hda_i915.h> +#include <sound/intel-nhlt.h> #include <linux/vgaarb.h> #include <linux/vga_switcheroo.h> #include <linux/firmware.h> @@ -124,6 +125,7 @@ static char *patch[SNDRV_CARDS]; static bool beep_mode[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] = CONFIG_SND_HDA_INPUT_BEEP_MODE}; #endif +static int dmic_detect = -1; module_param_array(index, int, NULL, 0444); MODULE_PARM_DESC(index, "Index value for Intel HD audio interface."); @@ -158,6 +160,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, bint, 0444); +MODULE_PARM_DESC(dmic_detect, "DMIC detect on SKL+ platforms"); #ifdef CONFIG_PM static int param_set_xint(const char *val, const struct kernel_param *kp); @@ -2025,6 +2029,25 @@ 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_dbg(&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) { @@ -2055,6 +2078,17 @@ static int azx_probe(struct pci_dev *pci, card->private_data = chip; hda = container_of(chip, struct hda_intel, chip); + /* + * stop probe if digital microphones detected on Skylake+ platform + * with the DSP enabled. This is an opt-in behavior defined at build + * time or at run-time with a module parameter + */ + if (IS_ENABLED(CONFIG_SND_HDA_INTEL_DETECT_DMIC) || dmic_detect >= 0) { + err = azx_check_dmic(pci, chip); + if (err < 0) + goto out_free; + } + pci_set_drvdata(pci, card); err = register_vga_switcheroo(chip);
The legacy HD-Audio driver cannot handle Skylake+ platforms with digital microphones. For those platforms, the SOF or SST drivers need to be used. This patch provides an automatic way of detecting the presence of DMICs using NHTL information reported by the BIOS. A kernel kconfig option or a kernel module parameter provide an opt-in means of stopping the probe. The kernel would then look for an alternate driver registered for the same PCI ID to probe. With this capability, distros no longer have to blacklist snd-hda-intel, but still need to make sure the SOF/SST drivers are functional by providing the relevant firmware and topology files in /lib/firmware/intel The coexistence between SOF and SST drivers and their dynamic detection is not addressed by this patch, different mechanisms need to be used, e.g. DMI-based quirks. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- sound/pci/hda/Kconfig | 10 ++++++++++ sound/pci/hda/hda_intel.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+)