| Message ID | 1350666204-8101-6-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Ben Widawsky |
| Headers | show |
On Fri, 19 Oct 2012 18:03:12 +0100 Chris Wilson <chris@chris-wilson.co.uk> wrote: > As we may wish to wrap regions preallocated by the BIOS, we need to do > that before carving out contiguous chunks of stolen space for FBC. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> More nitckicks: Reviewed-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 + > drivers/gpu/drm/i915/i915_gem_stolen.c | 115 ++++++++++++++++---------------- > drivers/gpu/drm/i915/intel_display.c | 3 + > drivers/gpu/drm/i915/intel_pm.c | 3 +- > 4 files changed, 66 insertions(+), 57 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 589b2f3..997abb3 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1516,6 +1516,8 @@ int i915_gem_evict_everything(struct drm_device *dev); > > /* i915_gem_stolen.c */ > int i915_gem_init_stolen(struct drm_device *dev); > +int i915_gem_stolen_setup_compression(struct drm_device *dev); > +void i915_gem_stolen_cleanup_compression(struct drm_device *dev); > void i915_gem_cleanup_stolen(struct drm_device *dev); > > /* i915_gem_tiling.c */ > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > index d023ed6..68ef22a 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -89,21 +89,13 @@ static unsigned long i915_stolen_to_physical(struct drm_device *dev) > return base; > } > > -static void i915_warn_stolen(struct drm_device *dev) > -{ > - DRM_INFO("not enough stolen space for compressed buffer, disabling\n"); > - DRM_INFO("hint: you may be able to increase stolen memory size in the BIOS to avoid this\n"); > -} > - > -static void i915_setup_compression(struct drm_device *dev, int size) > +static int i915_setup_compression(struct drm_device *dev, int size) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_mm_node *compressed_fb, *uninitialized_var(compressed_llb); > - unsigned long cfb_base; > - unsigned long ll_base = 0; > > - /* Just in case the BIOS is doing something questionable. */ > - intel_disable_fbc(dev); > + DRM_DEBUG_KMS("reserving %d bytes of contiguous stolen space for FBC\n", > + size); > I see you've moved this to modeset init. I question whether we even need it at all. > compressed_fb = drm_mm_search_free(&dev_priv->mm.stolen, size, 4096, 0); > if (compressed_fb) > @@ -111,11 +103,11 @@ static void i915_setup_compression(struct drm_device *dev, int size) > if (!compressed_fb) > goto err; > > - cfb_base = dev_priv->mm.stolen_base + compressed_fb->start; > - if (!cfb_base) > - goto err_fb; > - > - if (!(IS_GM45(dev) || HAS_PCH_SPLIT(dev))) { > + if (HAS_PCH_SPLIT(dev)) > + I915_WRITE(ILK_DPFC_CB_BASE, compressed_fb->start); > + else if (IS_GM45(dev)) { > + I915_WRITE(DPFC_CB_BASE, compressed_fb->start); > + } else { > compressed_llb = drm_mm_search_free(&dev_priv->mm.stolen, > 4096, 4096, 0); > if (compressed_llb) > @@ -124,56 +116,81 @@ static void i915_setup_compression(struct drm_device *dev, int size) > if (!compressed_llb) > goto err_fb; > > - ll_base = dev_priv->mm.stolen_base + compressed_llb->start; > - if (!ll_base) > - goto err_llb; > - } > + dev_priv->compressed_llb = compressed_llb; > > - dev_priv->cfb_size = size; > + I915_WRITE(FBC_CFB_BASE, > + dev_priv->mm.stolen_base + compressed_fb->start); > + I915_WRITE(FBC_LL_BASE, > + dev_priv->mm.stolen_base + compressed_llb->start); > + } > > dev_priv->compressed_fb = compressed_fb; > - if (HAS_PCH_SPLIT(dev)) > - I915_WRITE(ILK_DPFC_CB_BASE, compressed_fb->start); > - else if (IS_GM45(dev)) { > - I915_WRITE(DPFC_CB_BASE, compressed_fb->start); > - } else { > - I915_WRITE(FBC_CFB_BASE, cfb_base); > - I915_WRITE(FBC_LL_BASE, ll_base); > - dev_priv->compressed_llb = compressed_llb; > - } > + dev_priv->cfb_size = size; > > - DRM_DEBUG_KMS("FBC base 0x%08lx, ll base 0x%08lx, size %dM\n", > - (long)cfb_base, (long)ll_base, size >> 20); > - return; > + return size; > > -err_llb: > - drm_mm_put_block(compressed_llb); > err_fb: > drm_mm_put_block(compressed_fb); > err: > dev_priv->no_fbc_reason = FBC_STOLEN_TOO_SMALL; > - i915_warn_stolen(dev); > + DRM_INFO("not enough stolen space for compressed buffer (need %d bytes), disabling\n", size); > + DRM_INFO("hint: you may be able to increase stolen memory size in the BIOS to avoid this\n"); > + return 0; > +} > + > +int i915_gem_stolen_setup_compression(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_mm_node *node; > + unsigned long hole_start, hole_end, size; > + > + if (dev_priv->mm.stolen_base == 0) > + return 0; > + > + if (dev_priv->cfb_size) > + return dev_priv->cfb_size; > + > + /* Try to set up FBC with a reasonable compressed buffer size */ > + size = 0; > + drm_mm_for_each_hole(node, &dev_priv->mm.stolen, hole_start, hole_end) { > + unsigned long hole_size = hole_end - hole_start; > + if (hole_size > size) > + size = hole_size; > + } > + > + /* Try to get a 32M buffer... */ > + if (size > (36*1024*1024)) > + size = 32*1024*1024; > + else /* fall back to 3/4 of the stolen space */ > + size = size * 3 / 4; > + > + return i915_setup_compression(dev, size); > } > It seems a bit silly that you traverse the hole list here only to traverse it again later with drm_mm_search_free. If you know you want 32MB, how about you try for that with search free, and then if that fails, go for the drm_mm_for_each_hole? afaict, this slightly differs from the old code in that it doesn't respect i915_powersave any longer. I have no issue with that, but if that's truly not an oversight on my part, something in the commit message might be nice. You've also changed from 3/4 to 7/8 for no apparent reason. Not great for bisecting. At least you updated the comment. > -static void i915_cleanup_compression(struct drm_device *dev) > +void i915_gem_stolen_cleanup_compression(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > - drm_mm_put_block(dev_priv->compressed_fb); > + if (dev_priv->cfb_size == 0) > + return; > + > + if (dev_priv->compressed_fb) > + drm_mm_put_block(dev_priv->compressed_fb); > + > if (dev_priv->compressed_llb) > drm_mm_put_block(dev_priv->compressed_llb); > + > + dev_priv->cfb_size = 0; > } > > void i915_gem_cleanup_stolen(struct drm_device *dev) > { > - if (I915_HAS_FBC(dev) && i915_powersave) > - i915_cleanup_compression(dev); > + i915_gem_stolen_cleanup_compression(dev); > } > I think we lost the meaning of i915_powersave again here, but I could be mistaken. > 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) > @@ -183,21 +200,7 @@ int i915_gem_init_stolen(struct drm_device *dev) > dev_priv->mm.gtt->stolen_size, dev_priv->mm.stolen_base); > > /* Basic memrange allocator for stolen space */ > - drm_mm_init(&dev_priv->mm.stolen, 0, prealloc_size); > - > - /* Try to set up FBC with a reasonable compressed buffer size */ > - if (I915_HAS_FBC(dev) && i915_powersave) { > - int cfb_size; > - > - /* Leave 1M for line length buffer & misc. */ > - > - /* Try to get a 32M buffer... */ > - if (prealloc_size > (36*1024*1024)) > - cfb_size = 32*1024*1024; > - else /* fall back to 7/8 of the stolen space */ > - cfb_size = prealloc_size * 7 / 8; > - i915_setup_compression(dev, cfb_size); > - } > + drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->mm.gtt->stolen_size); > > return 0; > } > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index c2c219b..2ef497d0 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8351,6 +8351,9 @@ void intel_modeset_init(struct drm_device *dev) > /* Just disable it once at startup */ > i915_disable_vga(dev); > intel_setup_outputs(dev); > + > + /* Just in case the BIOS is doing something questionable. */ > + intel_disable_fbc(dev); > } > > static void > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 2b3cddf..9ee53cb 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -440,7 +440,7 @@ void intel_update_fbc(struct drm_device *dev) > dev_priv->no_fbc_reason = FBC_MODULE_PARAM; > goto out_disable; > } > - if (intel_fb->obj->base.size > dev_priv->cfb_size) { > + if (intel_fb->obj->base.size > i915_gem_stolen_setup_compression(dev)) { > DRM_DEBUG_KMS("framebuffer too large, disabling " > "compression\n"); > dev_priv->no_fbc_reason = FBC_STOLEN_TOO_SMALL; > @@ -526,6 +526,7 @@ out_disable: > DRM_DEBUG_KMS("unsupported config, disabling FBC\n"); > intel_disable_fbc(dev); > } > + i915_gem_stolen_cleanup_compression(dev); > } > > static void i915_pineview_get_mem_freq(struct drm_device *dev)
On Mon, 5 Nov 2012 13:44:13 +0000, Ben Widawsky <ben@bwidawsk.net> wrote: > > -static void i915_setup_compression(struct drm_device *dev, int size) > > +static int i915_setup_compression(struct drm_device *dev, int size) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > struct drm_mm_node *compressed_fb, *uninitialized_var(compressed_llb); > > - unsigned long cfb_base; > > - unsigned long ll_base = 0; > > > > - /* Just in case the BIOS is doing something questionable. */ > > - intel_disable_fbc(dev); > > + DRM_DEBUG_KMS("reserving %d bytes of contiguous stolen space for FBC\n", > > + size); > > > > I see you've moved this to modeset init. I question whether we even need > it at all. I presume you mean the FBC disable? We should never need it, just like we should never need any of the other BIOS sanitizers. > > +int i915_gem_stolen_setup_compression(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + struct drm_mm_node *node; > > + unsigned long hole_start, hole_end, size; > > + > > + if (dev_priv->mm.stolen_base == 0) > > + return 0; > > + > > + if (dev_priv->cfb_size) > > + return dev_priv->cfb_size; > > + > > + /* Try to set up FBC with a reasonable compressed buffer size */ > > + size = 0; > > + drm_mm_for_each_hole(node, &dev_priv->mm.stolen, hole_start, hole_end) { > > + unsigned long hole_size = hole_end - hole_start; > > + if (hole_size > size) > > + size = hole_size; > > + } > > + > > + /* Try to get a 32M buffer... */ > > + if (size > (36*1024*1024)) > > + size = 32*1024*1024; > > + else /* fall back to 3/4 of the stolen space */ > > + size = size * 3 / 4; > > + > > + return i915_setup_compression(dev, size); > > } > > > > It seems a bit silly that you traverse the hole list here only to > traverse it again later with drm_mm_search_free. If you know you want > 32MB, how about you try for that with search free, and then if that > fails, go for the drm_mm_for_each_hole? It was to try to preserve the flavour of the original code. If we felt adventurous could we actively manage the stolen space (i.e. evict/defragment). > afaict, this slightly differs from the old code in that it doesn't > respect i915_powersave any longer. I have no issue with that, but if > that's truly not an oversight on my part, something in the commit > message might be nice. No, we should simply never try to allocate stolen space if i915_powersave=0 and so never need to setup the compression buffer now. > You've also changed from 3/4 to 7/8 for no apparent reason. Not great > for bisecting. At least you updated the comment. Ah, just because we are trying to use stolen space for other things than FBC. More or less irrevelant until we enable FBC and care about the various tradeoffs. > > void i915_gem_cleanup_stolen(struct drm_device *dev) > > { > > - if (I915_HAS_FBC(dev) && i915_powersave) > > - i915_cleanup_compression(dev); > > + i915_gem_stolen_cleanup_compression(dev); > > } > > > > I think we lost the meaning of i915_powersave again here, but I could > be mistaken. Having allocated it, we always need to free it. We should not be allocating if i915_powersave=0 anyway. -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 589b2f3..997abb3 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1516,6 +1516,8 @@ int i915_gem_evict_everything(struct drm_device *dev); /* i915_gem_stolen.c */ int i915_gem_init_stolen(struct drm_device *dev); +int i915_gem_stolen_setup_compression(struct drm_device *dev); +void i915_gem_stolen_cleanup_compression(struct drm_device *dev); void i915_gem_cleanup_stolen(struct drm_device *dev); /* i915_gem_tiling.c */ diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index d023ed6..68ef22a 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -89,21 +89,13 @@ static unsigned long i915_stolen_to_physical(struct drm_device *dev) return base; } -static void i915_warn_stolen(struct drm_device *dev) -{ - DRM_INFO("not enough stolen space for compressed buffer, disabling\n"); - DRM_INFO("hint: you may be able to increase stolen memory size in the BIOS to avoid this\n"); -} - -static void i915_setup_compression(struct drm_device *dev, int size) +static int i915_setup_compression(struct drm_device *dev, int size) { struct drm_i915_private *dev_priv = dev->dev_private; struct drm_mm_node *compressed_fb, *uninitialized_var(compressed_llb); - unsigned long cfb_base; - unsigned long ll_base = 0; - /* Just in case the BIOS is doing something questionable. */ - intel_disable_fbc(dev); + DRM_DEBUG_KMS("reserving %d bytes of contiguous stolen space for FBC\n", + size); compressed_fb = drm_mm_search_free(&dev_priv->mm.stolen, size, 4096, 0); if (compressed_fb) @@ -111,11 +103,11 @@ static void i915_setup_compression(struct drm_device *dev, int size) if (!compressed_fb) goto err; - cfb_base = dev_priv->mm.stolen_base + compressed_fb->start; - if (!cfb_base) - goto err_fb; - - if (!(IS_GM45(dev) || HAS_PCH_SPLIT(dev))) { + if (HAS_PCH_SPLIT(dev)) + I915_WRITE(ILK_DPFC_CB_BASE, compressed_fb->start); + else if (IS_GM45(dev)) { + I915_WRITE(DPFC_CB_BASE, compressed_fb->start); + } else { compressed_llb = drm_mm_search_free(&dev_priv->mm.stolen, 4096, 4096, 0); if (compressed_llb) @@ -124,56 +116,81 @@ static void i915_setup_compression(struct drm_device *dev, int size) if (!compressed_llb) goto err_fb; - ll_base = dev_priv->mm.stolen_base + compressed_llb->start; - if (!ll_base) - goto err_llb; - } + dev_priv->compressed_llb = compressed_llb; - dev_priv->cfb_size = size; + I915_WRITE(FBC_CFB_BASE, + dev_priv->mm.stolen_base + compressed_fb->start); + I915_WRITE(FBC_LL_BASE, + dev_priv->mm.stolen_base + compressed_llb->start); + } dev_priv->compressed_fb = compressed_fb; - if (HAS_PCH_SPLIT(dev)) - I915_WRITE(ILK_DPFC_CB_BASE, compressed_fb->start); - else if (IS_GM45(dev)) { - I915_WRITE(DPFC_CB_BASE, compressed_fb->start); - } else { - I915_WRITE(FBC_CFB_BASE, cfb_base); - I915_WRITE(FBC_LL_BASE, ll_base); - dev_priv->compressed_llb = compressed_llb; - } + dev_priv->cfb_size = size; - DRM_DEBUG_KMS("FBC base 0x%08lx, ll base 0x%08lx, size %dM\n", - (long)cfb_base, (long)ll_base, size >> 20); - return; + return size; -err_llb: - drm_mm_put_block(compressed_llb); err_fb: drm_mm_put_block(compressed_fb); err: dev_priv->no_fbc_reason = FBC_STOLEN_TOO_SMALL; - i915_warn_stolen(dev); + DRM_INFO("not enough stolen space for compressed buffer (need %d bytes), disabling\n", size); + DRM_INFO("hint: you may be able to increase stolen memory size in the BIOS to avoid this\n"); + return 0; +} + +int i915_gem_stolen_setup_compression(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_mm_node *node; + unsigned long hole_start, hole_end, size; + + if (dev_priv->mm.stolen_base == 0) + return 0; + + if (dev_priv->cfb_size) + return dev_priv->cfb_size; + + /* Try to set up FBC with a reasonable compressed buffer size */ + size = 0; + drm_mm_for_each_hole(node, &dev_priv->mm.stolen, hole_start, hole_end) { + unsigned long hole_size = hole_end - hole_start; + if (hole_size > size) + size = hole_size; + } + + /* Try to get a 32M buffer... */ + if (size > (36*1024*1024)) + size = 32*1024*1024; + else /* fall back to 3/4 of the stolen space */ + size = size * 3 / 4; + + return i915_setup_compression(dev, size); } -static void i915_cleanup_compression(struct drm_device *dev) +void i915_gem_stolen_cleanup_compression(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - drm_mm_put_block(dev_priv->compressed_fb); + if (dev_priv->cfb_size == 0) + return; + + if (dev_priv->compressed_fb) + drm_mm_put_block(dev_priv->compressed_fb); + if (dev_priv->compressed_llb) drm_mm_put_block(dev_priv->compressed_llb); + + dev_priv->cfb_size = 0; } void i915_gem_cleanup_stolen(struct drm_device *dev) { - if (I915_HAS_FBC(dev) && i915_powersave) - i915_cleanup_compression(dev); + i915_gem_stolen_cleanup_compression(dev); } 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) @@ -183,21 +200,7 @@ int i915_gem_init_stolen(struct drm_device *dev) dev_priv->mm.gtt->stolen_size, dev_priv->mm.stolen_base); /* Basic memrange allocator for stolen space */ - drm_mm_init(&dev_priv->mm.stolen, 0, prealloc_size); - - /* Try to set up FBC with a reasonable compressed buffer size */ - if (I915_HAS_FBC(dev) && i915_powersave) { - int cfb_size; - - /* Leave 1M for line length buffer & misc. */ - - /* Try to get a 32M buffer... */ - if (prealloc_size > (36*1024*1024)) - cfb_size = 32*1024*1024; - else /* fall back to 7/8 of the stolen space */ - cfb_size = prealloc_size * 7 / 8; - i915_setup_compression(dev, cfb_size); - } + drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->mm.gtt->stolen_size); return 0; } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index c2c219b..2ef497d0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8351,6 +8351,9 @@ void intel_modeset_init(struct drm_device *dev) /* Just disable it once at startup */ i915_disable_vga(dev); intel_setup_outputs(dev); + + /* Just in case the BIOS is doing something questionable. */ + intel_disable_fbc(dev); } static void diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 2b3cddf..9ee53cb 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -440,7 +440,7 @@ void intel_update_fbc(struct drm_device *dev) dev_priv->no_fbc_reason = FBC_MODULE_PARAM; goto out_disable; } - if (intel_fb->obj->base.size > dev_priv->cfb_size) { + if (intel_fb->obj->base.size > i915_gem_stolen_setup_compression(dev)) { DRM_DEBUG_KMS("framebuffer too large, disabling " "compression\n"); dev_priv->no_fbc_reason = FBC_STOLEN_TOO_SMALL; @@ -526,6 +526,7 @@ out_disable: DRM_DEBUG_KMS("unsupported config, disabling FBC\n"); intel_disable_fbc(dev); } + i915_gem_stolen_cleanup_compression(dev); } static void i915_pineview_get_mem_freq(struct drm_device *dev)