Dne 07. 10. 19 v 3:50 Takashi Iwai napsal(a):
On Sun, 06 Oct 2019 17:22:32 +0200, Jaroslav Kysela wrote:
For distributions, we need one place where we can decide which driver will be activated for the auto-configation of the Intel's HDA hardware with DSP. Actually, we cover three drivers:
- Legacy HDA
- Intel SST
- Intel Sound Open Firmware (SOF)
All those drivers registers similar PCI IDs, so the first driver probed from the PCI stack can win. But... it is not guaranteed that the correct driver wins.
This commit changes Intel's NHLT ACPI module to a common DSP probe module for the Intel's hardware. All above sound drivers calls this code. The user can force another behaviour using the module parameter 'dsp_driver' located in the 'snd-intel-dspcfg' module.
This change allows to add specific dmi checks for the specific systems. The examples are taken from the pull request:
https://github.com/thesofproject/linux/pull/927
Tested on Lenovo Carbon X1 7th gen.
Signed-off-by: Jaroslav Kysela perex@perex.cz Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Cc: Cezary Rojewski cezary.rojewski@intel.com
Since I've been still traveling, just a few nitpicking:
--- a/sound/hda/Makefile +++ b/sound/hda/Makefile @@ -14,5 +14,8 @@ obj-$(CONFIG_SND_HDA_CORE) += snd-hda-core.o #extended hda obj-$(CONFIG_SND_HDA_EXT_CORE) += ext/
-snd-intel-nhlt-objs := intel-nhlt.o -obj-$(CONFIG_SND_INTEL_NHLT) += snd-intel-nhlt.o +snd-intel-dspcfg-objs := intel-dsp-config.o +ifneq ($(CONFIG_ACPI),)
- snd-intel-dspcfg-objs += intel-nhlt.o
+endif
This can be simplified by snd-intel-dspcfg-$(CONFIG_SND_INTEL_NHLT) += intel-nhlt.o
Yes, I added the ACPI condition later and didn't return back to Makefile. Fixed in v4 (will post after comments from Pierre-Louis).
--- /dev/null +++ b/sound/hda/intel-dsp-config.c @@ -0,0 +1,247 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2019 Jaroslav Kysela perex@perex.cz
+#include <sound/core.h> +#include <linux/module.h> +#include <linux/pci.h> +#include <linux/dmi.h> +#include <sound/intel-nhlt.h> +#include <sound/intel-dsp-config.h>
Please put sound/core.h after linux/*.h inclusions. Also in general the inclusions are arranged in the alphabetical order nowadays.
Uff. Rules and rules for rules. Will fix that in v4.
+int snd_intel_dsp_driver_probe(struct pci_dev *pci) +{
- const struct config_entry *cfg;
- 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_LEGACY;- 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;- }
If we treat this as an error, we should provide a code to work around this (e.g. quirk entry or such), so that the error can be avoided later.
It' s really an error. This path should not be executed with the known, functional hardware. The user might use dsp_driver module parameter to change the behaviour.
--- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c
....
@@ -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, "Force this driver to be used and bypass the DSP driver selection");
A negative option is often confusing, e.g. you may pass like no_dsp_driver=no. Better to use a positive form if possible.
I just tried to copy the dmic_detect value, but yes, it might be confusion. I will change that in v4.
Jaroslav