[RFC PATCH 0/6] ALSA/HDA: abort probe when DMICs are detected

[RFC PATCH 0/6] ALSA/HDA: abort probe when DMICs are detected

[Pierre-Louis Bossart]

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%)

-- 
2.20.1

[RFC PATCH 1/6] ASoC: Intel: boards: remove unnecessary inclusion of skl.h

[Pierre-Louis Bossart]

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
-- 
2.20.1

[RFC PATCH 2/6] ASoC: Intel: Skylake: move NHLT header to common directory

[Pierre-Louis Bossart]

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
-- 
2.20.1

[RFC PATCH 3/6] ASoC: Intel: common: move parts of NHLT code to new module

[Pierre-Louis Bossart]

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
-- 
2.20.1

[RFC PATCH 4/6] ASoC: Intel: Skylake: use common NHLT module

[Pierre-Louis Bossart]

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);
-- 
2.20.1

[RFC PATCH 5/6] ALSA / hda: stop probe if DMICS are detected on Skylake+ platforms

[Pierre-Louis Bossart]

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);
-- 
2.20.1

[RFC PATCH 6/6] [HACK] ASoC: Intel: NHLT: handle VENDOR_DEFINED DMIC geometry

[Pierre-Louis Bossart]

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);
-- 
2.20.1

Re: [RFC PATCH 2/6] ASoC: Intel: Skylake: move NHLT header to common directory

[Takashi Iwai]

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

Re: [RFC PATCH 3/6] ASoC: Intel: common: move parts of NHLT code to new module

[Takashi Iwai]

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
> 

Re: [RFC PATCH 5/6] ALSA / hda: stop probe if DMICS are detected on Skylake+ platforms

[Takashi Iwai]

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

Re: [RFC PATCH 0/6] ALSA/HDA: abort probe when DMICs are detected

[Takashi Iwai]

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

Re: [RFC PATCH 2/6] ASoC: Intel: Skylake: move NHLT header to common directory

[Pierre-Louis Bossart]

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?

Re: [RFC PATCH 2/6] ASoC: Intel: Skylake: move NHLT header to common directory

[Takashi Iwai]

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

Re: [RFC PATCH 3/6] ASoC: Intel: common: move parts of NHLT code to new module

[Pierre-Louis Bossart]

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.

Re: [RFC PATCH 5/6] ALSA / hda: stop probe if DMICS are detected on Skylake+ platforms

[Pierre-Louis Bossart]

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!

Re: [RFC PATCH 0/6] ALSA/HDA: abort probe when DMICs are detected

[Pierre-Louis Bossart]

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.

Re: [RFC PATCH 3/6] ASoC: Intel: common: move parts of NHLT code to new module

[Takashi Iwai]

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

Re: [RFC PATCH 0/6] ALSA/HDA: abort probe when DMICs are detected

[Takashi Iwai]

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

Re: [RFC PATCH 0/6] ALSA/HDA: abort probe when DMICs are detected

[Daniel Drake]

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

Re: [RFC PATCH 0/6] ALSA/HDA: abort probe when DMICs are detected

[Pierre-Louis Bossart]

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?

Re: [RFC PATCH 0/6] ALSA/HDA: abort probe when DMICs are detected

[Daniel Drake]

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.
> [...]
> 1. 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

Re: [RFC PATCH 0/6] ALSA/HDA: abort probe when DMICs are detected

[Pierre-Louis Bossart]

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.
>> [...]
>> 1. 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.

Re: [RFC PATCH 0/6] ALSA/HDA: abort probe when DMICs are detected

[Daniel Drake]

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

Re: [RFC PATCH 0/6] ALSA/HDA: abort probe when DMICs are detected

[Hui Wang]

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:
>
> 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.

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?
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC PATCH 0/6] ALSA/HDA: abort probe when DMICs are detected

[Hui Wang]

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:
>>
>> 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.
>
> 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?
>>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC PATCH 0/6] ALSA/HDA: abort probe when DMICs are detected

[Pierre-Louis Bossart]

>>>>> 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.
>>
>> 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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC PATCH 0/6] ALSA/HDA: abort probe when DMICs are detected

[Takashi Iwai]

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:
> >>>
> >>> 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.
> >>
> >> 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel