All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ben Widawsky <ben@bwidawsk.net>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 02/18] drm/i915: Fix detection of stolen base for gen2
Date: Thu, 1 Nov 2012 16:51:02 -0700	[thread overview]
Message-ID: <20121101165102.3cc9773c@bwidawsk.net> (raw)
In-Reply-To: <1350666204-8101-2-git-send-email-chris@chris-wilson.co.uk>

On Fri, 19 Oct 2012 18:03:08 +0100
Chris Wilson <chris@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@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.

Has git done something funny here?

-- 
Ben Widawsky, Intel Open Source Technology Center

  reply	other threads:[~2012-11-01 23:51 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-10-19 17:03 [PATCH 01/18] drm: Introduce drm_mm_create_block() Chris Wilson
2012-10-19 17:03 ` [PATCH 02/18] drm/i915: Fix detection of stolen base for gen2 Chris Wilson
2012-11-01 23:51   ` Ben Widawsky [this message]
2012-10-19 17:03 ` [PATCH 03/18] drm/i915: Fix location of stolen memory register for SandyBridge+ Chris Wilson
2012-10-26 21:58   ` Ben Widawsky
2012-10-28  9:48     ` Chris Wilson
2012-10-28 17:52       ` Ben Widawsky
2012-11-02  0:08       ` Ben Widawsky
2012-11-02  8:54         ` Chris Wilson
2012-11-05 13:53           ` Ben Widawsky
2012-10-19 17:03 ` [PATCH 04/18] drm/i915: Avoid clearing preallocated regions from the GTT Chris Wilson
2012-10-26 22:22   ` Ben Widawsky
2012-10-19 17:03 ` [PATCH 05/18] drm: Introduce an iterator over holes in the drm_mm range manager Chris Wilson
2012-11-05 13:41   ` Ben Widawsky
2012-11-05 15:13     ` Chris Wilson
2012-10-19 17:03 ` [PATCH 06/18] drm/i915: Delay allocation of stolen space for FBC Chris Wilson
2012-11-05 13:44   ` Ben Widawsky
2012-11-05 15:24     ` Chris Wilson
2012-10-19 17:03 ` [PATCH 07/18] drm/i915: Defer allocation of stolen memory for FBC until actual first use Chris Wilson
2012-11-05 15:00   ` Ben Widawsky
2012-11-05 15:28     ` Chris Wilson
2012-11-05 15:32       ` Ben Widawsky
2012-10-19 17:03 ` [PATCH 08/18] drm/i915: Allow objects to be created with no backing pages, but stolen space Chris Wilson
2012-10-19 17:03 ` [PATCH 09/18] drm/i915: Differentiate between prime and stolen objects Chris Wilson
2012-10-19 17:03 ` [PATCH 10/18] drm/i915: Support readback of stolen objects upon error Chris Wilson
2012-11-05 15:41   ` Ben Widawsky
2012-10-19 17:03 ` [PATCH 11/18] drm/i915: Handle stolen objects in pwrite Chris Wilson
2012-10-19 17:03 ` [PATCH 12/18] drm/i915: Handle stolen objects for pread Chris Wilson
2012-10-19 17:03 ` [PATCH 13/18] drm/i915: Introduce i915_gem_object_create_stolen() Chris Wilson
2012-11-05 16:32   ` Ben Widawsky
2012-11-05 16:59     ` Chris Wilson
2012-11-05 17:34       ` Ben Widawsky
2012-10-19 17:03 ` [PATCH 14/18] drm/i915: Allocate fbcon from stolen memory Chris Wilson
2012-10-19 17:03 ` [PATCH 15/18] drm/i915: Allocate ringbuffers " Chris Wilson
2012-10-19 17:03 ` [PATCH 16/18] drm/i915: Allocate overlay registers " Chris Wilson
2012-11-05 17:39   ` Ben Widawsky
2012-10-19 17:03 ` [PATCH 17/18] drm/i915: Use a slab for object allocation Chris Wilson
2012-10-24 20:21   ` Paulo Zanoni
2012-11-05 17:49   ` Ben Widawsky
2012-11-05 20:57     ` Chris Wilson
2012-11-07 13:59       ` Ben Widawsky
2012-11-05 18:07   ` Ben Widawsky
2012-11-05 18:10     ` Ben Widawsky
2012-10-19 17:03 ` [PATCH 18/18] drm/i915: Introduce mapping of user pages into video memory (userptr) ioctl Chris Wilson
2012-10-22 22:21   ` Jesse Barnes
2012-10-23  7:20     ` Daniel Vetter
2012-10-23 17:50       ` Eric Anholt
2012-10-26 21:47 ` [PATCH 01/18] drm: Introduce drm_mm_create_block() Ben Widawsky
2012-10-28  9:57   ` Chris Wilson
2012-10-28 18:12     ` Ben Widawsky
2012-10-28 18:14       ` Ben Widawsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20121101165102.3cc9773c@bwidawsk.net \
    --to=ben@bwidawsk.net \
    --cc=chris@chris-wilson.co.uk \
    --cc=intel-gfx@lists.freedesktop.org \
    --subject='Re: [PATCH 02/18] drm/i915: Fix detection of stolen base for gen2' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.