| Message ID | 20220107210516.907834-3-lucas.demarchi@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/3] x86/quirks: Replace QFLAG_APPLY_ONCE with static locals | expand |
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. > --- > > 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 >
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? > > > --- > > > > 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 > >
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 > > >
On Mon, Jan 10, 2022 at 11:32:11AM -0600, Bjorn Helgaas wrote: > 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. Lucas brought a good point about patch 1 touching more stuff than i915 one, so to minimize conflicts maybe the x86 tree would be better... also works for us if you prefer. > > 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 > > > >
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; } }
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> --- 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(-)