diff mbox

[2/7] drm/i915: Resolving the memory region conflict for Stolen area

Message ID 20140227164336.GB4561@nuc-i3427.alporthouse.com (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson Feb. 27, 2014, 4:43 p.m. UTC 
On Mon, Feb 24, 2014 at 09:22:31PM +0200, Jani Nikula wrote:
> I'm going to need a Reviewed-by and preferrably a Tested-by on this.

I really didn't like this patch because requesting a region other than
the one we use is morally equivalent to not requesting a region at all.
The approach I would prefer is to only use the requested resource as our
available stolen region, like in the attached.
-Chris

Comments

Jani Nikula Feb. 28, 2014, 2:06 p.m. UTC | #1 
On Thu, 27 Feb 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, Feb 24, 2014 at 09:22:31PM +0200, Jani Nikula wrote:
>> I'm going to need a Reviewed-by and preferrably a Tested-by on this.
>
> I really didn't like this patch because requesting a region other than
> the one we use is morally equivalent to not requesting a region at all.
> The approach I would prefer is to only use the requested resource as our
> available stolen region, like in the attached.

I would need the reviewed- and tested-bys on this one instead then.

I'm keeping Akash's patch in -fixes for now as I've already pushed it
there. Frankly, I'm inclined to queuing that one for 3.14 and fixing
this right for 3.15, as it's a broken BIOS we're talking about, but I
could be convinced otherwise. Particularly with the r-b and t-b.

BR,
Jani.


> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> From e5230c7bf7186699f1b83dcbff5a45a3267c9941 Mon Sep 17 00:00:00 2001
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Wed, 26 Feb 2014 18:01:42 +0000
> Subject: [PATCH] drm/i915: Trim stolen region for reserved pages and
>  conflicting BIOS setups
>
> On a few systems we have to reserve regions inside the stolen portion
> for use by the BIOS - we have to trim that out of our own allocation. In
> some cases, the BIOS will have reduced the reserved region in the e820
> map and so we have to adjust our own region request to suit. Either way,
> we need to only use the resource that we successfully reserve for
> ourselves - rather than claim one region and use another.
>
> v2: Fix resource request bounds to be inclusive. (Jani)
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h        |  2 +-
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 64 ++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_pm.c        |  9 +++--
>  3 files changed, 49 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 655a45c981fd..eb31c456eb35 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1099,7 +1099,7 @@ struct i915_gem_mm {
>  	struct list_head unbound_list;
>  
>  	/** Usable portion of the GTT for GEM */
> -	unsigned long stolen_base; /* limited to low memory (32-bit) */
> +	struct resource *stolen_region; /* limited to low memory (32-bit) */
>  
>  	/** PPGTT used for aliasing the PPGTT with the GTT */
>  	struct i915_hw_ppgtt *aliasing_ppgtt;
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index adc5c91f6946..984ada1b0084 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -45,10 +45,11 @@
>   * for is a boon.
>   */
>  
> -static unsigned long i915_stolen_to_physical(struct drm_device *dev)
> +static struct resource *i915_stolen_to_physical(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct resource *r;
> +	unsigned long start, end;
>  	u32 base;
>  
>  	/* Almost universally we can find the Graphics Base of Stolen Memory
> @@ -184,15 +185,38 @@ static unsigned long i915_stolen_to_physical(struct drm_device *dev)
>  	 * kernel. So if the region is already marked as busy, something
>  	 * is seriously wrong.
>  	 */
> -	r = devm_request_mem_region(dev->dev, base, dev_priv->gtt.stolen_size,
> +	start = base + PAGE_SIZE; /* leave the first page alone! */
> +
> +	end = base + dev_priv->gtt.stolen_size - 1;
> +	if (IS_VALLEYVIEW(dev))
> +		end -= 1024*1024; /* top 1M on VLV/BYT is reserved */
> +
> +	r = devm_request_mem_region(dev->dev, start, end-start,
>  				    "Graphics Stolen Memory");
>  	if (r == NULL) {
> -		DRM_ERROR("conflict detected with stolen region: [0x%08x - 0x%08x]\n",
> -			  base, base + (uint32_t)dev_priv->gtt.stolen_size);
> -		base = 0;
> +		/* Weird. BIOS has not reserved the whole region for us,
> +		 * try something smaller.
> +		 */
> +		do {
> +			start += PAGE_SIZE;
> +			end -= PAGE_SIZE;
> +			if (start < end)
> +				break;
> +
> +			r = devm_request_mem_region(dev->dev, start, end-start,
> +						    "Graphics Stolen Memory");
> +		} while (r == NULL);
> +
> +		if (r == NULL)
> +			DRM_ERROR("conflicting resource reservations detected with stolen region: [0x%08x - 0x%08x]\n",
> +				  base, base + (uint32_t)dev_priv->gtt.stolen_size - 1);
> +		else
> +			DRM_INFO("conflict detected with stolen region [0x%08x - 0x%08x], reducing to [0x%08lx - 0x%08lx]\n",
> +				 base, base + (uint32_t)dev_priv->gtt.stolen_size - 1,
> +				 start, end);
>  	}
>  
> -	return base;
> +	return r;
>  }
>  
>  static int i915_setup_compression(struct drm_device *dev, int size)
> @@ -232,9 +256,9 @@ static int i915_setup_compression(struct drm_device *dev, int size)
>  		dev_priv->fbc.compressed_llb = compressed_llb;
>  
>  		I915_WRITE(FBC_CFB_BASE,
> -			   dev_priv->mm.stolen_base + compressed_fb->start);
> +			   dev_priv->mm.stolen_region->start + compressed_fb->start);
>  		I915_WRITE(FBC_LL_BASE,
> -			   dev_priv->mm.stolen_base + compressed_llb->start);
> +			   dev_priv->mm.stolen_region->start + compressed_llb->start);
>  	}
>  
>  	dev_priv->fbc.compressed_fb = compressed_fb;
> @@ -304,28 +328,22 @@ void i915_gem_cleanup_stolen(struct drm_device *dev)
>  int i915_gem_init_stolen(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int bios_reserved = 0;
> +	struct resource *r;
>  
>  	if (dev_priv->gtt.stolen_size == 0)
>  		return 0;
>  
> -	dev_priv->mm.stolen_base = i915_stolen_to_physical(dev);
> -	if (dev_priv->mm.stolen_base == 0)
> +	r = i915_stolen_to_physical(dev);
> +	if (r == NULL)
>  		return 0;
>  
> -	DRM_DEBUG_KMS("found %zd bytes of stolen memory at %08lx\n",
> -		      dev_priv->gtt.stolen_size, dev_priv->mm.stolen_base);
> -
> -	if (IS_VALLEYVIEW(dev))
> -		bios_reserved = 1024*1024; /* top 1M on VLV/BYT */
> -
> -	if (WARN_ON(bios_reserved > dev_priv->gtt.stolen_size))
> -		return 0;
> +	DRM_DEBUG_KMS("found %ld bytes of stolen memory at %08lx\n",
> +		      (long)resource_size(r), (long)r->start);
>  
>  	/* Basic memrange allocator for stolen space */
> -	drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size -
> -		    bios_reserved);
> +	drm_mm_init(&dev_priv->mm.stolen, 0, resource_size(r));
>  
> +	dev_priv->mm.stolen_region = r;
>  	return 0;
>  }
>  
> @@ -338,7 +356,7 @@ i915_pages_create_for_stolen(struct drm_device *dev,
>  	struct scatterlist *sg;
>  
>  	DRM_DEBUG_DRIVER("offset=0x%x, size=%d\n", offset, size);
> -	BUG_ON(offset > dev_priv->gtt.stolen_size - size);
> +	BUG_ON(offset > resource_size(dev_priv->mm.stolen_region) - size);
>  
>  	/* We hide that we have no struct page backing our stolen object
>  	 * by wrapping the contiguous physical allocation with a fake
> @@ -358,7 +376,7 @@ i915_pages_create_for_stolen(struct drm_device *dev,
>  	sg->offset = 0;
>  	sg->length = size;
>  
> -	sg_dma_address(sg) = (dma_addr_t)dev_priv->mm.stolen_base + offset;
> +	sg_dma_address(sg) = (dma_addr_t)dev_priv->mm.stolen_region->start + offset;
>  	sg_dma_len(sg) = size;
>  
>  	return st;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3419ddae32c6..ac0cff40d5ec 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3564,12 +3564,17 @@ static void valleyview_setup_pctx(struct drm_device *dev)
>  	u32 pcbr;
>  	int pctx_size = 24*1024;
>  
> +	if (dev_priv->mm.stolen_region == NULL) {
> +		DRM_DEBUG("stolen space not reserved, unable to setup power saving context");
> +		return;
> +	}
> +
>  	pcbr = I915_READ(VLV_PCBR);
>  	if (pcbr) {
>  		/* BIOS set it up already, grab the pre-alloc'd space */
>  		int pcbr_offset;
>  
> -		pcbr_offset = (pcbr & (~4095)) - dev_priv->mm.stolen_base;
> +		pcbr_offset = (pcbr & (~4095)) - dev_priv->mm.stolen_region->start;
>  		pctx = i915_gem_object_create_stolen_for_preallocated(dev_priv->dev,
>  								      pcbr_offset,
>  								      I915_GTT_OFFSET_NONE,
> @@ -3591,7 +3596,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
>  		return;
>  	}
>  
> -	pctx_paddr = dev_priv->mm.stolen_base + pctx->stolen->start;
> +	pctx_paddr = dev_priv->mm.stolen_region->start + pctx->stolen->start;
>  	I915_WRITE(VLV_PCBR, pctx_paddr);
>  
>  out:
> -- 
> 1.9.0
>
Jani Nikula March 6, 2014, 6:21 a.m. UTC | #2 
On Fri, 28 Feb 2014, Jani Nikula <jani.nikula@intel.com> wrote:
> I'm keeping Akash's patch in -fixes for now as I've already pushed it
> there. Frankly, I'm inclined to queuing that one for 3.14 and fixing
> this right for 3.15, as it's a broken BIOS we're talking about, but I
> could be convinced otherwise. Particularly with the r-b and t-b.

Just following up on this: Akash's patch is now on its way to 3.14.

Chris, please rebase your patch on -nightly so we can get it ready for
3.15.

Thanks,
Jani.
diff mbox

Patch

From e5230c7bf7186699f1b83dcbff5a45a3267c9941 Mon Sep 17 00:00:00 2001
From: Chris Wilson <chris@chris-wilson.co.uk>
Date: Wed, 26 Feb 2014 18:01:42 +0000
Subject: [PATCH] drm/i915: Trim stolen region for reserved pages and
 conflicting BIOS setups

On a few systems we have to reserve regions inside the stolen portion
for use by the BIOS - we have to trim that out of our own allocation. In
some cases, the BIOS will have reduced the reserved region in the e820
map and so we have to adjust our own region request to suit. Either way,
we need to only use the resource that we successfully reserve for
ourselves - rather than claim one region and use another.

v2: Fix resource request bounds to be inclusive. (Jani)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h        |  2 +-
 drivers/gpu/drm/i915/i915_gem_stolen.c | 64 ++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_pm.c        |  9 +++--
 3 files changed, 49 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 655a45c981fd..eb31c456eb35 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1099,7 +1099,7 @@  struct i915_gem_mm {
 	struct list_head unbound_list;
 
 	/** Usable portion of the GTT for GEM */
-	unsigned long stolen_base; /* limited to low memory (32-bit) */
+	struct resource *stolen_region; /* limited to low memory (32-bit) */
 
 	/** PPGTT used for aliasing the PPGTT with the GTT */
 	struct i915_hw_ppgtt *aliasing_ppgtt;
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index adc5c91f6946..984ada1b0084 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -45,10 +45,11 @@ 
  * for is a boon.
  */
 
-static unsigned long i915_stolen_to_physical(struct drm_device *dev)
+static struct resource *i915_stolen_to_physical(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct resource *r;
+	unsigned long start, end;
 	u32 base;
 
 	/* Almost universally we can find the Graphics Base of Stolen Memory
@@ -184,15 +185,38 @@  static unsigned long i915_stolen_to_physical(struct drm_device *dev)
 	 * kernel. So if the region is already marked as busy, something
 	 * is seriously wrong.
 	 */
-	r = devm_request_mem_region(dev->dev, base, dev_priv->gtt.stolen_size,
+	start = base + PAGE_SIZE; /* leave the first page alone! */
+
+	end = base + dev_priv->gtt.stolen_size - 1;
+	if (IS_VALLEYVIEW(dev))
+		end -= 1024*1024; /* top 1M on VLV/BYT is reserved */
+
+	r = devm_request_mem_region(dev->dev, start, end-start,
 				    "Graphics Stolen Memory");
 	if (r == NULL) {
-		DRM_ERROR("conflict detected with stolen region: [0x%08x - 0x%08x]\n",
-			  base, base + (uint32_t)dev_priv->gtt.stolen_size);
-		base = 0;
+		/* Weird. BIOS has not reserved the whole region for us,
+		 * try something smaller.
+		 */
+		do {
+			start += PAGE_SIZE;
+			end -= PAGE_SIZE;
+			if (start < end)
+				break;
+
+			r = devm_request_mem_region(dev->dev, start, end-start,
+						    "Graphics Stolen Memory");
+		} while (r == NULL);
+
+		if (r == NULL)
+			DRM_ERROR("conflicting resource reservations detected with stolen region: [0x%08x - 0x%08x]\n",
+				  base, base + (uint32_t)dev_priv->gtt.stolen_size - 1);
+		else
+			DRM_INFO("conflict detected with stolen region [0x%08x - 0x%08x], reducing to [0x%08lx - 0x%08lx]\n",
+				 base, base + (uint32_t)dev_priv->gtt.stolen_size - 1,
+				 start, end);
 	}
 
-	return base;
+	return r;
 }
 
 static int i915_setup_compression(struct drm_device *dev, int size)
@@ -232,9 +256,9 @@  static int i915_setup_compression(struct drm_device *dev, int size)
 		dev_priv->fbc.compressed_llb = compressed_llb;
 
 		I915_WRITE(FBC_CFB_BASE,
-			   dev_priv->mm.stolen_base + compressed_fb->start);
+			   dev_priv->mm.stolen_region->start + compressed_fb->start);
 		I915_WRITE(FBC_LL_BASE,
-			   dev_priv->mm.stolen_base + compressed_llb->start);
+			   dev_priv->mm.stolen_region->start + compressed_llb->start);
 	}
 
 	dev_priv->fbc.compressed_fb = compressed_fb;
@@ -304,28 +328,22 @@  void i915_gem_cleanup_stolen(struct drm_device *dev)
 int i915_gem_init_stolen(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int bios_reserved = 0;
+	struct resource *r;
 
 	if (dev_priv->gtt.stolen_size == 0)
 		return 0;
 
-	dev_priv->mm.stolen_base = i915_stolen_to_physical(dev);
-	if (dev_priv->mm.stolen_base == 0)
+	r = i915_stolen_to_physical(dev);
+	if (r == NULL)
 		return 0;
 
-	DRM_DEBUG_KMS("found %zd bytes of stolen memory at %08lx\n",
-		      dev_priv->gtt.stolen_size, dev_priv->mm.stolen_base);
-
-	if (IS_VALLEYVIEW(dev))
-		bios_reserved = 1024*1024; /* top 1M on VLV/BYT */
-
-	if (WARN_ON(bios_reserved > dev_priv->gtt.stolen_size))
-		return 0;
+	DRM_DEBUG_KMS("found %ld bytes of stolen memory at %08lx\n",
+		      (long)resource_size(r), (long)r->start);
 
 	/* Basic memrange allocator for stolen space */
-	drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size -
-		    bios_reserved);
+	drm_mm_init(&dev_priv->mm.stolen, 0, resource_size(r));
 
+	dev_priv->mm.stolen_region = r;
 	return 0;
 }
 
@@ -338,7 +356,7 @@  i915_pages_create_for_stolen(struct drm_device *dev,
 	struct scatterlist *sg;
 
 	DRM_DEBUG_DRIVER("offset=0x%x, size=%d\n", offset, size);
-	BUG_ON(offset > dev_priv->gtt.stolen_size - size);
+	BUG_ON(offset > resource_size(dev_priv->mm.stolen_region) - size);
 
 	/* We hide that we have no struct page backing our stolen object
 	 * by wrapping the contiguous physical allocation with a fake
@@ -358,7 +376,7 @@  i915_pages_create_for_stolen(struct drm_device *dev,
 	sg->offset = 0;
 	sg->length = size;
 
-	sg_dma_address(sg) = (dma_addr_t)dev_priv->mm.stolen_base + offset;
+	sg_dma_address(sg) = (dma_addr_t)dev_priv->mm.stolen_region->start + offset;
 	sg_dma_len(sg) = size;
 
 	return st;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3419ddae32c6..ac0cff40d5ec 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3564,12 +3564,17 @@  static void valleyview_setup_pctx(struct drm_device *dev)
 	u32 pcbr;
 	int pctx_size = 24*1024;
 
+	if (dev_priv->mm.stolen_region == NULL) {
+		DRM_DEBUG("stolen space not reserved, unable to setup power saving context");
+		return;
+	}
+
 	pcbr = I915_READ(VLV_PCBR);
 	if (pcbr) {
 		/* BIOS set it up already, grab the pre-alloc'd space */
 		int pcbr_offset;
 
-		pcbr_offset = (pcbr & (~4095)) - dev_priv->mm.stolen_base;
+		pcbr_offset = (pcbr & (~4095)) - dev_priv->mm.stolen_region->start;
 		pctx = i915_gem_object_create_stolen_for_preallocated(dev_priv->dev,
 								      pcbr_offset,
 								      I915_GTT_OFFSET_NONE,
@@ -3591,7 +3596,7 @@  static void valleyview_setup_pctx(struct drm_device *dev)
 		return;
 	}
 
-	pctx_paddr = dev_priv->mm.stolen_base + pctx->stolen->start;
+	pctx_paddr = dev_priv->mm.stolen_region->start + pctx->stolen->start;
 	I915_WRITE(VLV_PCBR, pctx_paddr);
 
 out:
-- 
1.9.0