Re: [PATCH 2/8] x86: Add Intel graphics stolen memory quirk for gen2 platforms

[Ingo Molnar <mingo@kernel.org>]

* ville.syrjala@linux.intel.com <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> There doesn't seem to an explicit stolen memory base register on gen2.
> Some old comment in the i915 code suggests we should get it via
> max_low_pfn_mapped, but that's clearly a bad idea on my MGM.
> 
> The e820 map in said machine looks like this:
> [    0.000000] BIOS-e820: [mem 0x0000000000000000-0x000000000009f7ff] usable
> [    0.000000] BIOS-e820: [mem 0x000000000009f800-0x000000000009ffff] reserved
> [    0.000000] BIOS-e820: [mem 0x00000000000ce000-0x00000000000cffff] reserved
> [    0.000000] BIOS-e820: [mem 0x00000000000dc000-0x00000000000fffff] reserved
> [    0.000000] BIOS-e820: [mem 0x0000000000100000-0x000000001f6effff] usable
> [    0.000000] BIOS-e820: [mem 0x000000001f6f0000-0x000000001f6f7fff] ACPI data
> [    0.000000] BIOS-e820: [mem 0x000000001f6f8000-0x000000001f6fffff] ACPI NVS
> [    0.000000] BIOS-e820: [mem 0x000000001f700000-0x000000001fffffff] reserved
> [    0.000000] BIOS-e820: [mem 0x00000000fec10000-0x00000000fec1ffff] reserved
> [    0.000000] BIOS-e820: [mem 0x00000000ffb00000-0x00000000ffbfffff] reserved
> [    0.000000] BIOS-e820: [mem 0x00000000fff00000-0x00000000ffffffff] reserved
> 
> That makes max_low_pfn_mapped = 1f6f0000, so assuming our stolen memory
> would start there would place it on top of some ACPI memory regions.
> So not a good idea as already stated.
> 
> The 9MB region after the ACPI regions at 0x1f700000 however looks
> promising given that the macine reports the stolen memory size to be
> 8MB. Looking at the PGTBL_CTL register, the GTT entries are at offset
> 0x1fee00000, and given that the GTT entries occupy 128KB, it looks like
> the stolen memory could start at 0x1f700000 and the GTT entries would
> occupy the last 128KB of the stolen memory. I have no idea about the
> extra 1MB after the GTT entries.
> 
> I tested this on the machine in question, and so far I've not seen any
> issues with the use the this memory region. Hopefully the same rules
> hold for all gen2 machines...
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  arch/x86/kernel/early-quirks.c | 70 ++++++++++++++++++++++++++++++++++++++++++
>  include/drm/i915_drm.h         | 12 ++++++++
>  2 files changed, 82 insertions(+)
> 
> diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
> index ca49966..6cd90b4 100644
> --- a/arch/x86/kernel/early-quirks.c
> +++ b/arch/x86/kernel/early-quirks.c
> @@ -247,6 +247,62 @@ static u32 __init intel_stolen_base(int num, int slot, int func, size_t size)
>  #define MB(x)	(KB (KB (x)))
>  #define GB(x)	(MB (KB (x)))
>  
> +/*
> + * Gen2 stolen base is tricky. Try to deduce it from the
> + * page table base address.
> + */
> +static u32 __init gen2_stolen_base(int num, int slot, int func, size_t size)
> +{
> +	void __iomem *regs;
> +	u32 reg_addr;
> +	u32 pgtbl_ctl;
> +
> +	reg_addr = read_pci_config(0, 2, 0, I810_MMADDR) & 0xfff80000;

Why is the mmaddr rounded to 512K? Do the lower 19 bits hold some 
other piece of information, or are they always zero?

> +	regs = early_ioremap(reg_addr, KB(64));
> +	if (!regs)
> +		return 0;

Emitting a warning here might be useful, if anyone runs into this.

> +	pgtbl_ctl = readl(regs + I810_PGTBL_CTL);
> +	early_iounmap(regs, KB(64));
> +
> +	/* GTT disabled? */
> +	if (!(pgtbl_ctl & I810_PGTBL_ENABLED))
> +		return 0;
> +
> +	pgtbl_ctl &= I810_PGTBL_ADDRESS_MASK;
> +
> +	/* Assume GTT entries occupy the last 128KB of stolen/local memory */
> +	return pgtbl_ctl + KB(128) - size;

Btw., I love the cleanliness of the KB() / MB() / GB() macros, it 
ought to move from agp.h to include/linux/kernel.h or so.


> +static size_t __init i830_stolen_size(int num, int slot, int func)
> +{
> +	size_t stolen_size;
> +	u16 gmch_ctrl;
> +
> +	gmch_ctrl = read_pci_config_16(0, 0, 0, I830_GMCH_CTRL);
> +
> +	switch (gmch_ctrl & I830_GMCH_GMS_MASK) {
> +	case I830_GMCH_GMS_STOLEN_512:
> +		stolen_size = KB(512);
> +		break;
> +	case I830_GMCH_GMS_STOLEN_1024:
> +		stolen_size = MB(1);
> +		break;
> +	case I830_GMCH_GMS_STOLEN_8192:
> +		stolen_size = MB(8);
> +		break;
> +	case I830_GMCH_GMS_LOCAL:
> +		/* local memory isn't part of the normal address space */
> +		stolen_size = 0;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	return stolen_size;

Nit: looks like the labels 'I830_GMCH_GMS_LOCAL' and 'default' have 
the same effect so they could be merged.

> diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> index 97d5497..1526546 100644
> --- a/include/drm/i915_drm.h
> +++ b/include/drm/i915_drm.h
> @@ -54,8 +54,16 @@ extern bool i915_gpu_turbo_disable(void);
>  #define    BDW_GMCH_GMS_SHIFT   8
>  #define    BDW_GMCH_GMS_MASK    0xff
>  
> +#define I810_MMADDR		0x14
> +
>  #define I830_GMCH_CTRL			0x52

Nit: the I810_MMADDR define appears to be out of alignment with the 
surrounding constants, is that intentional?

Thanks,

	Ingo