Intel HDMI probe regression on IVB (and older?)

Intel HDMI probe regression on IVB (and older?)

[Takashi Iwai]

Hi,

we've got a regression report about Intel HDMI.  It seems that the
recent change to skip the component binding (commit c9db8a30d9f0)
throws away the devices incorrectly on IvyBridge.  I guess the similar
issue could happen on older chips.  The bug report is found at
  https://bugzilla.opensuse.org/show_bug.cgi?id=1200611

Judging from the logs there, on IvyBridge, the GPU is

00:02.0 0380: 8086:0162 (rev 09)
	DeviceName:  Onboard IGD
	Subsystem: 1043:84ca
	Flags: bus master, fast devsel, latency 0, IRQ 27
	Memory at f7800000 (64-bit, non-prefetchable) [size=4M]
	Memory at d0000000 (64-bit, prefetchable) [size=256M]
	I/O ports at f000 [size=64]
	Capabilities: <access denied>
	Kernel driver in use: i915
	Kernel modules: i915

while HD-audio is

00:1b.0 0403: 8086:1e20 (rev 04)
	Subsystem: 1043:84a8
	Flags: bus master, fast devsel, latency 0, IRQ 31
	Memory at f7d10000 (64-bit, non-prefetchable) [size=16K]
	Capabilities: <access denied>
	Kernel driver in use: snd_hda_intel
	Kernel modules: snd_hda_intel

Unlike Haswell, IVB has no dedicated HDMI audio controller, and it's
connected on HD-audio bus of 00:1b.0.

Kai, could you check this issue?


thanks,

Takashi

Re: Intel HDMI probe regression on IVB (and older?)

[Kai Vehmanen]

Hi,

On Mon, 20 Jun 2022, Takashi Iwai wrote:

> we've got a regression report about Intel HDMI.  It seems that the
> recent change to skip the component binding (commit c9db8a30d9f0)
> throws away the devices incorrectly on IvyBridge.  I guess the similar
> issue could happen on older chips.  The bug report is found at
>   https://bugzilla.opensuse.org/show_bug.cgi?id=1200611

we'll check. We actually have IVB (and older), in the i915 CI and I can 
see the binding check working correctly there (with 5.19-rc2). But 
obviously something goes wrong in the reporter's case, so needs more 
debug.

Br, Kai

Re: Intel HDMI probe regression on IVB (and older?)

[Takashi Iwai]

On Mon, 20 Jun 2022 11:26:52 +0200,
Kai Vehmanen wrote:
> 
> Hi,
> 
> On Mon, 20 Jun 2022, Takashi Iwai wrote:
> 
> > we've got a regression report about Intel HDMI.  It seems that the
> > recent change to skip the component binding (commit c9db8a30d9f0)
> > throws away the devices incorrectly on IvyBridge.  I guess the similar
> > issue could happen on older chips.  The bug report is found at
> >   https://bugzilla.opensuse.org/show_bug.cgi?id=1200611
> 
> we'll check. We actually have IVB (and older), in the i915 CI and I can 
> see the binding check working correctly there (with 5.19-rc2). But 
> obviously something goes wrong in the reporter's case, so needs more 
> debug.

So this looks like a bug due to the use of pci_get_class().
Since there is no pci_get_base_class(), we likely need to open-code
the search, e.g. something like below.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: hda: Fix discovery of i915 graphics PCI device

It's been reported that the recent fix for skipping the
component-binding with D-GPU caused a regression on some systems; it
resulted in the completely missing component binding with i915 GPU.

The problem was the use of pci_get_class() function.  It matches with
the full PCI class bits, while we want to match only partially the PCI
base class bits.  So, when a system has an i915 graphics device with
the PCI class 0380, it won't hit because we're looking for only the
PCI class 0300.

This patch fixes i915_gfx_present() to look up each PCI device and
match with PCI base class explicitly instead of pci_get_class().

Fixes: c9db8a30d9f0 ("ALSA: hda/i915 - skip acomp init if no matching display")
Link: https://bugzilla.opensuse.org/show_bug.cgi?id=1200611
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/hda/hdac_i915.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 3f35972e1cf7..161a9711cd63 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -119,21 +119,18 @@ static int i915_component_master_match(struct device *dev, int subcomponent,
 /* check whether Intel graphics is present and reachable */
 static int i915_gfx_present(struct pci_dev *hdac_pci)
 {
-	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 && display_dev->vendor == PCI_VENDOR_ID_INTEL &&
+	for_each_pci_dev(display_dev) {
+		if (display_dev->vendor == PCI_VENDOR_ID_INTEL &&
+		    (display_dev->class >> 16) == PCI_BASE_CLASS_DISPLAY &&
 		    connectivity_check(display_dev, hdac_pci)) {
 			pci_dev_put(display_dev);
-			match = true;
+			return true;
 		}
-	} while (!match && display_dev);
+	}
 
-	return match;
+	return false;
 }
 
 /**
-- 
2.35.3

Re: Intel HDMI probe regression on IVB (and older?)

[Kai Vehmanen]

Hi,

On Mon, 20 Jun 2022, Takashi Iwai wrote:

> So this looks like a bug due to the use of pci_get_class().
> Since there is no pci_get_base_class(), we likely need to open-code
> the search, e.g. something like below.

yes, this indeed seems to be the case.

> diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> index 3f35972e1cf7..161a9711cd63 100644
> --- a/sound/hda/hdac_i915.c
> +++ b/sound/hda/hdac_i915.c
> @@ -119,21 +119,18 @@ static int i915_component_master_match(struct device *dev, int subcomponent,
[...]
> -	do {
> -		display_dev = pci_get_class(class, display_dev);
> -
> -		if (display_dev && display_dev->vendor == PCI_VENDOR_ID_INTEL &&
> +	for_each_pci_dev(display_dev) {
> +		if (display_dev->vendor == PCI_VENDOR_ID_INTEL &&
> +		    (display_dev->class >> 16) == PCI_BASE_CLASS_DISPLAY &&
>  		    connectivity_check(display_dev, hdac_pci)) {
>  			pci_dev_put(display_dev);
> -			match = true;
> +			return true;
>  		}
> -	} while (!match && display_dev);
> +	}

To open code a bit less, I was first thinking:

--cut--
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -124,9 +124,9 @@ static int i915_gfx_present(struct pci_dev *hdac_pci)
        bool match = false;
 
        do {
-               display_dev = pci_get_class(class, display_dev);
+               display_dev = pci_get_device(PCI_VENDOR_ID_INTEL, PCI_ANY_ID, display_dev);
 
-               if (display_dev && display_dev->vendor == PCI_VENDOR_ID_INTEL &&
+               if (display_dev && (display_dev->class >> 16) == PCI_BASE_CLASS_DISPLAY &&
                    connectivity_check(display_dev, hdac_pci)) {
--cut--

But it's a marginal difference, so for your version:

Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

Br, Kai

Re: Intel HDMI probe regression on IVB (and older?)

[Takashi Iwai]

On Mon, 20 Jun 2022 17:31:09 +0200,
Kai Vehmanen wrote:
> 
> Hi,
> 
> On Mon, 20 Jun 2022, Takashi Iwai wrote:
> 
> > So this looks like a bug due to the use of pci_get_class().
> > Since there is no pci_get_base_class(), we likely need to open-code
> > the search, e.g. something like below.
> 
> yes, this indeed seems to be the case.
> 
> > diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> > index 3f35972e1cf7..161a9711cd63 100644
> > --- a/sound/hda/hdac_i915.c
> > +++ b/sound/hda/hdac_i915.c
> > @@ -119,21 +119,18 @@ static int i915_component_master_match(struct device *dev, int subcomponent,
> [...]
> > -	do {
> > -		display_dev = pci_get_class(class, display_dev);
> > -
> > -		if (display_dev && display_dev->vendor == PCI_VENDOR_ID_INTEL &&
> > +	for_each_pci_dev(display_dev) {
> > +		if (display_dev->vendor == PCI_VENDOR_ID_INTEL &&
> > +		    (display_dev->class >> 16) == PCI_BASE_CLASS_DISPLAY &&
> >  		    connectivity_check(display_dev, hdac_pci)) {
> >  			pci_dev_put(display_dev);
> > -			match = true;
> > +			return true;
> >  		}
> > -	} while (!match && display_dev);
> > +	}
> 
> To open code a bit less, I was first thinking:
> 
> --cut--
> --- a/sound/hda/hdac_i915.c
> +++ b/sound/hda/hdac_i915.c
> @@ -124,9 +124,9 @@ static int i915_gfx_present(struct pci_dev *hdac_pci)
>         bool match = false;
>  
>         do {
> -               display_dev = pci_get_class(class, display_dev);
> +               display_dev = pci_get_device(PCI_VENDOR_ID_INTEL, PCI_ANY_ID, display_dev);
>  
> -               if (display_dev && display_dev->vendor == PCI_VENDOR_ID_INTEL &&
> +               if (display_dev && (display_dev->class >> 16) == PCI_BASE_CLASS_DISPLAY &&
>                     connectivity_check(display_dev, hdac_pci)) {
> --cut--
> 
> But it's a marginal difference, so for your version:
> 
> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

OK, could you throw the patch to CI for verification?
I can merge it for the next pull request (probably in this week) once
after confirmation. 


thanks,

Takashi

Re: Intel HDMI probe regression on IVB (and older?)

[Kai Vehmanen]

Hi,

On Mon, 20 Jun 2022, Takashi Iwai wrote:

> > But it's a marginal difference, so for your version:
> > 
> > Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> 
> OK, could you throw the patch to CI for verification?
> I can merge it for the next pull request (probably in this week) once
> after confirmation. 

sure thing, I'll confirm later this week.

Br, Kai

Re: Intel HDMI probe regression on IVB (and older?)

[Kai Vehmanen]

Hi,

On Mon, 20 Jun 2022, Kai Vehmanen wrote:

> On Mon, 20 Jun 2022, Takashi Iwai wrote:
> 
> > > But it's a marginal difference, so for your version:
> > > 
> > > Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> > 
> > OK, could you throw the patch to CI for verification?
> > I can merge it for the next pull request (probably in this week) once
> > after confirmation. 
> 
> sure thing, I'll confirm later this week.

tests are clean for the patch so

Tested-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

Did a bit of analysis why this was not caught in testing earlier. IVB (and 
older) systems are covered in the intel-gfx CI, but it turns out PCI class 
is 0300 (VGA compatible adapter) on all of these. But alas on the 
reporter's system, PCI_CLASS_DISPLAY_OTHER was used, so the problem is 
definitely there and the fix is needed.

Br, Kai

Re: Intel HDMI probe regression on IVB (and older?)

[Takashi Iwai]

On Tue, 21 Jun 2022 13:36:04 +0200,
Kai Vehmanen wrote:
> 
> Hi,
> 
> On Mon, 20 Jun 2022, Kai Vehmanen wrote:
> 
> > On Mon, 20 Jun 2022, Takashi Iwai wrote:
> > 
> > > > But it's a marginal difference, so for your version:
> > > > 
> > > > Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> > > 
> > > OK, could you throw the patch to CI for verification?
> > > I can merge it for the next pull request (probably in this week) once
> > > after confirmation. 
> > 
> > sure thing, I'll confirm later this week.
> 
> tests are clean for the patch so
> 
> Tested-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> 
> Did a bit of analysis why this was not caught in testing earlier. IVB (and 
> older) systems are covered in the intel-gfx CI, but it turns out PCI class 
> is 0300 (VGA compatible adapter) on all of these. But alas on the 
> reporter's system, PCI_CLASS_DISPLAY_OTHER was used, so the problem is 
> definitely there and the fix is needed.

Great, thanks for the update.  I submitted the proper patch now.


Takashi