[PATCH] ALSA: hda/i915 - skip acomp init if no matching display

[PATCH] ALSA: hda/i915 - skip acomp init if no matching display

[Kai Vehmanen]

In systems with only a discrete i915 GPU, the acomp init will
always timeout for the PCH HDA controller instance.

Avoid the timeout by checking the PCI device hierarchy
whether any display class PCI device can be found on the system,
and at the same level as the HDA PCI device. If found, proceed
with the acomp init, which will wait until i915 probe is complete
and component binding can proceed. If no matching display
device is found, the audio component bind can be safely skipped.

The bind timeout will still be hit if the display is present
in the system, but i915 driver does not bind to it by configuration
choice or probe error. In this case the 60sec timeout will be
hit.

Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 sound/hda/hdac_i915.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index efe810af28c5..55b61b1a0ef9 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -116,16 +116,25 @@ static int i915_component_master_match(struct device *dev, int subcomponent,
 	return 0;
 }
 
-/* check whether intel graphics is present */
-static bool i915_gfx_present(void)
+/* check whether Intel graphics is present and reachable */
+static int i915_gfx_present(struct pci_dev *hdac_pci)
 {
-	static const struct pci_device_id ids[] = {
-		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_ANY_ID),
-		  .class = PCI_BASE_CLASS_DISPLAY << 16,
-		  .class_mask = 0xff << 16 },
-		{}
-	};
-	return pci_dev_present(ids);
+	unsigned int class = PCI_BASE_CLASS_DISPLAY << 16;
+	struct pci_dev *display_dev = NULL;
+	bool match = false;
+
+	do {
+		display_dev = pci_get_class(class, display_dev);
+		if (display_dev)
+			if (display_dev->vendor == PCI_VENDOR_ID_INTEL &&
+			    connectivity_check(display_dev, hdac_pci))
+				match = true;
+
+		pci_dev_put(display_dev);
+
+	} while (!match && display_dev);
+
+	return match;
 }
 
 /**
@@ -145,7 +154,7 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
 	struct drm_audio_component *acomp;
 	int err;
 
-	if (!i915_gfx_present())
+	if (!i915_gfx_present(to_pci_dev(bus->dev)))
 		return -ENODEV;
 
 	err = snd_hdac_acomp_init(bus, NULL,

base-commit: bfa1e1a62c8bdbe3d8c915fbb7a078dc783b2ee8
-- 
2.35.1

Re: [PATCH] ALSA: hda/i915 - skip acomp init if no matching display

[Lucas De Marchi]

On Mon, Apr 04, 2022 at 04:03:56PM +0300, Kai Vehmanen wrote:
>In systems with only a discrete i915 GPU, the acomp init will
>always timeout for the PCH HDA controller instance.
>
>Avoid the timeout by checking the PCI device hierarchy
>whether any display class PCI device can be found on the system,
>and at the same level as the HDA PCI device. If found, proceed
>with the acomp init, which will wait until i915 probe is complete
>and component binding can proceed. If no matching display
>device is found, the audio component bind can be safely skipped.
>
>The bind timeout will still be hit if the display is present
>in the system, but i915 driver does not bind to it by configuration
>choice or probe error. In this case the 60sec timeout will be
>hit.
>
>Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
>Acked-by: Lucas De Marchi <lucas.demarchi@intel.com>
>---
> sound/hda/hdac_i915.c | 29 +++++++++++++++++++----------
> 1 file changed, 19 insertions(+), 10 deletions(-)
>
>diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
>index efe810af28c5..55b61b1a0ef9 100644
>--- a/sound/hda/hdac_i915.c
>+++ b/sound/hda/hdac_i915.c
>@@ -116,16 +116,25 @@ static int i915_component_master_match(struct device *dev, int subcomponent,
> 	return 0;
> }
>
>-/* check whether intel graphics is present */
>-static bool i915_gfx_present(void)
>+/* check whether Intel graphics is present and reachable */
>+static int i915_gfx_present(struct pci_dev *hdac_pci)
> {
>-	static const struct pci_device_id ids[] = {
>-		{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_ANY_ID),
>-		  .class = PCI_BASE_CLASS_DISPLAY << 16,
>-		  .class_mask = 0xff << 16 },
>-		{}
>-	};
>-	return pci_dev_present(ids);
>+	unsigned int class = PCI_BASE_CLASS_DISPLAY << 16;
>+	struct pci_dev *display_dev = NULL;
>+	bool match = false;
>+
>+	do {
>+		display_dev = pci_get_class(class, display_dev);
>+		if (display_dev)
>+			if (display_dev->vendor == PCI_VENDOR_ID_INTEL &&
>+			    connectivity_check(display_dev, hdac_pci))

why not put this all in a single if level?

		if (display_dev && display_dev->vendor == PCI_VENDOR_ID_INTEL &&
		    connectivity_check(display_dev, hdac_pci))

basically the first check protects dereferencing the variable.

Lucas De Marchi

>+				match = true;
>+
>+		pci_dev_put(display_dev);
>+
>+	} while (!match && display_dev);
>+
>+	return match;
> }
>
> /**
>@@ -145,7 +154,7 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
> 	struct drm_audio_component *acomp;
> 	int err;
>
>-	if (!i915_gfx_present())
>+	if (!i915_gfx_present(to_pci_dev(bus->dev)))
> 		return -ENODEV;
>
> 	err = snd_hdac_acomp_init(bus, NULL,
>
>base-commit: bfa1e1a62c8bdbe3d8c915fbb7a078dc783b2ee8
>-- 
>2.35.1
>