On Fri, 19 Oct 2012 18:03:13 +0100 Chris Wilson <chris at 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 at 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) -- Ben Widawsky, Intel Open Source Technology Center