Re: [PATCH 07/18] drm/i915: Defer allocation of stolen memory for FBC until actual first use

[Ben Widawsky <ben@bwidawsk.net>]

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)



-- 
Ben Widawsky, Intel Open Source Technology Center