[alsa-devel] [PATCH v2 0/2] ALSA: HDAudio: enable dynamic selection between legacy and Skylake drivers
Use detection logic based on the PCI class/subclass/prog-if in the HDaudio legacy probe, and add a module parameter to bypass or force the automatic detection should it ever fail (or in case of missing firmware or topology files)
This patchset was tested on 1. an SKL HP device with the DSP disabled 2. a WHL Acer Swift3 laptop 3. APL NUC6 4. KBL NUC7
5 reboot tests were done for each device with the DSP presence detected reliably. The "pci_binding" parameter was also tested on the WHL device.
The dynamic selection adds about 5ms worst case.
[ 2.813741] snd_hda_intel 0000:00:1f.3: The DSP is enabled on this platform, aborting probe [ 2.850588] snd_soc_skl 0000:00:1f.3: DSP detected with PCI class/subclass/prog-if info 0x040100
The SOF driver will use exactly the same mechanism, and add a mutual exclusion with the Skylake/SST driver to avoid having 3 drivers for the same PCI ID. Two is complicated enough.
Changes since v1 (Feedback from Takashi) a) added explanations and renamed Kconfigs to SND_HDA_INTEL_DSP_DETECTION_XYZ b) less verbose dev_info c) fixed pci_binding parameter
Pierre-Louis Bossart (2): ALSA: HD-Audio: SKL+: abort probe if DSP is present and Skylake driver selected ALSA: HD-Audio: SKL+: force HDaudio legacy or SKL+ driver selection
include/sound/hdaudio.h | 6 ++++ sound/pci/hda/Kconfig | 62 ++++++++++++++++++++++++++++++++++ sound/pci/hda/hda_controller.h | 2 +- sound/pci/hda/hda_intel.c | 54 +++++++++++++++++++++++++---- sound/soc/intel/Kconfig | 6 ++++ sound/soc/intel/skylake/skl.c | 48 ++++++++++++++++++-------- 6 files changed, 155 insertions(+), 23 deletions(-)
base-commit: ba26f635bf7253b42b08135152b18a2c8082abb7
Now that the SST/Skylake driver supports per platform selectors, we can add logic to automatically select the right driver.
If the Skylake driver is selected for a specific platform, and the DSP is detected at run-time based on the PCI class/subclass/prog-if information, the legacy HDaudio driver aborts the probe. This will result in a single driver probing and remove the need for modprobe blacklists.
Follow-up patches will add a module parameter to bypass the logic if this automatic detection fails, or if the Skylake driver is unable to actually support the platform (firmware authentication, missing topology file, hardware issue, etc).
The same mechanism will be used to conflicts generated by the same PCI ID being registered by both legacy HDAuudio and SOF drivers for Intel platforms. In other words SOF will not require changes to the HDaudio legacy.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/pci/hda/Kconfig | 62 ++++++++++++++++++++++++++++++++++ sound/pci/hda/hda_controller.h | 2 +- sound/pci/hda/hda_intel.c | 34 +++++++++++++++---- sound/soc/intel/Kconfig | 6 ++++ 4 files changed, 96 insertions(+), 8 deletions(-)
diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index 4235907b7858..0d38c006e182 100644 --- a/sound/pci/hda/Kconfig +++ b/sound/pci/hda/Kconfig @@ -226,6 +226,68 @@ config SND_HDA_POWER_SAVE_DEFAULT The default time-out value in seconds for HD-audio automatic power-save mode. 0 means to disable the power-save mode.
+if SND_HDA_INTEL + +# The options below should not be enabled by distributions or +# users. They are selected by Intel/Skylake or SOF drivers when they +# register for a PCI ID which is also handled by the HDAudio legacy +# driver. When this option is selected and the DSP is detected based on +# the PCI class/subclass/prog-if, the probe of the HDAudio legacy +# aborts. This mechanism removes the need for distributions to use +# blacklists. It can be bypassed with module parameters should the +# Intel/Skylake or SOF drivers fail to handle a specific platform. + +config SND_HDA_INTEL_DSP_DETECTION_SKL + bool + help + This option is selected by SOF or SST drivers, not users or distros. + It enables DSP detection based on PCI class information for + Skylake machines. + +config SND_HDA_INTEL_DSP_DETECTION_APL + bool + help + This option is selected by SOF or SST drivers, not users or distros. + It enables DSP detection based on PCI class information for + Broxton/ApolloLake machines + +config SND_HDA_INTEL_DSP_DETECTION_KBL + bool + help + This option is selected by SOF or SST drivers, not users or distros. + It enables DSP detection based on PCI class information for + KabyLake machines + +config SND_HDA_INTEL_DSP_DETECTION_GLK + bool + help + This option is selected by SOF or SST drivers, not users or distros. + It enables DSP detection based on PCI class information for + GeminiLake machines + +config SND_HDA_INTEL_DSP_DETECTION_CNL + bool + help + This option is selected by SOF or SST drivers, not users or distros. + It enables DSP detection based on PCI class information for + CannonLake machines + +config SND_HDA_INTEL_DSP_DETECTION_CFL + bool + help + This option is selected by SOF or SST drivers, not users or distros. + It enables DSP detection based on PCI class information for + CoffeeLake machines + +config SND_HDA_INTEL_DSP_DETECTION_ICL + bool + help + This option is selected by SOF or SST drivers, not users or distros. + It enables DSP detection based on PCI class information for + IceLake machines + +endif ## SND_HDA_INTEL + endif
endmenu diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h index c95097bb5a0c..2351115f922e 100644 --- a/sound/pci/hda/hda_controller.h +++ b/sound/pci/hda/hda_controller.h @@ -37,7 +37,7 @@ #else #define AZX_DCAPS_I915_COMPONENT 0 /* NOP */ #endif -/* 14 unused */ +#define AZX_DCAPS_INTEL_SHARED (1 << 14) /* shared with ASoC */ #define AZX_DCAPS_CTX_WORKAROUND (1 << 15) /* X-Fi workaround */ #define AZX_DCAPS_POSFIX_LPIB (1 << 16) /* Use LPIB as default */ /* 17 unused */ diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 76f03abd15ab..307473cc5a1b 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -360,6 +360,7 @@ enum { AZX_DCAPS_NO_64BIT |\ AZX_DCAPS_4K_BDLE_BOUNDARY | AZX_DCAPS_SNOOP_OFF)
+#define AZX_DCAPS_INTEL_DSP_DETECTION(conf) (IS_ENABLED(CONFIG_SND_HDA_INTEL_DSP_DETECTION_##conf) ? AZX_DCAPS_INTEL_SHARED : 0) /* * vga_switcheroo support */ @@ -2091,6 +2092,11 @@ static int azx_probe(struct pci_dev *pci, bool schedule_probe; int err;
+ /* check if this driver can be used on SKL+ Intel platforms */ + if ((pci_id->driver_data & AZX_DCAPS_INTEL_SHARED) && + pci->class != 0x040300) + return -ENODEV; + if (dev >= SNDRV_CARDS) return -ENODEV; if (!enable[dev]) { @@ -2408,34 +2414,48 @@ static const struct pci_device_id azx_ids[] = { .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE }, /* Sunrise Point-LP */ { PCI_DEVICE(0x8086, 0x9d70), - .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE }, + .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE | + AZX_DCAPS_INTEL_DSP_DETECTION(SKL) + }, /* Kabylake */ { PCI_DEVICE(0x8086, 0xa171), .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE }, /* Kabylake-LP */ { PCI_DEVICE(0x8086, 0x9d71), - .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE }, + .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE | + AZX_DCAPS_INTEL_DSP_DETECTION(KBL) + }, /* Kabylake-H */ { PCI_DEVICE(0x8086, 0xa2f0), .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE }, /* Coffelake */ { PCI_DEVICE(0x8086, 0xa348), - .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE}, + .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE | + AZX_DCAPS_INTEL_DSP_DETECTION(CFL) + }, /* Cannonlake */ { PCI_DEVICE(0x8086, 0x9dc8), - .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE}, + .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE | + AZX_DCAPS_INTEL_DSP_DETECTION(CNL) + }, /* Icelake */ { PCI_DEVICE(0x8086, 0x34c8), - .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE}, + .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE | + AZX_DCAPS_INTEL_DSP_DETECTION(ICL) + }, /* Broxton-P(Apollolake) */ { PCI_DEVICE(0x8086, 0x5a98), - .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON }, + .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON | + AZX_DCAPS_INTEL_DSP_DETECTION(APL) + }, /* Broxton-T */ { PCI_DEVICE(0x8086, 0x1a98), .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON }, /* Gemini-Lake */ { PCI_DEVICE(0x8086, 0x3198), - .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON }, + .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON | + AZX_DCAPS_INTEL_DSP_DETECTION(GLK) + }, /* Haswell */ { PCI_DEVICE(0x8086, 0x0a0c), .driver_data = AZX_DRIVER_HDMI | AZX_DCAPS_INTEL_HASWELL }, diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index 99a62ba409df..2fd1b61e8331 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -188,6 +188,12 @@ config SND_SOC_INTEL_SKYLAKE_COMMON select SND_SOC_TOPOLOGY select SND_SOC_INTEL_SST select SND_SOC_HDAC_HDA if SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC + select SND_HDA_INTEL_DSP_DETECTION_SKL if SND_SOC_INTEL_SKL + select SND_HDA_INTEL_DSP_DETECTION_APL if SND_SOC_INTEL_APL + select SND_HDA_INTEL_DSP_DETECTION_KBL if SND_SOC_INTEL_KBL + select SND_HDA_INTEL_DSP_DETECTION_GLK if SND_SOC_INTEL_GLK + select SND_HDA_INTEL_DSP_DETECTION_CNL if SND_SOC_INTEL_CNL + select SND_HDA_INTEL_DSP_DETECTION_CFL if SND_SOC_INTEL_CFL select SND_SOC_ACPI_INTEL_MATCH help If you have a Intel Skylake/Broxton/ApolloLake/KabyLake/
For HDaudio and Skylake drivers, add module parameter "pci_binding"
When pci_binding == 0 (AUTO), the PCI class/subclass info is used to select drivers based on the presence of the DSP.
pci_binding == 1 (LEGACY) forces the use of the HDAudio legacy driver, even if the DSP is present.
pci_binding == 2 (ASOC) forces the use of the ASOC driver. The information on the DSP presence is bypassed.
The value for the module parameter needs to be identical for both drivers. This parameter is intended as a back-up solution if the automatic detection fails or when the DSP usage fails. Such cases should be reported on the alsa-devel mailing list for analysis.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- include/sound/hdaudio.h | 6 +++++ sound/pci/hda/hda_intel.c | 26 ++++++++++++++++--- sound/soc/intel/skylake/skl.c | 48 ++++++++++++++++++++++++----------- 3 files changed, 62 insertions(+), 18 deletions(-)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index cd1773d0e08f..5934d7f228f0 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -99,6 +99,12 @@ enum { HDA_DEV_ASOC, };
+enum { + SND_SKL_PCI_BIND_AUTO, /* automatic selection based on pci class */ + SND_SKL_PCI_BIND_LEGACY,/* bind only with legacy driver */ + SND_SKL_PCI_BIND_ASOC /* bind only with ASoC driver */ +}; + /* direction */ enum { HDA_INPUT, HDA_OUTPUT diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 307473cc5a1b..8d35a6912def 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -172,6 +172,9 @@ 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 +static int skl_pci_binding; +module_param_named(pci_binding, skl_pci_binding, int, 0444); +MODULE_PARM_DESC(pci_binding, "PCI binding (0=auto, 1=only legacy, 2=only asoc");
#ifdef CONFIG_PM static int param_set_xint(const char *val, const struct kernel_param *kp); @@ -2093,9 +2096,26 @@ static int azx_probe(struct pci_dev *pci, int err;
/* check if this driver can be used on SKL+ Intel platforms */ - if ((pci_id->driver_data & AZX_DCAPS_INTEL_SHARED) && - pci->class != 0x040300) - return -ENODEV; + if (pci_id->driver_data & AZX_DCAPS_INTEL_SHARED) { + switch (skl_pci_binding) { + case SND_SKL_PCI_BIND_AUTO: + if (pci->class != 0x040300) { + dev_info(&pci->dev, "The DSP is enabled on this platform, aborting probe\n"); + return -ENODEV; + } + dev_info(&pci->dev, "No DSP detected, continuing HDaudio legacy probe\n"); + break; + case SND_SKL_PCI_BIND_LEGACY: + dev_info(&pci->dev, "Module parameter forced binding with HDaudio legacy, bypassed detection logic\n"); + break; + case SND_SKL_PCI_BIND_ASOC: + dev_info(&pci->dev, "Module parameter forced binding with SKL+ ASoC driver, aborting probe\n"); + return -ENODEV; + default: + dev_err(&pci->dev, "invalid value for skl_pci_binding module parameter, ignored\n"); + break; + } + }
if (dev >= SNDRV_CARDS) return -ENODEV; diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 5abd35ca4e41..6d87402a5b7f 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -40,6 +40,9 @@ #if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC) #include "../../../soc/codecs/hdac_hda.h" #endif +static int skl_pci_binding; +module_param_named(pci_binding, skl_pci_binding, int, 0444); +MODULE_PARM_DESC(pci_binding, "PCI binding (0=auto, 1=only legacy, 2=only asoc");
/* * initialize the PCI registers @@ -914,21 +917,6 @@ static int skl_first_init(struct hdac_bus *bus) unsigned short gcap; int cp_streams, pb_streams, start_idx;
- /* - * detect DSP by checking class/subclass/prog-id information - * class=04 subclass 03 prog-if 00: no DSP, legacy driver needs to be used - * 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: either of DSP or legacy mode can be used - */ - if (pci->class == 0x040300) { - dev_err(bus->dev, "The DSP is not enabled on this platform, aborting probe\n"); - return -ENODEV; - } else if (pci->class != 0x040100 && pci->class != 0x040380) { - dev_err(bus->dev, "Unknown PCI class/subclass/prog-if information (0x%06x) found, aborting probe\n", pci->class); - return -ENODEV; - } - dev_info(bus->dev, "DSP detected with PCI class/subclass/prog-if info 0x%06x\n", pci->class); - err = pci_request_regions(pci, "Skylake HD audio"); if (err < 0) return err; @@ -1002,6 +990,36 @@ static int skl_probe(struct pci_dev *pci, struct hdac_bus *bus = NULL; int err;
+ 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"); + 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"); + return -ENODEV; + case SND_SKL_PCI_BIND_ASOC: + dev_info(&pci->dev, "Module parameter forced binding with SKL driver, bypassed detection logic\n"); + break; + default: + dev_err(&pci->dev, "invalid value for skl_pci_binding module parameter, ignored\n"); + break; + } + /* we use ext core ops, so provide NULL for ops here */ err = skl_create(pci, NULL, &skl); if (err < 0)
On Sat, 15 Dec 2018 21:07:21 +0100, Pierre-Louis Bossart wrote:
Use detection logic based on the PCI class/subclass/prog-if in the HDaudio legacy probe, and add a module parameter to bypass or force the automatic detection should it ever fail (or in case of missing firmware or topology files)
This patchset was tested on
- an SKL HP device with the DSP disabled
- a WHL Acer Swift3 laptop
- APL NUC6
- KBL NUC7
5 reboot tests were done for each device with the DSP presence detected reliably. The "pci_binding" parameter was also tested on the WHL device.
The dynamic selection adds about 5ms worst case.
[ 2.813741] snd_hda_intel 0000:00:1f.3: The DSP is enabled on this platform, aborting probe [ 2.850588] snd_soc_skl 0000:00:1f.3: DSP detected with PCI class/subclass/prog-if info 0x040100
The SOF driver will use exactly the same mechanism, and add a mutual exclusion with the Skylake/SST driver to avoid having 3 drivers for the same PCI ID. Two is complicated enough.
Changes since v1 (Feedback from Takashi) a) added explanations and renamed Kconfigs to SND_HDA_INTEL_DSP_DETECTION_XYZ b) less verbose dev_info c) fixed pci_binding parameter
Pierre-Louis Bossart (2): ALSA: HD-Audio: SKL+: abort probe if DSP is present and Skylake driver selected ALSA: HD-Audio: SKL+: force HDaudio legacy or SKL+ driver selection
For both, feel free to take my ack: Reviewed-by: Takashi Iwai tiwai@suse.de
thanks,
Takashi
On Sat, Dec 15, 2018 at 02:07:21PM -0600, Pierre-Louis Bossart wrote:
Use detection logic based on the PCI class/subclass/prog-if in the HDaudio legacy probe, and add a module parameter to bypass or force the automatic detection should it ever fail (or in case of missing firmware or topology files)
Given the issues Linus found is this series still valid?
On 1/9/19 1:23 PM, Mark Brown wrote:
On Sat, Dec 15, 2018 at 02:07:21PM -0600, Pierre-Louis Bossart wrote:
Use detection logic based on the PCI class/subclass/prog-if in the HDaudio legacy probe, and add a module parameter to bypass or force the automatic detection should it ever fail (or in case of missing firmware or topology files)
Given the issues Linus found is this series still valid?
No. please drop this patchset. We need to first fix the HDMI issues due to broken programming sequences that existed before the dynamic selection was added. we can also simplify it if we change the probe order to avoid making any changes to the legacy and add more intelligence so e.g. only use the Skylake driver in "auto" mode if digital mics controlled by the DSP are detected in the NHLT tables.
participants (3)
-
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai