[PATCH 03/18] drm/i915: Fix location of stolen memory register for SandyBridge+

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 02 Nov 2012 08:54:39 +0000
Chris Wilson <chris at chris-wilson.co.uk> wrote:

> On Thu, 1 Nov 2012 17:08:20 -0700, Ben Widawsky <ben at bwidawsk.net> wrote:
> > On Sun, 28 Oct 2012 09:48:35 +0000
> > Chris Wilson <chris at chris-wilson.co.uk> wrote:
> > 
> > > On Fri, 26 Oct 2012 14:58:45 -0700, Ben Widawsky <ben at bwidawsk.net> wrote:
> > > > On Fri, 19 Oct 2012 18:03:09 +0100
> > > > Chris Wilson <chris at chris-wilson.co.uk> wrote:
> > > > 
> > > > > A few of the earlier registers where enlarged and so the Base Data of
> > > > > Stolen Memory Register (BDSM) was pushed to 0xb0.
> > > > > 
> > > > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > > > Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> > > > 
> > > > This patch seems irrelevant to me. I have a i915_stolen_to_phys which
> > > > already looks correct (git blame shows you last updated it in April).
> > > > 
> > > > Can you help unconfuse me?
> > > 
> > > As the patch suggests the current registers being used by stolen-to-phys
> > > are incorrect for SNB+.
> > > -Chris
> > > 
> > 
> > It looks like all you've done here is fix up the bug you left from
> > patch2.
> > 
> > -- 
> > Ben Widawsky, Intel Open Source Technology Center
> From: Chris Wilson <chris at chris-wilson.co.uk>
> Subject: Re: [PATCH 02/18] drm/i915: Fix detection of stolen base for gen2
> To: Ben Widawsky <ben at bwidawsk.net>
> Cc: intel-gfx at lists.freedesktop.org
> In-Reply-To: <20121101165102.3cc9773c at bwidawsk.net>
> References: <1350666204-8101-1-git-send-email-chris at chris-wilson.co.uk> <1350666204-8101-2-git-send-email-chris at chris-wilson.co.uk> <20121101165102.3cc9773c at bwidawsk.net>
> 
> On Thu, 1 Nov 2012 16:51:02 -0700, Ben Widawsky <ben at bwidawsk.net> wrote:
> > On Fri, 19 Oct 2012 18:03:08 +0100
> > Chris Wilson <chris at chris-wilson.co.uk> 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 <chris at chris-wilson.co.uk>
> > > ---
> > >  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.
> 
> The code as it stands is broken on SNB (it only has a semblance of
> working but the address it generates is random and points into active
> system memory), so it seems immaterial as to whether it then remained
> broken after this patch, with the benefit of then having individual
> patches to address the later generations. To allay your fears, this can
> be split into a preparatory patch to allow the routine to fail, and then
> fail until each is fixed.
> -Chris
> 

Please do allay my fears, and I can review it again. However in the
meantime, I disagree. The code seems correct to me as I said above (for
SNB, I haven't back-checked to previous generations). What I am looking
at now tells me that stolen memory base (which is located directly above
the GTT stolen memory) is defined by the 31:20 of offset 0xb0.
Similarly, we know the size of the stolen memory for the gtt ptes from
offset 0x50 (caculated previously), therefore the address certainly
isn't random. If there is a gap between the PTEs and the stolen
memory, then I agree it's somewhat problematic, but it still seems
better than what you changed it to.

Anyway, this is all in reference to patch 2 really since patch 3 fixes
the problem. 



-- 
Ben Widawsky, Intel Open Source Technology Center


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux