On Thu, 27 Feb 2014, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxx>
> ---
> 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, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
