From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 02/18] drm/i915: Fix detection of stolen base for gen2 Date: Thu, 1 Nov 2012 16:51:02 -0700 Message-ID: <20121101165102.3cc9773c@bwidawsk.net> References: <1350666204-8101-1-git-send-email-chris@chris-wilson.co.uk> <1350666204-8101-2-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from shiva.chad-versace.us (209-20-75-48.static.cloud-ips.com [209.20.75.48]) by gabe.freedesktop.org (Postfix) with ESMTP id 97BEA9E794 for ; Thu, 1 Nov 2012 16:51:10 -0700 (PDT) In-Reply-To: <1350666204-8101-2-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, 19 Oct 2012 18:03:08 +0100 Chris Wilson wrote: > It was not until the G33 refresh, that a PCI config register was > introduced that explicitly said where the stolen memory was. Prior to > 865G there was not even a register that said where the end of usable > low memory was and where the stolen memory began (or ended depending > upon chipset). Before then, one has to look at the BIOS memory maps to > find the Top of Memory. Alas that is not exported by arch/x86 and so we > have to resort to disabling stolen memory on gen2 for the time being. > > Signed-off-by: Chris Wilson > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_gem_stolen.c | 69 ++++++++++++++------------------ > 2 files changed, 31 insertions(+), 39 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 4728d30..687f379 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -705,6 +705,7 @@ typedef struct drm_i915_private { > unsigned long gtt_start; > unsigned long gtt_mappable_end; > unsigned long gtt_end; > + unsigned long stolen_base; /* limited to low memory (32-bit) */ > > struct io_mapping *gtt_mapping; > phys_addr_t gtt_base_addr; > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > index ada2e90..a01ff74 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -43,56 +43,43 @@ > * for is a boon. > */ > > -#define PTE_ADDRESS_MASK 0xfffff000 > -#define PTE_ADDRESS_MASK_HIGH 0x000000f0 /* i915+ */ > -#define PTE_MAPPING_TYPE_UNCACHED (0 << 1) > -#define PTE_MAPPING_TYPE_DCACHE (1 << 1) /* i830 only */ > -#define PTE_MAPPING_TYPE_CACHED (3 << 1) > -#define PTE_MAPPING_TYPE_MASK (3 << 1) > -#define PTE_VALID (1 << 0) > - > -/** > - * i915_stolen_to_phys - take an offset into stolen memory and turn it into > - * a physical one > - * @dev: drm device > - * @offset: address to translate > - * > - * Some chip functions require allocations from stolen space and need the > - * physical address of the memory in question. > - */ > -static unsigned long i915_stolen_to_phys(struct drm_device *dev, u32 offset) > +static unsigned long i915_stolen_to_physical(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct pci_dev *pdev = dev_priv->bridge_dev; > u32 base; > > -#if 0 > /* On the machines I have tested the Graphics Base of Stolen Memory > - * is unreliable, so compute the base by subtracting the stolen memory > - * from the Top of Low Usable DRAM which is where the BIOS places > - * the graphics stolen memory. > + * is unreliable, so on those compute the base by subtracting the > + * stolen memory from the Top of Low Usable DRAM which is where the > + * BIOS places the graphics stolen memory. > + * > + * On gen2, the layout is slightly different with the Graphics Segment > + * immediately following Top of Memory (or Top of Usable DRAM). Note > + * it appears that TOUD is only reported by 865g, so we just use the > + * top of memory as determined by the e820 probe. > + * > + * XXX gen2 requires an unavailable symbol and 945gm fails with > + * its value of TOLUD. > */ > + base = 0; > if (INTEL_INFO(dev)->gen > 3 || IS_G33(dev)) { > - /* top 32bits are reserved = 0 */ > + /* Read Graphics Base of Stolen Memory directly */ > pci_read_config_dword(pdev, 0xA4, &base); > - } else { > - /* XXX presume 8xx is the same as i915 */ > - pci_bus_read_config_dword(pdev->bus, 2, 0x5C, &base); > - } > -#else > - if (INTEL_INFO(dev)->gen > 3 || IS_G33(dev)) { > - u16 val; > - pci_read_config_word(pdev, 0xb0, &val); > - base = val >> 4 << 20; > - } else { > +#if 0 > + } else if (IS_GEN3(dev)) { > u8 val; > + /* Stolen is immediately below Top of Low Usable DRAM */ > pci_read_config_byte(pdev, 0x9c, &val); > base = val >> 3 << 27; > - } > - base -= dev_priv->mm.gtt->stolen_size; > + base -= dev_priv->mm.gtt->stolen_size; > + } else { > + /* Stolen is immediately above Top of Memory */ > + base = max_low_pfn_mapped << PAGE_SHIFT; > #endif > + } > > - return base + offset; > + return base; > } > Comments on this below. > static void i915_warn_stolen(struct drm_device *dev) > @@ -117,7 +104,7 @@ static void i915_setup_compression(struct drm_device *dev, int size) > if (!compressed_fb) > goto err; > > - cfb_base = i915_stolen_to_phys(dev, compressed_fb->start); > + cfb_base = dev_priv->mm.stolen_base + compressed_fb->start; > if (!cfb_base) > goto err_fb; > > @@ -130,7 +117,7 @@ static void i915_setup_compression(struct drm_device *dev, int size) > if (!compressed_llb) > goto err_fb; > > - ll_base = i915_stolen_to_phys(dev, compressed_llb->start); > + ll_base = dev_priv->mm.stolen_base + compressed_llb->start; > if (!ll_base) > goto err_llb; > } > @@ -149,7 +136,7 @@ static void i915_setup_compression(struct drm_device *dev, int size) > } > > DRM_DEBUG_KMS("FBC base 0x%08lx, ll base 0x%08lx, size %dM\n", > - cfb_base, ll_base, size >> 20); > + (long)cfb_base, (long)ll_base, size >> 20); > return; > > err_llb: > @@ -181,6 +168,10 @@ int i915_gem_init_stolen(struct drm_device *dev) > struct drm_i915_private *dev_priv = dev->dev_private; > unsigned long prealloc_size = dev_priv->mm.gtt->stolen_size; > > + dev_priv->mm.stolen_base = i915_stolen_to_physical(dev); > + if (dev_priv->mm.stolen_base == 0) > + return 0; > + > /* Basic memrange allocator for stolen space */ > drm_mm_init(&dev_priv->mm.stolen, 0, prealloc_size); > Since I found viewing the diff difficult for this patch, I am going to write the before and after (with the #if 0 blocks removed): Before: if (INTEL_INFO(dev)->gen > 3 || IS_G33(dev)) { u16 val; pci_read_config_word(pdev, 0xb0, &val); base = val >> 4 << 20; } else { u8 val; pci_read_config_byte(pdev, 0x9c, &val); base = val >> 3 << 27; } base -= dev_priv->mm.gtt->stolen_size; After: base = 0; if (INTEL_INFO(dev)->gen > 3 || IS_G33(dev)) { /* Read Graphics Base of Stolen Memory directly */ pci_read_config_dword(pdev, 0xA4, &base); I like that this is a simple helper now to calculate GSM Base. However, I am completely baffled by this patch. The read of 0xA4 did exist before, but was #if 0'd out appears to be the PCI capabilities pointer as of GEN6 (I won't check before that). The before code on the other hand at least looks correct for gen6 (BDSM - GGMS). This seems like it will definitely break SNB+, which would be bad for bisection. Has git done something funny here? -- Ben Widawsky, Intel Open Source Technology Center