[Intel-gfx] [PATCH 2/2] x86: add early quirk for reserving Intel graphics stolen memory

[Jesse Barnes]

On Wed, 24 Jul 2013 18:59:02 +0100
Chris Wilson <chris at chris-wilson.co.uk> wrote:
> > + * And yes, so far on current devices the base addr is always under 4G.
> > + */
> 
> And for the indefinite future.

Yeah going to 64 bits will require code changes too, I just wanted to
head off that inevitable comment.

> > +	base = read_pci_config_32(num, slot, func, 0x5c);
> > +	base &= ~((1<<20) - 1);
> 
> I wish there was some synatic sugar we can apply for base &= -MB(1);

I think it's clear enough.  Or is this a snarky comment that kernel.h
has a macro for this you're not mentioning? :)

> > +
> > +	return base;
> > +}
> > +
> > +#define KB(x)	((x) * 1024)
> > +#define MB(x)	(KB (KB (x)))
> > +#define GB(x)	(MB (KB (x)))
> > +
> > +static size_t __init intel_stolen_size(int num, int slot, int func)
> 
> SNB+ has a different defintion. (And IVB has multiple definitions in the
> bspec, but we've found the SNB one works best...)
> 
> > +static struct pci_device_id intel_stolen_ids[] __initdata = {
> 
> const? Or is initdata already applying that fixup?

const and __initdata don't get along because then the compiler doesn't
know which section to put the array in.

> 
> > +static void __init intel_graphics_stolen(int num, int slot, int func)
> > +{
> > +	size_t size;
> > +	int i;
> > +	u32 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 &&
> > +		    (intel_stolen_ids[i].subvendor == PCI_ANY_ID ||
> > +		     intel_stolen_ids[i].subvendor == subvendor) &&
> > +		    (intel_stolen_ids[i].subdevice == PCI_ANY_ID ||
> > +		     intel_stolen_ids[i].subdevice == subdevice)) {
> > +			size = intel_stolen_size(num, slot, func);
> 
> See above comments for needing to split this on before/after SNB. And
> wouldn't it be cool to unifying the SNB quirk dispatch with this table?
> Is that even possible?

I looked at it, there's not much in common aside from the PCI IDs.  The
SNB reservation occurs after the memory allocator is initialized (so we
can steal the pages).  It may be possible to do it earlier along with
this code, but I'd say that's a separate patch either way.

> 
> > +			start = intel_stolen_base(num, slot, func);
> > +			if (size && start)
> > +				goto found;
> > +			else
> > +				break;
> > +		}
> > +	}
> > +
> > +	/* No match or invalid data, don't bother reserving */
> > +	return;
> > +found:
> > +	/* Mark this space as reserved */
> > +	e820_add_region(start, size, E820_RESERVED);
> > +	return;
> > +}
> > +
> >  #define QFLAG_APPLY_ONCE 	0x1
> >  #define QFLAG_APPLIED		0x2
> >  #define QFLAG_DONE		(QFLAG_APPLY_ONCE|QFLAG_APPLIED)
> > @@ -241,6 +389,8 @@ static struct chipset early_qrk[] __initdata = {
> >  	  PCI_BASE_CLASS_BRIDGE, 0, intel_remapping_check },
> >  	{ 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 },
> >  	{}
> >  };
> >  
> > diff --git a/arch/x86/pci/early.c b/arch/x86/pci/early.c
> > index d1067d5..a3fc44c 100644
> > --- a/arch/x86/pci/early.c
> > +++ b/arch/x86/pci/early.c
> > @@ -31,6 +31,14 @@ u16 read_pci_config_16(u8 bus, u8 slot, u8 func, u8 offset)
> >  	return v;
> >  }
> >  
> > +u32 read_pci_config_32(u8 bus, u8 slot, u8 func, u8 offset)
> > +{
> > +	u32 v;
> > +	outl(0x80000000 | (bus<<16) | (slot<<11) | (func<<8) | offset, 0xcf8);
> > +	v = inl(0xcfc);
> > +	return v;
> > +}
> 
> This is just read_pci_config().

Oops missed it probably due to the consistent naming (_byte, _16, maybe
we should add a _24).  Will drop that bit.

-- 
Jesse Barnes, Intel Open Source Technology Center