| Message ID | 1350666204-8101-7-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:13 +0100 Chris Wilson <chris@chris-wilson.co.uk> wrote: > As FBC is commonly disabled due to limitations of the chipset upon > output configurations, on many systems FBC is never enabled. For those > systems, it is advantageous to make use of the stolen memory for other > objects and so we defer allocation of the FBC chunk until we actually > require it. This increases the likelihood of that allocation failing, > which in turns means that we are already taking advantage of the stolen > memory! I'm failing to see how this patch is doing what it advertises to do. At least applies to dinq it's only deferring the error check. None of the steps that now happen before allocating the stolen compressed fb use stolen memory. On any of the errors, we seem to free the stolen memory. I see a mode check, a platform/plane check, a tiling check, a debug check now happening before we setup compression, but I fail to see how that really effects anything.... I'm sorry if I am being obtuse, but could you please explain a bit better? > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_pm.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 9ee53cb..cbf3f38 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -440,12 +440,6 @@ 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 > i915_gem_stolen_setup_compression(dev)) { > - DRM_DEBUG_KMS("framebuffer too large, disabling " > - "compression\n"); > - dev_priv->no_fbc_reason = FBC_STOLEN_TOO_SMALL; > - goto out_disable; > - } > if ((crtc->mode.flags & DRM_MODE_FLAG_INTERLACE) || > (crtc->mode.flags & DRM_MODE_FLAG_DBLSCAN)) { > DRM_DEBUG_KMS("mode incompatible with compression, " > @@ -479,6 +473,13 @@ void intel_update_fbc(struct drm_device *dev) > if (in_dbg_master()) > goto out_disable; > > + 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; > + goto out_disable; > + } > + While we're moving this... I'd like to see the DRM_DEBUG statement on one line (I think almost everyone agrees these days that breaking the 80 character limit is acceptable in favor of string grepability). Also you may as well make intel_fb->obj just obj. > /* If the scanout has not changed, don't modify the FBC settings. > * Note that we make the fundamental assumption that the fb->obj > * cannot be unpinned (and have its GTT offset and fence revoked)
On Mon, 5 Nov 2012 15:00:36 +0000, Ben Widawsky <ben@bwidawsk.net> wrote: > On Fri, 19 Oct 2012 18:03:13 +0100 > Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > As FBC is commonly disabled due to limitations of the chipset upon > > output configurations, on many systems FBC is never enabled. For those > > systems, it is advantageous to make use of the stolen memory for other > > objects and so we defer allocation of the FBC chunk until we actually > > require it. This increases the likelihood of that allocation failing, > > which in turns means that we are already taking advantage of the stolen > > memory! > > I'm failing to see how this patch is doing what it advertises to do. At > least applies to dinq it's only deferring the error check. None of the > steps that now happen before allocating the stolen compressed fb use > stolen memory. On any of the errors, we seem to free the stolen memory. > I see a mode check, a platform/plane check, a tiling check, a debug > check now happening before we setup compression, but I fail to see how > that really effects anything.... I'm sorry if I am being obtuse, but > could you please explain a bit better? All of those previous checks are more likely to be false - and previously we never tried to recover the stolen memory. I can go back to a single patch as this is now just an optimisation rather than preventing a permanent loss of memory. -Chris
On Mon, 05 Nov 2012 15:28:42 +0000 Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Mon, 5 Nov 2012 15:00:36 +0000, Ben Widawsky <ben@bwidawsk.net> wrote: > > On Fri, 19 Oct 2012 18:03:13 +0100 > > Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > > As FBC is commonly disabled due to limitations of the chipset upon > > > output configurations, on many systems FBC is never enabled. For those > > > systems, it is advantageous to make use of the stolen memory for other > > > objects and so we defer allocation of the FBC chunk until we actually > > > require it. This increases the likelihood of that allocation failing, > > > which in turns means that we are already taking advantage of the stolen > > > memory! > > > > I'm failing to see how this patch is doing what it advertises to do. At > > least applies to dinq it's only deferring the error check. None of the > > steps that now happen before allocating the stolen compressed fb use > > stolen memory. On any of the errors, we seem to free the stolen memory. > > I see a mode check, a platform/plane check, a tiling check, a debug > > check now happening before we setup compression, but I fail to see how > > that really effects anything.... I'm sorry if I am being obtuse, but > > could you please explain a bit better? > > All of those previous checks are more likely to be false - and > previously we never tried to recover the stolen memory. I can go back to > a single patch as this is now just an optimisation rather than > preventing a permanent loss of memory. > -Chris > On the basis that all the other checks are more likely to be false, it is: Reviewed-by: Ben Widawsky <ben@bwidawsk.net> if you want to keep it. Maybe update the commit message if you feel like it.
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 9ee53cb..cbf3f38 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -440,12 +440,6 @@ 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 > i915_gem_stolen_setup_compression(dev)) { - DRM_DEBUG_KMS("framebuffer too large, disabling " - "compression\n"); - dev_priv->no_fbc_reason = FBC_STOLEN_TOO_SMALL; - goto out_disable; - } if ((crtc->mode.flags & DRM_MODE_FLAG_INTERLACE) || (crtc->mode.flags & DRM_MODE_FLAG_DBLSCAN)) { DRM_DEBUG_KMS("mode incompatible with compression, " @@ -479,6 +473,13 @@ void intel_update_fbc(struct drm_device *dev) if (in_dbg_master()) goto out_disable; + 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; + goto out_disable; + } + /* If the scanout has not changed, don't modify the FBC settings. * Note that we make the fundamental assumption that the fb->obj * cannot be unpinned (and have its GTT offset and fence revoked)
As FBC is commonly disabled due to limitations of the chipset upon output configurations, on many systems FBC is never enabled. For those systems, it is advantageous to make use of the stolen memory for other objects and so we defer allocation of the FBC chunk until we actually require it. This increases the likelihood of that allocation failing, which in turns means that we are already taking advantage of the stolen memory! Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_pm.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)