From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ingo Molnar Subject: Re: [PATCH 2/8] x86: Add Intel graphics stolen memory quirk for gen2 platforms Date: Sat, 30 Nov 2013 13:58:33 +0100 Message-ID: <20131130125833.GB12143@gmail.com> References: <1385651710-7768-1-git-send-email-ville.syrjala@linux.intel.com> <1385651710-7768-3-git-send-email-ville.syrjala@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-bk0-f45.google.com (mail-bk0-f45.google.com [209.85.214.45]) by gabe.freedesktop.org (Postfix) with ESMTP id 2C23BFA604 for ; Sat, 30 Nov 2013 04:58:36 -0800 (PST) Received: by mail-bk0-f45.google.com with SMTP id mx13so4673947bkb.32 for ; Sat, 30 Nov 2013 04:58:36 -0800 (PST) Content-Disposition: inline In-Reply-To: <1385651710-7768-3-git-send-email-ville.syrjala@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: ville.syrjala@linux.intel.com Cc: x86@kernel.org, intel-gfx@lists.freedesktop.org, Ingo Molnar , Thomas Gleixner , "H. Peter Anvin" List-Id: intel-gfx@lists.freedesktop.org * ville.syrjala@linux.intel.com wrote: > From: Ville Syrj=E4l=E4 > = > 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] usa= ble > [ 0.000000] BIOS-e820: [mem 0x000000000009f800-0x000000000009ffff] res= erved > [ 0.000000] BIOS-e820: [mem 0x00000000000ce000-0x00000000000cffff] res= erved > [ 0.000000] BIOS-e820: [mem 0x00000000000dc000-0x00000000000fffff] res= erved > [ 0.000000] BIOS-e820: [mem 0x0000000000100000-0x000000001f6effff] usa= ble > [ 0.000000] BIOS-e820: [mem 0x000000001f6f0000-0x000000001f6f7fff] ACP= I data > [ 0.000000] BIOS-e820: [mem 0x000000001f6f8000-0x000000001f6fffff] ACP= I NVS > [ 0.000000] BIOS-e820: [mem 0x000000001f700000-0x000000001fffffff] res= erved > [ 0.000000] BIOS-e820: [mem 0x00000000fec10000-0x00000000fec1ffff] res= erved > [ 0.000000] BIOS-e820: [mem 0x00000000ffb00000-0x00000000ffbfffff] res= erved > [ 0.000000] BIOS-e820: [mem 0x00000000fff00000-0x00000000ffffffff] res= erved > = > That makes max_low_pfn_mapped =3D 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 > Cc: Ingo Molnar > Cc: "H. Peter Anvin" > Cc: x86@kernel.org > Signed-off-by: Ville Syrj=E4l=E4 > --- > 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-quirk= s.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 slo= t, 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 s= ize) > +{ > + void __iomem *regs; > + u32 reg_addr; > + u32 pgtbl_ctl; > + > + reg_addr =3D 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 =3D early_ioremap(reg_addr, KB(64)); > + if (!regs) > + return 0; Emitting a warning here might be useful, if anyone runs into this. > + pgtbl_ctl =3D readl(regs + I810_PGTBL_CTL); > + early_iounmap(regs, KB(64)); > + > + /* GTT disabled? */ > + if (!(pgtbl_ctl & I810_PGTBL_ENABLED)) > + return 0; > + > + pgtbl_ctl &=3D 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 =3D 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 =3D KB(512); > + break; > + case I830_GMCH_GMS_STOLEN_1024: > + stolen_size =3D MB(1); > + break; > + case I830_GMCH_GMS_STOLEN_8192: > + stolen_size =3D MB(8); > + break; > + case I830_GMCH_GMS_LOCAL: > + /* local memory isn't part of the normal address space */ > + stolen_size =3D 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