Re: [Intel-gfx] [PATCH v3 3/3] x86/quirks: Fix stolen detection with integrated + discrete GPU

From: Bjorn Helgaas <helgaas@kernel.org>
To: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	linux-pci@vger.kernel.org, intel-gfx@lists.freedesktop.org,
	x86@kernel.org, Ingo Molnar <mingo@redhat.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [Intel-gfx] [PATCH v3 3/3] x86/quirks: Fix stolen detection with integrated + discrete GPU
Date: Mon, 10 Jan 2022 11:32:11 -0600	[thread overview]
Message-ID: <20220110173211.GA61717@bhelgaas> (raw)
In-Reply-To: <YdxoyHIYssuJjN4w@intel.com>

On Mon, Jan 10, 2022 at 12:11:36PM -0500, Rodrigo Vivi wrote:
> On Fri, Jan 07, 2022 at 08:57:32PM -0600, Bjorn Helgaas wrote:
> > On Fri, Jan 07, 2022 at 01:05:16PM -0800, Lucas De Marchi wrote:
> > > early_pci_scan_bus() does a depth-first traversal, possibly calling
> > > the quirk functions for each device based on vendor, device and class
> > > from early_qrk table. intel_graphics_quirks() however uses PCI_ANY_ID
> > > and does additional filtering in the quirk.
> > > 
> > > If there is an Intel integrated + discrete GPU the quirk may be called
> > > first for the discrete GPU based on the PCI topology. Then we will fail
> > > to reserve the system stolen memory for the integrated GPU, because we
> > > will already have marked the quirk as "applied".
> > > 
> > > This was reproduced in a setup with Alderlake-P (integrated) + DG2
> > > (discrete), with the following PCI topology:
> > > 
> > > 	- 00:01.0 Bridge
> > > 	  `- 03:00.0 DG2
> > > 	- 00:02.0 Integrated GPU
> > > 
> > > Move the setting of quirk_applied in intel_graphics_quirks() so it's
> > > mark as applied only when we find the integrated GPU based on the
> > > intel_early_ids table.
> > > 
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > 
> > I don't know the details of stolen memory, but the implementation of
> > this quirk looks good to me.  Very nice that it's now very clear
> > exactly what the change is.
> 
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Bjorn, ack to merge through drm-intel?

This is a bit of a shared area between the x86 folks and me, but
certainly no objection from me.

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> > > ---
> > > 
> > > v3: now that we do the refactor before the fix, we can do a single line
> > > change to fix intel_graphics_quirks(). Also, we don't change
> > > intel_graphics_stolen() anymore as we did in v2: we don't have to check
> > > other devices anymore if there was a previous match causing
> > > intel_graphics_stolen() to be called (there can only be one integrated
> > > GPU reserving the stolen memory).
> > > 
> > >  arch/x86/kernel/early-quirks.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> > > index df34963e23bf..932f9087c324 100644
> > > --- a/arch/x86/kernel/early-quirks.c
> > > +++ b/arch/x86/kernel/early-quirks.c
> > > @@ -609,8 +609,6 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
> > >  	if (quirk_applied)
> > >  		return;
> > >  
> > > -	quirk_applied = true;
> > > -
> > >  	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
> > >  
> > >  	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
> > > @@ -623,6 +621,8 @@ static void __init intel_graphics_quirks(int num, int slot, int func)
> > >  
> > >  		intel_graphics_stolen(num, slot, func, early_ops);
> > >  
> > > +		quirk_applied = true;
> > > +
> > >  		return;
> > >  	}
> > >  }
> > > -- 
> > > 2.34.1
> > > 