[alsa-devel] [RFC PATCH 0/6] ALSA/HDA: abort probe when DMICs are detected
This is the second take on same problem of detecting when the HDaudio legacy driver can be used and when the SST or SOF drivers are required.
The previous attempt based on a the PCI Class information was a resounding failure and broke audio on Linus' laptop, so we need additional information to avoid enabling a DSP-based driver without a good reason to do so.
This patchset suggests the use of the NHLT information which *in theory* exposes DMIC endpoints. The legacy HDaudio driver cannot handle DMICs and will not provide any capture capabilities. Since it's typically the first one to probe due to the Makefile order, aborting the probe will let the PCI subsystem look for the next driver which hopefully will support this capability.
I tested this patch on 5 devices (SKL, KBL, APL, GLK, WHL), three without DMICs and two with, and the detection seems to work as planned. I would appreciate it if HDaudio integrators and folks at Google/Canonical/Endless can give this a try. As usual I expect that we will have to use quirks and work-arounds, but it'd be a better idea than a build-time mutual exclusion. We could also make this optional (Kconfig and/or module parameters) if people prefer to muck with blacklists.
Feedback and comments welcome!
Pierre-Louis Bossart (6): ASoC: Intel: boards: remove unnecessary inclusion of skl.h ASoC: Intel: Skylake: move NHLT header to common directory ASoC: Intel: common: move parts of NHLT code to new module ASoC: Intel: Skylake: use common NHLT module ALSA / hda: stop probe if DMICS are detected on Skylake+ platforms [HACK] ASoC: Intel: NHLT: handle VENDOR_DEFINED DMIC geometry
sound/pci/hda/Kconfig | 1 + sound/pci/hda/hda_intel.c | 30 ++++++ sound/soc/intel/Kconfig | 3 + sound/soc/intel/boards/glk_rt5682_max98357a.c | 1 - sound/soc/intel/boards/kbl_da7219_max98357a.c | 1 - sound/soc/intel/boards/kbl_da7219_max98927.c | 1 - sound/soc/intel/boards/skl_hda_dsp_common.c | 1 - sound/soc/intel/common/Makefile | 3 + sound/soc/intel/common/intel-nhlt.c | 101 ++++++++++++++++++ .../skl-nhlt.h => common/intel-nhlt.h} | 28 +++++ sound/soc/intel/skylake/skl-nhlt.c | 91 +--------------- sound/soc/intel/skylake/skl-ssp-clk.c | 1 + sound/soc/intel/skylake/skl-topology.c | 1 + sound/soc/intel/skylake/skl.c | 11 +- sound/soc/intel/skylake/skl.h | 4 - 15 files changed, 176 insertions(+), 102 deletions(-) create mode 100644 sound/soc/intel/common/intel-nhlt.c rename sound/soc/intel/{skylake/skl-nhlt.h => common/intel-nhlt.h} (83%)
We've used a standard interface for machine drivers for some time now, there is no need for this dependency on a Skylake-specific header
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/intel/boards/glk_rt5682_max98357a.c | 1 - sound/soc/intel/boards/kbl_da7219_max98357a.c | 1 - sound/soc/intel/boards/kbl_da7219_max98927.c | 1 - sound/soc/intel/boards/skl_hda_dsp_common.c | 1 - 4 files changed, 4 deletions(-)
diff --git a/sound/soc/intel/boards/glk_rt5682_max98357a.c b/sound/soc/intel/boards/glk_rt5682_max98357a.c index 6b677b5bcdcd..7180100a9084 100644 --- a/sound/soc/intel/boards/glk_rt5682_max98357a.c +++ b/sound/soc/intel/boards/glk_rt5682_max98357a.c @@ -17,7 +17,6 @@ #include <sound/pcm_params.h> #include <sound/soc.h> #include <sound/soc-acpi.h> -#include "../skylake/skl.h" #include "../../codecs/rt5682.h" #include "../../codecs/hdac_hdmi.h"
diff --git a/sound/soc/intel/boards/kbl_da7219_max98357a.c b/sound/soc/intel/boards/kbl_da7219_max98357a.c index 07491a0f8fb8..4e5db2241fb9 100644 --- a/sound/soc/intel/boards/kbl_da7219_max98357a.c +++ b/sound/soc/intel/boards/kbl_da7219_max98357a.c @@ -19,7 +19,6 @@ #include <sound/soc.h> #include "../../codecs/da7219.h" #include "../../codecs/hdac_hdmi.h" -#include "../skylake/skl.h" #include "../../codecs/da7219-aad.h"
#define KBL_DIALOG_CODEC_DAI "da7219-hifi" diff --git a/sound/soc/intel/boards/kbl_da7219_max98927.c b/sound/soc/intel/boards/kbl_da7219_max98927.c index 1efe7fdad2cb..d6765c359449 100644 --- a/sound/soc/intel/boards/kbl_da7219_max98927.c +++ b/sound/soc/intel/boards/kbl_da7219_max98927.c @@ -19,7 +19,6 @@ #include <sound/soc.h> #include "../../codecs/da7219.h" #include "../../codecs/hdac_hdmi.h" -#include "../skylake/skl.h" #include "../../codecs/da7219-aad.h"
#define KBL_DIALOG_CODEC_DAI "da7219-hifi" diff --git a/sound/soc/intel/boards/skl_hda_dsp_common.c b/sound/soc/intel/boards/skl_hda_dsp_common.c index 8b68f41a5b88..82f10bf2abb2 100644 --- a/sound/soc/intel/boards/skl_hda_dsp_common.c +++ b/sound/soc/intel/boards/skl_hda_dsp_common.c @@ -12,7 +12,6 @@ #include <sound/pcm_params.h> #include <sound/soc.h> #include "../../codecs/hdac_hdmi.h" -#include "../skylake/skl.h" #include "skl_hda_dsp_common.h"
#define NAME_SIZE 32
Prepare move from NHLT code to common directory, starting with header.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/intel/{skylake/skl-nhlt.h => common/intel-nhlt.h} | 0 sound/soc/intel/skylake/skl-nhlt.c | 1 + sound/soc/intel/skylake/skl-ssp-clk.c | 1 + sound/soc/intel/skylake/skl-topology.c | 1 + sound/soc/intel/skylake/skl.h | 1 - 5 files changed, 3 insertions(+), 1 deletion(-) rename sound/soc/intel/{skylake/skl-nhlt.h => common/intel-nhlt.h} (100%)
diff --git a/sound/soc/intel/skylake/skl-nhlt.h b/sound/soc/intel/common/intel-nhlt.h similarity index 100% rename from sound/soc/intel/skylake/skl-nhlt.h rename to sound/soc/intel/common/intel-nhlt.h diff --git a/sound/soc/intel/skylake/skl-nhlt.c b/sound/soc/intel/skylake/skl-nhlt.c index 5d125a3df527..8c72b737c1fa 100644 --- a/sound/soc/intel/skylake/skl-nhlt.c +++ b/sound/soc/intel/skylake/skl-nhlt.c @@ -18,6 +18,7 @@ * */ #include <linux/pci.h> +#include "../common/intel-nhlt.h" #include "skl.h" #include "skl-i2s.h"
diff --git a/sound/soc/intel/skylake/skl-ssp-clk.c b/sound/soc/intel/skylake/skl-ssp-clk.c index cda1b5fa7436..55a066c22c81 100644 --- a/sound/soc/intel/skylake/skl-ssp-clk.c +++ b/sound/soc/intel/skylake/skl-ssp-clk.c @@ -14,6 +14,7 @@ #include "skl.h" #include "skl-ssp-clk.h" #include "skl-topology.h" +#include "../common/intel-nhlt.h"
#define to_skl_clk(_hw) container_of(_hw, struct skl_clk, hw)
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c index 389f1862bc43..a900c38328d5 100644 --- a/sound/soc/intel/skylake/skl-topology.c +++ b/sound/soc/intel/skylake/skl-topology.c @@ -30,6 +30,7 @@ #include "skl.h" #include "../common/sst-dsp.h" #include "../common/sst-dsp-priv.h" +#include "../common/intel-nhlt.h"
#define SKL_CH_FIXUP_MASK (1 << 0) #define SKL_RATE_FIXUP_MASK (1 << 1) diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h index 85f8bb6687dc..f0a15cb3d311 100644 --- a/sound/soc/intel/skylake/skl.h +++ b/sound/soc/intel/skylake/skl.h @@ -25,7 +25,6 @@ #include <sound/hdaudio_ext.h> #include <sound/hda_codec.h> #include <sound/soc.h> -#include "skl-nhlt.h" #include "skl-ssp-clk.h"
#define SKL_SUSPEND_DELAY 2000
On Fri, 24 May 2019 01:39:47 +0200, Pierre-Louis Bossart wrote:
Prepare move from NHLT code to common directory, starting with header.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
sound/soc/intel/{skylake/skl-nhlt.h => common/intel-nhlt.h} | 0
Since the header is included over ASoC, it should be rather into include/sound.
thanks,
Takashi
On 5/24/19 2:23 AM, Takashi Iwai wrote:
On Fri, 24 May 2019 01:39:47 +0200, Pierre-Louis Bossart wrote:
Prepare move from NHLT code to common directory, starting with header.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
sound/soc/intel/{skylake/skl-nhlt.h => common/intel-nhlt.h} | 0
Since the header is included over ASoC, it should be rather into include/sound.
ok. should we use an hda-intel-nhlt name then?
On Fri, 24 May 2019 15:11:29 +0200, Pierre-Louis Bossart wrote:
On 5/24/19 2:23 AM, Takashi Iwai wrote:
On Fri, 24 May 2019 01:39:47 +0200, Pierre-Louis Bossart wrote:
Prepare move from NHLT code to common directory, starting with header.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
sound/soc/intel/{skylake/skl-nhlt.h => common/intel-nhlt.h} | 0
Since the header is included over ASoC, it should be rather into include/sound.
ok. should we use an hda-intel-nhlt name then?
Feel free to pick a name as you like, I don't mind unless it conflicts with something else :)
Takashi
Move parts of the code outside of the Skylake driver to help detect the presence of DMICs (which are not supported by the HDaudio legacy driver).
No functionality change, only indentation and checkpatch fixes, and making sure that the code compiles without ACPI
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/intel/Kconfig | 3 + sound/soc/intel/common/Makefile | 3 + sound/soc/intel/common/intel-nhlt.c | 98 +++++++++++++++++++++++++++++ sound/soc/intel/common/intel-nhlt.h | 28 +++++++++ 4 files changed, 132 insertions(+) create mode 100644 sound/soc/intel/common/intel-nhlt.c
diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index b089ed3bf77f..71f19b73eed7 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -205,6 +205,7 @@ config SND_SOC_INTEL_SKYLAKE_COMMON select SND_SOC_INTEL_SST select SND_SOC_HDAC_HDA if SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC select SND_SOC_ACPI_INTEL_MATCH + select SND_SOC_INTEL_NHLT help If you have a Intel Skylake/Broxton/ApolloLake/KabyLake/ GeminiLake or CannonLake platform with the DSP enabled in the BIOS @@ -224,6 +225,8 @@ config SND_SOC_ACPI_INTEL_MATCH
endif ## SND_SOC_INTEL_SST_TOPLEVEL || SND_SOC_SOF_INTEL_TOPLEVEL
+config SND_SOC_INTEL_NHLT + tristate
# ASoC codec drivers source "sound/soc/intel/boards/Kconfig" diff --git a/sound/soc/intel/common/Makefile b/sound/soc/intel/common/Makefile index 56c81e20b5bf..4a5d2e4c28e4 100644 --- a/sound/soc/intel/common/Makefile +++ b/sound/soc/intel/common/Makefile @@ -10,7 +10,10 @@ snd-soc-acpi-intel-match-objs := soc-acpi-intel-byt-match.o soc-acpi-intel-cht-m soc-acpi-intel-cnl-match.o soc-acpi-intel-icl-match.o \ soc-acpi-intel-hda-match.o
+snd-soc-intel-nhlt-objs := intel-nhlt.o + obj-$(CONFIG_SND_SOC_INTEL_SST) += snd-soc-sst-dsp.o snd-soc-sst-ipc.o obj-$(CONFIG_SND_SOC_INTEL_SST_ACPI) += snd-soc-sst-acpi.o obj-$(CONFIG_SND_SOC_INTEL_SST_FIRMWARE) += snd-soc-sst-firmware.o obj-$(CONFIG_SND_SOC_ACPI_INTEL_MATCH) += snd-soc-acpi-intel-match.o +obj-$(CONFIG_SND_SOC_INTEL_NHLT) += snd-soc-intel-nhlt.o diff --git a/sound/soc/intel/common/intel-nhlt.c b/sound/soc/intel/common/intel-nhlt.c new file mode 100644 index 000000000000..d93ecc32d996 --- /dev/null +++ b/sound/soc/intel/common/intel-nhlt.c @@ -0,0 +1,98 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2015-2019 Intel Corporation + +#include <linux/acpi.h> +#include "intel-nhlt.h" + +#define NHLT_ACPI_HEADER_SIG "NHLT" + +/* Unique identification for getting NHLT blobs */ +static guid_t osc_guid = + GUID_INIT(0xA69F886E, 0x6CEB, 0x4594, + 0xA4, 0x1F, 0x7B, 0x5D, 0xCE, 0x24, 0xC5, 0x53); + +struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) +{ + acpi_handle handle; + union acpi_object *obj; + struct nhlt_resource_desc *nhlt_ptr = NULL; + struct nhlt_acpi_table *nhlt_table = NULL; + + handle = ACPI_HANDLE(dev); + if (!handle) { + dev_err(dev, "Didn't find ACPI_HANDLE\n"); + return NULL; + } + + obj = acpi_evaluate_dsm(handle, &osc_guid, 1, 1, NULL); + if (obj && obj->type == ACPI_TYPE_BUFFER) { + nhlt_ptr = (struct nhlt_resource_desc *)obj->buffer.pointer; + if (nhlt_ptr->length) + nhlt_table = (struct nhlt_acpi_table *) + memremap(nhlt_ptr->min_addr, nhlt_ptr->length, + MEMREMAP_WB); + ACPI_FREE(obj); + if (nhlt_table && + (strncmp(nhlt_table->header.signature, + NHLT_ACPI_HEADER_SIG, + strlen(NHLT_ACPI_HEADER_SIG)) != 0)) { + memunmap(nhlt_table); + dev_err(dev, "NHLT ACPI header signature incorrect\n"); + return NULL; + } + return nhlt_table; + } + + dev_dbg(dev, "No NHLT table found\n"); + return NULL; +} +EXPORT_SYMBOL_GPL(intel_nhlt_init); + +void intel_nhlt_free(struct nhlt_acpi_table *nhlt) +{ + memunmap((void *)nhlt); +} +EXPORT_SYMBOL_GPL(intel_nhlt_free); + +int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) +{ + struct nhlt_endpoint *epnt; + struct nhlt_dmic_array_config *cfg; + unsigned int dmic_geo = 0; + u8 j; + + if (!nhlt) + return 0; + + epnt = (struct nhlt_endpoint *)nhlt->desc; + + for (j = 0; j < nhlt->endpoint_count; j++) { + if (epnt->linktype == NHLT_LINK_DMIC) { + cfg = (struct nhlt_dmic_array_config *) + (epnt->config.caps); + switch (cfg->array_type) { + case NHLT_MIC_ARRAY_2CH_SMALL: + case NHLT_MIC_ARRAY_2CH_BIG: + dmic_geo |= MIC_ARRAY_2CH; + break; + + case NHLT_MIC_ARRAY_4CH_1ST_GEOM: + case NHLT_MIC_ARRAY_4CH_L_SHAPED: + case NHLT_MIC_ARRAY_4CH_2ND_GEOM: + dmic_geo |= MIC_ARRAY_4CH; + break; + + default: + dev_warn(dev, "undefined DMIC array_type 0x%0x\n", + cfg->array_type); + } + } + epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length); + } + + return dmic_geo; +} +EXPORT_SYMBOL_GPL(intel_nhlt_get_dmic_geo); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("Intel NHLT driver"); diff --git a/sound/soc/intel/common/intel-nhlt.h b/sound/soc/intel/common/intel-nhlt.h index 116534e7b3c5..125e47ae9a3c 100644 --- a/sound/soc/intel/common/intel-nhlt.h +++ b/sound/soc/intel/common/intel-nhlt.h @@ -20,6 +20,8 @@ #ifndef __SKL_NHLT_H__ #define __SKL_NHLT_H__
+#if IS_ENABLED(CONFIG_ACPI) + #include <linux/acpi.h>
struct wav_fmt { @@ -125,4 +127,30 @@ enum { NHLT_MIC_ARRAY_VENDOR_DEFINED = 0xf, };
+struct nhlt_acpi_table *intel_nhlt_init(struct device *dev); + +void intel_nhlt_free(struct nhlt_acpi_table *addr); + +int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt); + +#else + +struct nhlt_acpi_table; + +static inline nhlt_acpi_table *intel_nhlt_init(struct device *dev) +{ + return NULL; +} + +static inline void intel_nhlt_free(struct nhlt_acpi_table *addr) +{ +} + +static inline int intel_nhlt_get_dmic_geo(struct device *dev, + struct nhlt_acpi_table *nhlt) +{ + return 0; +} +#endif + #endif
On Fri, 24 May 2019 01:39:48 +0200, Pierre-Louis Bossart wrote:
Move parts of the code outside of the Skylake driver to help detect the presence of DMICs (which are not supported by the HDaudio legacy driver).
No functionality change, only indentation and checkpatch fixes, and making sure that the code compiles without ACPI
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
This is no ASoC-specific code, which will be called from the legacy HDA driver, too. So better to drop SOC_ prefix.
thanks,
Takashi
sound/soc/intel/Kconfig | 3 + sound/soc/intel/common/Makefile | 3 + sound/soc/intel/common/intel-nhlt.c | 98 +++++++++++++++++++++++++++++ sound/soc/intel/common/intel-nhlt.h | 28 +++++++++ 4 files changed, 132 insertions(+) create mode 100644 sound/soc/intel/common/intel-nhlt.c
diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig index b089ed3bf77f..71f19b73eed7 100644 --- a/sound/soc/intel/Kconfig +++ b/sound/soc/intel/Kconfig @@ -205,6 +205,7 @@ config SND_SOC_INTEL_SKYLAKE_COMMON select SND_SOC_INTEL_SST select SND_SOC_HDAC_HDA if SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC select SND_SOC_ACPI_INTEL_MATCH
- select SND_SOC_INTEL_NHLT help If you have a Intel Skylake/Broxton/ApolloLake/KabyLake/ GeminiLake or CannonLake platform with the DSP enabled in the BIOS
@@ -224,6 +225,8 @@ config SND_SOC_ACPI_INTEL_MATCH
endif ## SND_SOC_INTEL_SST_TOPLEVEL || SND_SOC_SOF_INTEL_TOPLEVEL
+config SND_SOC_INTEL_NHLT
- tristate
# ASoC codec drivers source "sound/soc/intel/boards/Kconfig" diff --git a/sound/soc/intel/common/Makefile b/sound/soc/intel/common/Makefile index 56c81e20b5bf..4a5d2e4c28e4 100644 --- a/sound/soc/intel/common/Makefile +++ b/sound/soc/intel/common/Makefile @@ -10,7 +10,10 @@ snd-soc-acpi-intel-match-objs := soc-acpi-intel-byt-match.o soc-acpi-intel-cht-m soc-acpi-intel-cnl-match.o soc-acpi-intel-icl-match.o \ soc-acpi-intel-hda-match.o
+snd-soc-intel-nhlt-objs := intel-nhlt.o
obj-$(CONFIG_SND_SOC_INTEL_SST) += snd-soc-sst-dsp.o snd-soc-sst-ipc.o obj-$(CONFIG_SND_SOC_INTEL_SST_ACPI) += snd-soc-sst-acpi.o obj-$(CONFIG_SND_SOC_INTEL_SST_FIRMWARE) += snd-soc-sst-firmware.o obj-$(CONFIG_SND_SOC_ACPI_INTEL_MATCH) += snd-soc-acpi-intel-match.o +obj-$(CONFIG_SND_SOC_INTEL_NHLT) += snd-soc-intel-nhlt.o diff --git a/sound/soc/intel/common/intel-nhlt.c b/sound/soc/intel/common/intel-nhlt.c new file mode 100644 index 000000000000..d93ecc32d996 --- /dev/null +++ b/sound/soc/intel/common/intel-nhlt.c @@ -0,0 +1,98 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2015-2019 Intel Corporation
+#include <linux/acpi.h> +#include "intel-nhlt.h"
+#define NHLT_ACPI_HEADER_SIG "NHLT"
+/* Unique identification for getting NHLT blobs */ +static guid_t osc_guid =
- GUID_INIT(0xA69F886E, 0x6CEB, 0x4594,
0xA4, 0x1F, 0x7B, 0x5D, 0xCE, 0x24, 0xC5, 0x53);+struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) +{
- acpi_handle handle;
- union acpi_object *obj;
- struct nhlt_resource_desc *nhlt_ptr = NULL;
- struct nhlt_acpi_table *nhlt_table = NULL;
- handle = ACPI_HANDLE(dev);
- if (!handle) {
dev_err(dev, "Didn't find ACPI_HANDLE\n");return NULL;- }
- obj = acpi_evaluate_dsm(handle, &osc_guid, 1, 1, NULL);
- if (obj && obj->type == ACPI_TYPE_BUFFER) {
nhlt_ptr = (struct nhlt_resource_desc *)obj->buffer.pointer;if (nhlt_ptr->length)nhlt_table = (struct nhlt_acpi_table *)memremap(nhlt_ptr->min_addr, nhlt_ptr->length,MEMREMAP_WB);ACPI_FREE(obj);if (nhlt_table &&(strncmp(nhlt_table->header.signature,NHLT_ACPI_HEADER_SIG,strlen(NHLT_ACPI_HEADER_SIG)) != 0)) {memunmap(nhlt_table);dev_err(dev, "NHLT ACPI header signature incorrect\n");return NULL;}return nhlt_table;- }
- dev_dbg(dev, "No NHLT table found\n");
- return NULL;
+} +EXPORT_SYMBOL_GPL(intel_nhlt_init);
+void intel_nhlt_free(struct nhlt_acpi_table *nhlt) +{
- memunmap((void *)nhlt);
+} +EXPORT_SYMBOL_GPL(intel_nhlt_free);
+int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) +{
- struct nhlt_endpoint *epnt;
- struct nhlt_dmic_array_config *cfg;
- unsigned int dmic_geo = 0;
- u8 j;
- if (!nhlt)
return 0;- epnt = (struct nhlt_endpoint *)nhlt->desc;
- for (j = 0; j < nhlt->endpoint_count; j++) {
if (epnt->linktype == NHLT_LINK_DMIC) {cfg = (struct nhlt_dmic_array_config *)(epnt->config.caps);switch (cfg->array_type) {case NHLT_MIC_ARRAY_2CH_SMALL:case NHLT_MIC_ARRAY_2CH_BIG:dmic_geo |= MIC_ARRAY_2CH;break;case NHLT_MIC_ARRAY_4CH_1ST_GEOM:case NHLT_MIC_ARRAY_4CH_L_SHAPED:case NHLT_MIC_ARRAY_4CH_2ND_GEOM:dmic_geo |= MIC_ARRAY_4CH;break;default:dev_warn(dev, "undefined DMIC array_type 0x%0x\n",cfg->array_type);}}epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length);- }
- return dmic_geo;
+} +EXPORT_SYMBOL_GPL(intel_nhlt_get_dmic_geo);
+MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("Intel NHLT driver"); diff --git a/sound/soc/intel/common/intel-nhlt.h b/sound/soc/intel/common/intel-nhlt.h index 116534e7b3c5..125e47ae9a3c 100644 --- a/sound/soc/intel/common/intel-nhlt.h +++ b/sound/soc/intel/common/intel-nhlt.h @@ -20,6 +20,8 @@ #ifndef __SKL_NHLT_H__ #define __SKL_NHLT_H__
+#if IS_ENABLED(CONFIG_ACPI)
#include <linux/acpi.h>
struct wav_fmt { @@ -125,4 +127,30 @@ enum { NHLT_MIC_ARRAY_VENDOR_DEFINED = 0xf, };
+struct nhlt_acpi_table *intel_nhlt_init(struct device *dev);
+void intel_nhlt_free(struct nhlt_acpi_table *addr);
+int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt);
+#else
+struct nhlt_acpi_table;
+static inline nhlt_acpi_table *intel_nhlt_init(struct device *dev) +{
- return NULL;
+}
+static inline void intel_nhlt_free(struct nhlt_acpi_table *addr) +{ +}
+static inline int intel_nhlt_get_dmic_geo(struct device *dev,
struct nhlt_acpi_table *nhlt)+{
- return 0;
+} +#endif
#endif
2.20.1
On 5/24/19 2:54 AM, Takashi Iwai wrote:
On Fri, 24 May 2019 01:39:48 +0200, Pierre-Louis Bossart wrote:
Move parts of the code outside of the Skylake driver to help detect the presence of DMICs (which are not supported by the HDaudio legacy driver).
No functionality change, only indentation and checkpatch fixes, and making sure that the code compiles without ACPI
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
This is no ASoC-specific code, which will be called from the legacy HDA driver, too. So better to drop SOC_ prefix.
ok. I also thought about the location of the code, I am not sure this works if SND_SOC is not selected.
If SND_SOC is not selected, there is also no conflict with another driver so we may want to bypass this code. Or use it, flag that the config will prevent capture from working but continue the probe so that the playback works at least.
On Fri, 24 May 2019 15:16:05 +0200, Pierre-Louis Bossart wrote:
On 5/24/19 2:54 AM, Takashi Iwai wrote:
On Fri, 24 May 2019 01:39:48 +0200, Pierre-Louis Bossart wrote:
Move parts of the code outside of the Skylake driver to help detect the presence of DMICs (which are not supported by the HDaudio legacy driver).
No functionality change, only indentation and checkpatch fixes, and making sure that the code compiles without ACPI
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
This is no ASoC-specific code, which will be called from the legacy HDA driver, too. So better to drop SOC_ prefix.
ok. I also thought about the location of the code, I am not sure this works if SND_SOC is not selected.
If SND_SOC is not selected, there is also no conflict with another driver so we may want to bypass this code. Or use it, flag that the config will prevent capture from working but continue the probe so that the playback works at least.
Actually since the code is shared among ASoC and other parts, it might make sense to move it into sound/hda common directory, too.
Takashi
No functionality change, only use common functions now.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/intel/skylake/skl-nhlt.c | 90 ------------------------------ sound/soc/intel/skylake/skl.c | 11 ++-- sound/soc/intel/skylake/skl.h | 3 - 3 files changed, 7 insertions(+), 97 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-nhlt.c b/sound/soc/intel/skylake/skl-nhlt.c index 8c72b737c1fa..bb73d2544dc5 100644 --- a/sound/soc/intel/skylake/skl-nhlt.c +++ b/sound/soc/intel/skylake/skl-nhlt.c @@ -22,54 +22,6 @@ #include "skl.h" #include "skl-i2s.h"
-#define NHLT_ACPI_HEADER_SIG "NHLT" - -/* Unique identification for getting NHLT blobs */ -static guid_t osc_guid = - GUID_INIT(0xA69F886E, 0x6CEB, 0x4594, - 0xA4, 0x1F, 0x7B, 0x5D, 0xCE, 0x24, 0xC5, 0x53); - - -struct nhlt_acpi_table *skl_nhlt_init(struct device *dev) -{ - acpi_handle handle; - union acpi_object *obj; - struct nhlt_resource_desc *nhlt_ptr = NULL; - struct nhlt_acpi_table *nhlt_table = NULL; - - handle = ACPI_HANDLE(dev); - if (!handle) { - dev_err(dev, "Didn't find ACPI_HANDLE\n"); - return NULL; - } - - obj = acpi_evaluate_dsm(handle, &osc_guid, 1, 1, NULL); - if (obj && obj->type == ACPI_TYPE_BUFFER) { - nhlt_ptr = (struct nhlt_resource_desc *)obj->buffer.pointer; - if (nhlt_ptr->length) - nhlt_table = (struct nhlt_acpi_table *) - memremap(nhlt_ptr->min_addr, nhlt_ptr->length, - MEMREMAP_WB); - ACPI_FREE(obj); - if (nhlt_table && (strncmp(nhlt_table->header.signature, - NHLT_ACPI_HEADER_SIG, - strlen(NHLT_ACPI_HEADER_SIG)) != 0)) { - memunmap(nhlt_table); - dev_err(dev, "NHLT ACPI header signature incorrect\n"); - return NULL; - } - return nhlt_table; - } - - dev_err(dev, "device specific method to extract NHLT blob failed\n"); - return NULL; -} - -void skl_nhlt_free(struct nhlt_acpi_table *nhlt) -{ - memunmap((void *) nhlt); -} - static struct nhlt_specific_cfg *skl_get_specific_cfg( struct device *dev, struct nhlt_fmt *fmt, u8 no_ch, u32 rate, u16 bps, u8 linktype) @@ -172,48 +124,6 @@ struct nhlt_specific_cfg return NULL; }
-int skl_get_dmic_geo(struct skl *skl) -{ - struct nhlt_acpi_table *nhlt = (struct nhlt_acpi_table *)skl->nhlt; - struct nhlt_endpoint *epnt; - struct nhlt_dmic_array_config *cfg; - struct device *dev = &skl->pci->dev; - unsigned int dmic_geo = 0; - u8 j; - - if (!nhlt) - return 0; - - epnt = (struct nhlt_endpoint *)nhlt->desc; - - for (j = 0; j < nhlt->endpoint_count; j++) { - if (epnt->linktype == NHLT_LINK_DMIC) { - cfg = (struct nhlt_dmic_array_config *) - (epnt->config.caps); - switch (cfg->array_type) { - case NHLT_MIC_ARRAY_2CH_SMALL: - case NHLT_MIC_ARRAY_2CH_BIG: - dmic_geo |= MIC_ARRAY_2CH; - break; - - case NHLT_MIC_ARRAY_4CH_1ST_GEOM: - case NHLT_MIC_ARRAY_4CH_L_SHAPED: - case NHLT_MIC_ARRAY_4CH_2ND_GEOM: - dmic_geo |= MIC_ARRAY_4CH; - break; - - default: - dev_warn(dev, "undefined DMIC array_type 0x%0x\n", - cfg->array_type); - - } - } - epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length); - } - - return dmic_geo; -} - static void skl_nhlt_trim_space(char *trim) { char *s = trim; diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index f864f7b3df3a..322bbb36531b 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -37,6 +37,7 @@ #include "skl.h" #include "skl-sst-dsp.h" #include "skl-sst-ipc.h" +#include "../common/intel-nhlt.h" #if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC) #include "../../../soc/codecs/hdac_hda.h" #endif @@ -505,7 +506,9 @@ static int skl_find_machine(struct skl *skl, void *driver_data)
if (pdata) { skl->use_tplg_pcm = pdata->use_tplg_pcm; - mach->mach_params.dmic_num = skl_get_dmic_geo(skl); + mach->mach_params.dmic_num = + intel_nhlt_get_dmic_geo(&skl->pci->dev, + skl->nhlt); }
return 0; @@ -1014,7 +1017,7 @@ static int skl_probe(struct pci_dev *pci,
device_disable_async_suspend(bus->dev);
- skl->nhlt = skl_nhlt_init(bus->dev); + skl->nhlt = intel_nhlt_init(bus->dev);
if (skl->nhlt == NULL) { #if !IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC) @@ -1080,7 +1083,7 @@ static int skl_probe(struct pci_dev *pci, out_clk_free: skl_clock_device_unregister(skl); out_nhlt_free: - skl_nhlt_free(skl->nhlt); + intel_nhlt_free(skl->nhlt); out_free: skl_free(bus);
@@ -1130,7 +1133,7 @@ static void skl_remove(struct pci_dev *pci) skl_dmic_device_unregister(skl); skl_clock_device_unregister(skl); skl_nhlt_remove_sysfs(skl); - skl_nhlt_free(skl->nhlt); + intel_nhlt_free(skl->nhlt); skl_free(bus); dev_set_drvdata(&pci->dev, NULL); } diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h index f0a15cb3d311..0f19c9d47017 100644 --- a/sound/soc/intel/skylake/skl.h +++ b/sound/soc/intel/skylake/skl.h @@ -136,13 +136,10 @@ struct skl_dsp_ops { int skl_platform_unregister(struct device *dev); int skl_platform_register(struct device *dev);
-struct nhlt_acpi_table *skl_nhlt_init(struct device *dev); -void skl_nhlt_free(struct nhlt_acpi_table *addr); struct nhlt_specific_cfg *skl_get_ep_blob(struct skl *skl, u32 instance, u8 link_type, u8 s_fmt, u8 no_ch, u32 s_rate, u8 dirn, u8 dev_type);
-int skl_get_dmic_geo(struct skl *skl); int skl_nhlt_update_topology_bin(struct skl *skl); int skl_init_dsp(struct skl *skl); int skl_free_dsp(struct skl *skl);
The legacy HD-Audio driver cannot handle Skylake+ platforms with digital microphones. For those platforms, the SOF driver needs to be used.
Use the common intel-nhlt module to stop the probe when the DSP is enabled and DMICs are exposed in the NHTL tables.
Note: This assumes that the BIOS information is correct, and additional testing is required to see on which platforms the detection is a false positive.
FIXME: I need to find what is the mirror of azx_create() to free all the resources on exit.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/pci/hda/Kconfig | 1 + sound/pci/hda/hda_intel.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+)
diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index 4235907b7858..7b560c557b07 100644 --- a/sound/pci/hda/Kconfig +++ b/sound/pci/hda/Kconfig @@ -11,6 +11,7 @@ config SND_HDA_INTEL tristate "HD Audio PCI" depends on SND_PCI select SND_HDA + select SND_SOC_INTEL_NHLT help Say Y here to include support for Intel "High Definition Audio" (Azalia) and its compatible devices. diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 0741eae23f10..e9f427d75a5d 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -66,6 +66,7 @@ #include <sound/hda_codec.h> #include "hda_controller.h" #include "hda_intel.h" +#include "../../soc/intel/common/intel-nhlt.h"
#define CREATE_TRACE_POINTS #include "hda_intel_trace.h" @@ -2038,6 +2039,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) { @@ -2068,6 +2088,16 @@ 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 + */ + err = azx_check_dmic(pci, chip); + if (err < 0) { + /* FIXME: need to free everything allocated in azx_create */ + goto out_free; + } + pci_set_drvdata(pci, card);
err = register_vga_switcheroo(chip);
On Fri, 24 May 2019 01:39:50 +0200, Pierre-Louis Bossart wrote:
The legacy HD-Audio driver cannot handle Skylake+ platforms with digital microphones. For those platforms, the SOF driver needs to be used.
Use the common intel-nhlt module to stop the probe when the DSP is enabled and DMICs are exposed in the NHTL tables.
Note: This assumes that the BIOS information is correct, and additional testing is required to see on which platforms the detection is a false positive.
FIXME: I need to find what is the mirror of azx_create() to free all the resources on exit.
azx_free() does the whole, so just goto out_free should suffice.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
sound/pci/hda/Kconfig | 1 + sound/pci/hda/hda_intel.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+)
diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index 4235907b7858..7b560c557b07 100644 --- a/sound/pci/hda/Kconfig +++ b/sound/pci/hda/Kconfig @@ -11,6 +11,7 @@ config SND_HDA_INTEL tristate "HD Audio PCI" depends on SND_PCI select SND_HDA
- select SND_SOC_INTEL_NHLT
Better to select conditionally depending on ACPI
select SND_SOC_INTEL_NHLT if ACPI
thanks,
Takashi
On 5/24/19 2:56 AM, Takashi Iwai wrote:
On Fri, 24 May 2019 01:39:50 +0200, Pierre-Louis Bossart wrote:
The legacy HD-Audio driver cannot handle Skylake+ platforms with digital microphones. For those platforms, the SOF driver needs to be used.
Use the common intel-nhlt module to stop the probe when the DSP is enabled and DMICs are exposed in the NHTL tables.
Note: This assumes that the BIOS information is correct, and additional testing is required to see on which platforms the detection is a false positive.
FIXME: I need to find what is the mirror of azx_create() to free all the resources on exit.
azx_free() does the whole, so just goto out_free should suffice.
ok, thanks for the feedback!
The NHLT spec defines a VENDOR_DEFINED geometry without defining how many microphones are supported. Fall back to 2ch until we have better information or experimental evidence on what to do.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/intel/common/intel-nhlt.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/sound/soc/intel/common/intel-nhlt.c b/sound/soc/intel/common/intel-nhlt.c index d93ecc32d996..86f8a7f3f059 100644 --- a/sound/soc/intel/common/intel-nhlt.c +++ b/sound/soc/intel/common/intel-nhlt.c @@ -81,7 +81,10 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) case NHLT_MIC_ARRAY_4CH_2ND_GEOM: dmic_geo |= MIC_ARRAY_4CH; break; - + case NHLT_MIC_ARRAY_VENDOR_DEFINED: + dev_dbg(dev, "VENDOR_DEFINED DMIC array_type, using 2CH_SMALL\n"); + dmic_geo |= NHLT_MIC_ARRAY_2CH_SMALL; + break; default: dev_warn(dev, "undefined DMIC array_type 0x%0x\n", cfg->array_type);
On Fri, 24 May 2019 01:39:45 +0200, Pierre-Louis Bossart wrote:
This is the second take on same problem of detecting when the HDaudio legacy driver can be used and when the SST or SOF drivers are required.
The previous attempt based on a the PCI Class information was a resounding failure and broke audio on Linus' laptop, so we need additional information to avoid enabling a DSP-based driver without a good reason to do so.
This patchset suggests the use of the NHLT information which *in theory* exposes DMIC endpoints. The legacy HDaudio driver cannot handle DMICs and will not provide any capture capabilities. Since it's typically the first one to probe due to the Makefile order, aborting the probe will let the PCI subsystem look for the next driver which hopefully will support this capability.
I tested this patch on 5 devices (SKL, KBL, APL, GLK, WHL), three without DMICs and two with, and the detection seems to work as planned. I would appreciate it if HDaudio integrators and folks at Google/Canonical/Endless can give this a try. As usual I expect that we will have to use quirks and work-arounds, but it'd be a better idea than a build-time mutual exclusion. We could also make this optional (Kconfig and/or module parameters) if people prefer to muck with blacklists.
Feedback and comments welcome!
The general idea and suggested implementation look almost good to me. Of course we have to provide a way to override the default behavior in case of buggy BIOS (I bet a drink for the existence of such :)
thanks,
Takashi
On 5/24/19 2:58 AM, Takashi Iwai wrote:
On Fri, 24 May 2019 01:39:45 +0200, Pierre-Louis Bossart wrote:
This is the second take on same problem of detecting when the HDaudio legacy driver can be used and when the SST or SOF drivers are required.
The previous attempt based on a the PCI Class information was a resounding failure and broke audio on Linus' laptop, so we need additional information to avoid enabling a DSP-based driver without a good reason to do so.
This patchset suggests the use of the NHLT information which *in theory* exposes DMIC endpoints. The legacy HDaudio driver cannot handle DMICs and will not provide any capture capabilities. Since it's typically the first one to probe due to the Makefile order, aborting the probe will let the PCI subsystem look for the next driver which hopefully will support this capability.
I tested this patch on 5 devices (SKL, KBL, APL, GLK, WHL), three without DMICs and two with, and the detection seems to work as planned. I would appreciate it if HDaudio integrators and folks at Google/Canonical/Endless can give this a try. As usual I expect that we will have to use quirks and work-arounds, but it'd be a better idea than a build-time mutual exclusion. We could also make this optional (Kconfig and/or module parameters) if people prefer to muck with blacklists.
Feedback and comments welcome!
The general idea and suggested implementation look almost good to me. Of course we have to provide a way to override the default behavior in case of buggy BIOS (I bet a drink for the existence of such :)
I am not sure if it's a good idea to enable this by default, the experience of the first round showed it's risky to make assumptions on what BIOS vendors implemented. I'd rather make this an opt-in solution for distros that have to deal with this conflict and don't want to use blacklists, and provide both a Kconfig and kernel parameter to either statically or dynamically enable this capability. Also this is really needed mostly with WHL+ platforms where a lot of vendors use DMICs, for previous generations there are only Chromebooks and a single-digit number of KBL devices.
On Fri, 24 May 2019 15:26:54 +0200, Pierre-Louis Bossart wrote:
On 5/24/19 2:58 AM, Takashi Iwai wrote:
On Fri, 24 May 2019 01:39:45 +0200, Pierre-Louis Bossart wrote:
This is the second take on same problem of detecting when the HDaudio legacy driver can be used and when the SST or SOF drivers are required.
The previous attempt based on a the PCI Class information was a resounding failure and broke audio on Linus' laptop, so we need additional information to avoid enabling a DSP-based driver without a good reason to do so.
This patchset suggests the use of the NHLT information which *in theory* exposes DMIC endpoints. The legacy HDaudio driver cannot handle DMICs and will not provide any capture capabilities. Since it's typically the first one to probe due to the Makefile order, aborting the probe will let the PCI subsystem look for the next driver which hopefully will support this capability.
I tested this patch on 5 devices (SKL, KBL, APL, GLK, WHL), three without DMICs and two with, and the detection seems to work as planned. I would appreciate it if HDaudio integrators and folks at Google/Canonical/Endless can give this a try. As usual I expect that we will have to use quirks and work-arounds, but it'd be a better idea than a build-time mutual exclusion. We could also make this optional (Kconfig and/or module parameters) if people prefer to muck with blacklists.
Feedback and comments welcome!
The general idea and suggested implementation look almost good to me. Of course we have to provide a way to override the default behavior in case of buggy BIOS (I bet a drink for the existence of such :)
I am not sure if it's a good idea to enable this by default, the experience of the first round showed it's risky to make assumptions on what BIOS vendors implemented. I'd rather make this an opt-in solution for distros that have to deal with this conflict and don't want to use blacklists, and provide both a Kconfig and kernel parameter to either statically or dynamically enable this capability. Also this is really needed mostly with WHL+ platforms where a lot of vendors use DMICs, for previous generations there are only Chromebooks and a single-digit number of KBL devices.
Yes, it sounds reasonable.
thanks,
Takashi
Thanks for the patches!
On Fri, May 24, 2019 at 7:26 AM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
I am not sure if it's a good idea to enable this by default, the experience of the first round showed it's risky to make assumptions on what BIOS vendors implemented.
Can you clarify what you mean here, are you saying you don't want to enable this new DMIC hardware support by default?
Daniel
On 5/24/19 10:45 AM, Daniel Drake wrote:
Thanks for the patches!
On Fri, May 24, 2019 at 7:26 AM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
I am not sure if it's a good idea to enable this by default, the experience of the first round showed it's risky to make assumptions on what BIOS vendors implemented.
Can you clarify what you mean here, are you saying you don't want to enable this new DMIC hardware support by default?
No. What I am saying is that a) the legacy HDaudio driver does not support DMICs b) the decision to abort the HDaudio legacy driver probe should not be the default, since it depends on BIOS information that may be wrong and on which I have *zero* control.
There are 4 cases really:
1. DMICs attached to PCH and BIOS/NHLT reports DMICS -> abort HDaudio legacy probe 2. No DMICs attached to PCH and BIOS/NHLT does not report DMICs -> continue probe and use HDAudio legacy. 3. DMICs attached to PCH and BIOS/NHLT does not report DMICs -> broken config, we will need an option to abort the probe by force and ignore the BIOS if you care about audio capture. 4. no DMICs attached to PCH and BIOS/NHLT reports DMICs -> broken config, we need an option to ignore the BIOS and continue the probe.
Does this help?
Still not quite clear about the default behaviour here.
On Fri, May 24, 2019 at 10:12 AM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
b) the decision to abort the HDaudio legacy driver probe should not be the default, since it depends on BIOS information that may be wrong and on which I have *zero* control. [...]
- DMICs attached to PCH and BIOS/NHLT reports DMICS -> abort HDaudio
legacy probe
In the case of DMICs attached to PCH and BIOS/NHLT reports DMIC, is the HDaudio legacy probe aborted by default or not?
Thanks Daniel
On 5/24/19 12:34 PM, Daniel Drake wrote:
Still not quite clear about the default behaviour here.
On Fri, May 24, 2019 at 10:12 AM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
b) the decision to abort the HDaudio legacy driver probe should not be the default, since it depends on BIOS information that may be wrong and on which I have *zero* control. [...]
- DMICs attached to PCH and BIOS/NHLT reports DMICS -> abort HDaudio
legacy probe
In the case of DMICs attached to PCH and BIOS/NHLT reports DMIC, is the HDaudio legacy probe aborted by default or not?
no.
On Fri, May 24, 2019 at 12:38 PM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
In the case of DMICs attached to PCH and BIOS/NHLT reports DMIC, is the HDaudio legacy probe aborted by default or not?
no.
Thanks for the clarification. The plan makes sense to me! I'll check if we have any more devices available with DMIC where we can test.
Daniel
On 2019/5/25 上午12:12, Pierre-Louis Bossart wrote:
On 5/24/19 10:45 AM, Daniel Drake wrote:
Thanks for the patches!
On Fri, May 24, 2019 at 7:26 AM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
I am not sure if it's a good idea to enable this by default, the experience of the first round showed it's risky to make assumptions on what BIOS vendors implemented.
Can you clarify what you mean here, are you saying you don't want to enable this new DMIC hardware support by default?
No. What I am saying is that a) the legacy HDaudio driver does not support DMICs b) the decision to abort the HDaudio legacy driver probe should not be the default, since it depends on BIOS information that may be wrong and on which I have *zero* control.
There are 4 cases really:
- DMICs attached to PCH and BIOS/NHLT reports DMICS -> abort HDaudio
legacy probe 2. No DMICs attached to PCH and BIOS/NHLT does not report DMICs -> continue probe and use HDAudio legacy. 3. DMICs attached to PCH and BIOS/NHLT does not report DMICs -> broken config, we will need an option to abort the probe by force and ignore the BIOS if you care about audio capture. 4. no DMICs attached to PCH and BIOS/NHLT reports DMICs -> broken config, we need an option to ignore the BIOS and continue the probe.
Got it, we will do a test on a couple of machines, let us see if we can meet 3 and 4 in reality.
Does this help?
On 2019/5/25 上午9:40, Hui Wang wrote:
On 2019/5/25 上午12:12, Pierre-Louis Bossart wrote:
On 5/24/19 10:45 AM, Daniel Drake wrote:
Thanks for the patches!
On Fri, May 24, 2019 at 7:26 AM Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
I am not sure if it's a good idea to enable this by default, the experience of the first round showed it's risky to make assumptions on what BIOS vendors implemented.
Can you clarify what you mean here, are you saying you don't want to enable this new DMIC hardware support by default?
No. What I am saying is that a) the legacy HDaudio driver does not support DMICs b) the decision to abort the HDaudio legacy driver probe should not be the default, since it depends on BIOS information that may be wrong and on which I have *zero* control.
There are 4 cases really:
- DMICs attached to PCH and BIOS/NHLT reports DMICS -> abort HDaudio
legacy probe 2. No DMICs attached to PCH and BIOS/NHLT does not report DMICs -> continue probe and use HDAudio legacy. 3. DMICs attached to PCH and BIOS/NHLT does not report DMICs -> broken config, we will need an option to abort the probe by force and ignore the BIOS if you care about audio capture. 4. no DMICs attached to PCH and BIOS/NHLT reports DMICs -> broken config, we need an option to ignore the BIOS and continue the probe.
Got it, we will do a test on a couple of machines, let us see if we can meet 3 and 4 in reality.
I backported these 6 patches to our 5.0 kernel with the sof driver in it, and tested on 3 machines which have dmic connected to PCH (audio controller pciid 0x9dc8), without these 6 patches, I need to blacklist snd_hda_intel.ko and snd_soc_skl.ko to make the sof driver work, after backporting these 6 patches, I don't need to blacklist snd_hda_intel.ko, but still need to blacklist snd_soc_skl.ko, otherwise the sof driver doesn't work.
And I also tested these 6 patches on 3 machines without dmic, I don't need to blacklist anything, the audio works well via legacy hdaudio.
So for coexistence of soc_skl and soc_sof drivers, do we have any plan?
Thanks,
Hui.
Does this help?
I am not sure if it's a good idea to enable this by default, the experience of the first round showed it's risky to make assumptions on what BIOS vendors implemented.
Can you clarify what you mean here, are you saying you don't want to enable this new DMIC hardware support by default?
No. What I am saying is that a) the legacy HDaudio driver does not support DMICs b) the decision to abort the HDaudio legacy driver probe should not be the default, since it depends on BIOS information that may be wrong and on which I have *zero* control.
There are 4 cases really:
- DMICs attached to PCH and BIOS/NHLT reports DMICS -> abort HDaudio
legacy probe 2. No DMICs attached to PCH and BIOS/NHLT does not report DMICs -> continue probe and use HDAudio legacy. 3. DMICs attached to PCH and BIOS/NHLT does not report DMICs -> broken config, we will need an option to abort the probe by force and ignore the BIOS if you care about audio capture. 4. no DMICs attached to PCH and BIOS/NHLT reports DMICs -> broken config, we need an option to ignore the BIOS and continue the probe.
Got it, we will do a test on a couple of machines, let us see if we can meet 3 and 4 in reality.
I backported these 6 patches to our 5.0 kernel with the sof driver in it, and tested on 3 machines which have dmic connected to PCH (audio controller pciid 0x9dc8), without these 6 patches, I need to blacklist snd_hda_intel.ko and snd_soc_skl.ko to make the sof driver work, after backporting these 6 patches, I don't need to blacklist snd_hda_intel.ko, but still need to blacklist snd_soc_skl.ko, otherwise the sof driver doesn't work.
Thanks for testing. I've done additional updates on my side and I am reasonably confident we can make this SOF/legacy autodetect work.
And I also tested these 6 patches on 3 machines without dmic, I don't need to blacklist anything, the audio works well via legacy hdaudio.
So for coexistence of soc_skl and soc_sof drivers, do we have any plan?
HDaudio is not well supported by the SKL driver. We introduced this support in v4.19, but there are quite a few issues on specific platforms that are not well handled (questionable probe which lead to breaking audio on Linus' laptop, interrupt issues fixed with SOF and legacy, etc). Since SOF is well supported on those platforms, I am honestly leaning to de-feature HDAudio support from the SKL driver (as in make the HDaudio codec Kconfig option not recommended) and apply the same trick as for the legacy to abort the probe if there is no I2S codec and DMICs are detected. It would have no effect on any users and would help distros like Ubuntu avoid the need for blacklists. We can also apply DMI-based quirks for Chrome, e.g. for now all platforms prior to GLK should use SKL and more recent ones SOF.
On Tue, 18 Jun 2019 08:51:27 +0200, Pierre-Louis Bossart wrote:
I am not sure if it's a good idea to enable this by default, the experience of the first round showed it's risky to make assumptions on what BIOS vendors implemented.
Can you clarify what you mean here, are you saying you don't want to enable this new DMIC hardware support by default?
No. What I am saying is that a) the legacy HDaudio driver does not support DMICs b) the decision to abort the HDaudio legacy driver probe should not be the default, since it depends on BIOS information that may be wrong and on which I have *zero* control.
There are 4 cases really:
- DMICs attached to PCH and BIOS/NHLT reports DMICS -> abort
HDaudio legacy probe 2. No DMICs attached to PCH and BIOS/NHLT does not report DMICs -> continue probe and use HDAudio legacy. 3. DMICs attached to PCH and BIOS/NHLT does not report DMICs -> broken config, we will need an option to abort the probe by force and ignore the BIOS if you care about audio capture. 4. no DMICs attached to PCH and BIOS/NHLT reports DMICs -> broken config, we need an option to ignore the BIOS and continue the probe.
Got it, we will do a test on a couple of machines, let us see if we can meet 3 and 4 in reality.
I backported these 6 patches to our 5.0 kernel with the sof driver in it, and tested on 3 machines which have dmic connected to PCH (audio controller pciid 0x9dc8), without these 6 patches, I need to blacklist snd_hda_intel.ko and snd_soc_skl.ko to make the sof driver work, after backporting these 6 patches, I don't need to blacklist snd_hda_intel.ko, but still need to blacklist snd_soc_skl.ko, otherwise the sof driver doesn't work.
Thanks for testing. I've done additional updates on my side and I am reasonably confident we can make this SOF/legacy autodetect work.
And I also tested these 6 patches on 3 machines without dmic, I don't need to blacklist anything, the audio works well via legacy hdaudio.
So for coexistence of soc_skl and soc_sof drivers, do we have any plan?
HDaudio is not well supported by the SKL driver. We introduced this support in v4.19, but there are quite a few issues on specific platforms that are not well handled (questionable probe which lead to breaking audio on Linus' laptop, interrupt issues fixed with SOF and legacy, etc). Since SOF is well supported on those platforms, I am honestly leaning to de-feature HDAudio support from the SKL driver (as in make the HDaudio codec Kconfig option not recommended) and apply the same trick as for the legacy to abort the probe if there is no I2S codec and DMICs are detected. It would have no effect on any users and would help distros like Ubuntu avoid the need for blacklists. We can also apply DMI-based quirks for Chrome, e.g. for now all platforms prior to GLK should use SKL and more recent ones SOF.
As far as the functionality is kept from use POV, it should be fine. And yes, blacklisting is no primary option.
I guess we can start tweaking Kconfig to disable HD-audio on SKL SSL while keeping the code intact.
thanks,
Takashi
participants (4)
-
Daniel Drake -
Hui Wang -
Pierre-Louis Bossart -
Takashi Iwai