[2/2] drm/i915: Function per early graphics quirk

Commit Message

Joonas Lahtinen April 22, 2016, 11:25 a.m. UTC

Move graphics stolen memory related early quirk into a function to
allow easy adding of other graphics quirks to fix memory maps on
machines running old BIOS versions.

While at it;
- _funcs -> _ops to follow de facto naming
- make the iteration code tad more readable
- remove unused variables

Cc: Chris Wilson <[e-mail]>
Cc: Mika Kuoppala <[e-mail]>
Cc: Ville Syrjälä <[e-mail]>
Cc: Tvrtko Ursulin <[e-mail]>
Signed-off-by: Joonas Lahtinen <[e-mail]>
---
 arch/x86/kernel/early-quirks.c | 187 +++++++++++++++++++++--------------------
 1 file changed, 98 insertions(+), 89 deletions(-)

Comments

Chris Wilson April 22, 2016, 11:55 a.m. UTC | #1

On Fri, Apr 22, 2016 at 02:25:07PM +0300, Joonas Lahtinen wrote:
> Move graphics stolen memory related early quirk into a function to
> allow easy adding of other graphics quirks to fix memory maps on
> machines running old BIOS versions.
> 
> While at it;
> - _funcs -> _ops to follow de facto naming
> - make the iteration code tad more readable
> - remove unused variables
> 
> Cc: Chris Wilson <[e-mail]>
> Cc: Mika Kuoppala <[e-mail]>
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Tvrtko Ursulin <[e-mail]>
> Signed-off-by: Joonas Lahtinen <[e-mail]>

Mechanical again, and less quirky,
Reserved-by: Chris Wilson <[e-mail]>

> -static void __init intel_graphics_stolen(int num, int slot, int func)
> +static void __init
> +intel_graphics_stolen(int num, int slot, int func,
> +		      const struct intel_early_ops *early_ops)
>  {
> +	phys_addr_t base;
>  	size_t size;
> +
> +	size = early_ops->stolen_size(num, slot, func);
> +	base = early_ops->stolen_base(num, slot, func, size);
> +
> +	if (!size || !base)
> +		return;

if (size + base < base)
	return;

Just in case? Hmm, not sure if the integer sizes will always be suitable
for the test, so probably just trust the hardware registers?
-Chris

Joonas Lahtinen April 22, 2016, 12:07 p.m. UTC | #2

On pe, 2016-04-22 at 12:55 +0100, Chris Wilson wrote:
> On Fri, Apr 22, 2016 at 02:25:07PM +0300, Joonas Lahtinen wrote:
> > 
> > Move graphics stolen memory related early quirk into a function to
> > allow easy adding of other graphics quirks to fix memory maps on
> > machines running old BIOS versions.
> > 
> > While at it;
> > - _funcs -> _ops to follow de facto naming
> > - make the iteration code tad more readable
> > - remove unused variables
> > 
> > Cc: Chris Wilson <[e-mail]>
> > Cc: Mika Kuoppala <[e-mail]>
> > Cc: Ville Syrjälä <[e-mail]>
> > Cc: Tvrtko Ursulin <[e-mail]>
> > Signed-off-by: Joonas Lahtinen <[e-mail]>
> Mechanical again, and less quirky,
> Reserved-by: Chris Wilson <[e-mail]>

A new type of R-b? :)

> 
> > 
> > -static void __init intel_graphics_stolen(int num, int slot, int func)
> > +static void __init
> > +intel_graphics_stolen(int num, int slot, int func,
> > +		      const struct intel_early_ops *early_ops)
> >  {
> > +	phys_addr_t base;
> >  	size_t size;
> > +
> > +	size = early_ops->stolen_size(num, slot, func);
> > +	base = early_ops->stolen_base(num, slot, func, size);
> > +
> > +	if (!size || !base)
> > +		return;
> if (size + base < base)
> 	return;
> 
> Just in case? Hmm, not sure if the integer sizes will always be suitable
> for the test, so probably just trust the hardware registers?

(This was actually the test used by earlier version of code too.
Intention was to not change the resulting logic that much. So I'd add
this check as a follow-up.)

I don't see how this could happen, the original hardware registers are
unlikely to cause an overflow of the phys_addr_t. If we want to add a
check, we should rather test against e820_end_of_ram_pfn() or so.

Regards, Joonas

> -Chris
>

Chris Wilson April 22, 2016, 12:17 p.m. UTC | #3

On Fri, Apr 22, 2016 at 03:07:42PM +0300, Joonas Lahtinen wrote:
> On pe, 2016-04-22 at 12:55 +0100, Chris Wilson wrote:
> > On Fri, Apr 22, 2016 at 02:25:07PM +0300, Joonas Lahtinen wrote:
> > > 
> > > Move graphics stolen memory related early quirk into a function to
> > > allow easy adding of other graphics quirks to fix memory maps on
> > > machines running old BIOS versions.
> > > 
> > > While at it;
> > > - _funcs -> _ops to follow de facto naming
> > > - make the iteration code tad more readable
> > > - remove unused variables
> > > 
> > > Cc: Chris Wilson <[e-mail]>
> > > Cc: Mika Kuoppala <[e-mail]>
> > > Cc: Ville Syrjälä <[e-mail]>
> > > Cc: Tvrtko Ursulin <[e-mail]>
> > > Signed-off-by: Joonas Lahtinen <[e-mail]>
> > Mechanical again, and less quirky,
> > Reserved-by: Chris Wilson <[e-mail]>
> 
> A new type of R-b? :)

I'm tired. :|

> > > -static void __init intel_graphics_stolen(int num, int slot, int func)
> > > +static void __init
> > > +intel_graphics_stolen(int num, int slot, int func,
> > > +		      const struct intel_early_ops *early_ops)
> > >  {
> > > +	phys_addr_t base;
> > >  	size_t size;
> > > +
> > > +	size = early_ops->stolen_size(num, slot, func);
> > > +	base = early_ops->stolen_base(num, slot, func, size);
> > > +
> > > +	if (!size || !base)
> > > +		return;
> > if (size + base < base)
> > 	return;
> > 
> > Just in case? Hmm, not sure if the integer sizes will always be suitable
> > for the test, so probably just trust the hardware registers?
> 
> (This was actually the test used by earlier version of code too.
> Intention was to not change the resulting logic that much. So I'd add
> this check as a follow-up.)
> 
> I don't see how this could happen, the original hardware registers are
> unlikely to cause an overflow of the phys_addr_t. If we want to add a
> check, we should rather test against e820_end_of_ram_pfn() or so.

It can't happen. But I know better than to trust a BIOS or our own
interpretation of the guides. From off the top of my head, stolen will
always be below 4G, and doing a sanity check that it is may or may not
be worth the infuriation if it ever moves above 4G.
-Chris

Patch

diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index f169475..bef1376 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -423,11 +423,6 @@  static size_t __init chv_stolen_size(int num, int slot, int func)
 		return (size_t)(gms - 0x17 + 9) * MB(4);
 }
 
-struct intel_stolen_funcs {
-	size_t (*size)(int num, int slot, int func);
-	phys_addr_t (*base)(int num, int slot, int func, size_t size);
-};
-
 static size_t __init gen9_stolen_size(int num, int slot, int func)
 {
 	u16 gmch_ctrl;
@@ -444,115 +439,129 @@  static size_t __init gen9_stolen_size(int num, int slot, int func)
 		return (size_t)(gms - 0xf0 + 1) * MB(4);
 }
 
-typedef size_t (*stolen_size_fn)(int num, int slot, int func);
+struct intel_early_ops {
+	size_t (*stolen_size)(int num, int slot, int func);
+	phys_addr_t (*stolen_base)(int num, int slot, int func, size_t size);
+};
 
-static const struct intel_stolen_funcs i830_stolen_funcs __initconst = {
-	.base = i830_stolen_base,
-	.size = i830_stolen_size,
+static const struct intel_early_ops i830_early_ops __initconst = {
+	.stolen_base = i830_stolen_base,
+	.stolen_size = i830_stolen_size,
 };
 
-static const struct intel_stolen_funcs i845_stolen_funcs __initconst = {
-	.base = i845_stolen_base,
-	.size = i830_stolen_size,
+static const struct intel_early_ops i845_early_ops __initconst = {
+	.stolen_base = i845_stolen_base,
+	.stolen_size = i830_stolen_size,
 };
 
-static const struct intel_stolen_funcs i85x_stolen_funcs __initconst = {
-	.base = i85x_stolen_base,
-	.size = gen3_stolen_size,
+static const struct intel_early_ops i85x_early_ops __initconst = {
+	.stolen_base = i85x_stolen_base,
+	.stolen_size = gen3_stolen_size,
 };
 
-static const struct intel_stolen_funcs i865_stolen_funcs __initconst = {
-	.base = i865_stolen_base,
-	.size = gen3_stolen_size,
+static const struct intel_early_ops i865_early_ops __initconst = {
+	.stolen_base = i865_stolen_base,
+	.stolen_size = gen3_stolen_size,
 };
 
-static const struct intel_stolen_funcs gen3_stolen_funcs __initconst = {
-	.base = gen3_stolen_base,
-	.size = gen3_stolen_size,
+static const struct intel_early_ops gen3_early_ops __initconst = {
+	.stolen_base = gen3_stolen_base,
+	.stolen_size = gen3_stolen_size,
 };
 
-static const struct intel_stolen_funcs gen6_stolen_funcs __initconst = {
-	.base = gen3_stolen_base,
-	.size = gen6_stolen_size,
+static const struct intel_early_ops gen6_early_ops __initconst = {
+	.stolen_base = gen3_stolen_base,
+	.stolen_size = gen6_stolen_size,
 };
 
-static const struct intel_stolen_funcs gen8_stolen_funcs __initconst = {
-	.base = gen3_stolen_base,
-	.size = gen8_stolen_size,
+static const struct intel_early_ops gen8_early_ops __initconst = {
+	.stolen_base = gen3_stolen_base,
+	.stolen_size = gen8_stolen_size,
 };
 
-static const struct intel_stolen_funcs gen9_stolen_funcs __initconst = {
-	.base = gen3_stolen_base,
-	.size = gen9_stolen_size,
+static const struct intel_early_ops gen9_early_ops __initconst = {
+	.stolen_base = gen3_stolen_base,
+	.stolen_size = gen9_stolen_size,
 };
 
-static const struct intel_stolen_funcs chv_stolen_funcs __initconst = {
-	.base = gen3_stolen_base,
-	.size = chv_stolen_size,
+static const struct intel_early_ops chv_early_ops __initconst = {
+	.stolen_base = gen3_stolen_base,
+	.stolen_size = chv_stolen_size,
 };
 
-static const struct pci_device_id intel_stolen_ids[] __initconst = {
-	INTEL_I830_IDS(&i830_stolen_funcs),
-	INTEL_I845G_IDS(&i845_stolen_funcs),
-	INTEL_I85X_IDS(&i85x_stolen_funcs),
-	INTEL_I865G_IDS(&i865_stolen_funcs),
-	INTEL_I915G_IDS(&gen3_stolen_funcs),
-	INTEL_I915GM_IDS(&gen3_stolen_funcs),
-	INTEL_I945G_IDS(&gen3_stolen_funcs),
-	INTEL_I945GM_IDS(&gen3_stolen_funcs),
-	INTEL_VLV_M_IDS(&gen6_stolen_funcs),
-	INTEL_VLV_D_IDS(&gen6_stolen_funcs),
-	INTEL_PINEVIEW_IDS(&gen3_stolen_funcs),
-	INTEL_I965G_IDS(&gen3_stolen_funcs),
-	INTEL_G33_IDS(&gen3_stolen_funcs),
-	INTEL_I965GM_IDS(&gen3_stolen_funcs),
-	INTEL_GM45_IDS(&gen3_stolen_funcs),
-	INTEL_G45_IDS(&gen3_stolen_funcs),
-	INTEL_IRONLAKE_D_IDS(&gen3_stolen_funcs),
-	INTEL_IRONLAKE_M_IDS(&gen3_stolen_funcs),
-	INTEL_SNB_D_IDS(&gen6_stolen_funcs),
-	INTEL_SNB_M_IDS(&gen6_stolen_funcs),
-	INTEL_IVB_M_IDS(&gen6_stolen_funcs),
-	INTEL_IVB_D_IDS(&gen6_stolen_funcs),
-	INTEL_HSW_D_IDS(&gen6_stolen_funcs),
-	INTEL_HSW_M_IDS(&gen6_stolen_funcs),
-	INTEL_BDW_M_IDS(&gen8_stolen_funcs),
-	INTEL_BDW_D_IDS(&gen8_stolen_funcs),
-	INTEL_CHV_IDS(&chv_stolen_funcs),
-	INTEL_SKL_IDS(&gen9_stolen_funcs),
-	INTEL_BXT_IDS(&gen9_stolen_funcs),
-	INTEL_KBL_IDS(&gen9_stolen_funcs),
+static const struct pci_device_id intel_early_ids[] __initconst = {
+	INTEL_I830_IDS(&i830_early_ops),
+	INTEL_I845G_IDS(&i845_early_ops),
+	INTEL_I85X_IDS(&i85x_early_ops),
+	INTEL_I865G_IDS(&i865_early_ops),
+	INTEL_I915G_IDS(&gen3_early_ops),
+	INTEL_I915GM_IDS(&gen3_early_ops),
+	INTEL_I945G_IDS(&gen3_early_ops),
+	INTEL_I945GM_IDS(&gen3_early_ops),
+	INTEL_VLV_M_IDS(&gen6_early_ops),
+	INTEL_VLV_D_IDS(&gen6_early_ops),
+	INTEL_PINEVIEW_IDS(&gen3_early_ops),
+	INTEL_I965G_IDS(&gen3_early_ops),
+	INTEL_G33_IDS(&gen3_early_ops),
+	INTEL_I965GM_IDS(&gen3_early_ops),
+	INTEL_GM45_IDS(&gen3_early_ops),
+	INTEL_G45_IDS(&gen3_early_ops),
+	INTEL_IRONLAKE_D_IDS(&gen3_early_ops),
+	INTEL_IRONLAKE_M_IDS(&gen3_early_ops),
+	INTEL_SNB_D_IDS(&gen6_early_ops),
+	INTEL_SNB_M_IDS(&gen6_early_ops),
+	INTEL_IVB_M_IDS(&gen6_early_ops),
+	INTEL_IVB_D_IDS(&gen6_early_ops),
+	INTEL_HSW_D_IDS(&gen6_early_ops),
+	INTEL_HSW_M_IDS(&gen6_early_ops),
+	INTEL_BDW_M_IDS(&gen8_early_ops),
+	INTEL_BDW_D_IDS(&gen8_early_ops),
+	INTEL_CHV_IDS(&chv_early_ops),
+	INTEL_SKL_IDS(&gen9_early_ops),
+	INTEL_BXT_IDS(&gen9_early_ops),
+	INTEL_KBL_IDS(&gen9_early_ops),
 };
 
-static void __init intel_graphics_stolen(int num, int slot, int func)
+static void __init
+intel_graphics_stolen(int num, int slot, int func,
+		      const struct intel_early_ops *early_ops)
 {
+	phys_addr_t base;
 	size_t size;
+
+	size = early_ops->stolen_size(num, slot, func);
+	base = early_ops->stolen_base(num, slot, func, size);
+
+	if (!size || !base)
+		return;
+
+	printk(KERN_INFO "Reserving Intel graphics stolen memory at "
+	       "0x%llx-0x%llx\n", base, base + size - 1);
+
+	/* Mark this space as reserved */
+	e820_add_region(base, size, E820_RESERVED);
+	sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &e820.nr_map);
+}
+
+static void __init intel_graphics_quirks(int num, int slot, int func)
+{
+	const struct intel_early_ops *early_ops;
+	u16 device;
 	int i;
-	phys_addr_t start;
-	u16 device, subvendor, subdevice;
 
 	device = read_pci_config_16(num, slot, func, PCI_DEVICE_ID);
-	subvendor = read_pci_config_16(num, slot, func,
-				       PCI_SUBSYSTEM_VENDOR_ID);
-	subdevice = read_pci_config_16(num, slot, func, PCI_SUBSYSTEM_ID);
-
-	for (i = 0; i < ARRAY_SIZE(intel_stolen_ids); i++) {
-		if (intel_stolen_ids[i].device == device) {
-			const struct intel_stolen_funcs *stolen_funcs =
-				(const struct intel_stolen_funcs *)intel_stolen_ids[i].driver_data;
-			size = stolen_funcs->size(num, slot, func);
-			start = stolen_funcs->base(num, slot, func, size);
-			if (size && start) {
-				printk(KERN_INFO "Reserving Intel graphics stolen memory at 0x%llx-0x%llx\n",
-				       start, start + size - 1);
-				/* Mark this space as reserved */
-				e820_add_region(start, size, E820_RESERVED);
-				sanitize_e820_map(e820.map,
-						  ARRAY_SIZE(e820.map),
-						  &e820.nr_map);
-			}
-			return;
-		}
+
+	for (i = 0; i < ARRAY_SIZE(intel_early_ids); i++) {
+		kernel_ulong_t driver_data = intel_early_ids[i].driver_data;
+
+		if (intel_early_ids[i].device != device)
+			continue;
+
+		early_ops = (typeof(early_ops))driver_data;
+
+		intel_graphics_stolen(num, slot, func, early_ops);
+
+		return;
 	}
 }
 
@@ -601,7 +610,7 @@  static struct chipset early_qrk[] __initdata = {
 	{ PCI_VENDOR_ID_INTEL, 0x3406, PCI_CLASS_BRIDGE_HOST,
 	  PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
 	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, PCI_CLASS_DISPLAY_VGA, PCI_ANY_ID,
-	  QFLAG_APPLY_ONCE, intel_graphics_stolen },
+	  QFLAG_APPLY_ONCE, intel_graphics_quirks },
 	/*
 	 * HPET on the current version of the Baytrail platform has accuracy
 	 * problems: it will halt in deep idle state - so we disable it.