Re: [PATCH 18/29] drm/i915: Delay allocation of stolen space for FBC

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Aug 11, 2012 at 03:41:17PM +0100, Chris Wilson 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>

Some comments inline below.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h        |    1 +
>  drivers/gpu/drm/i915/i915_gem_stolen.c |  114 +++++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_display.c   |    3 +
>  drivers/gpu/drm/i915/intel_pm.c        |   13 ++--
>  4 files changed, 70 insertions(+), 61 deletions(-)
> 

[snip]

> +int i915_gem_stolen_setup_compression(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_mm_node *entry;
> +	unsigned long 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;
> +	list_for_each_entry(entry, &dev_priv->mm.stolen.hole_stack, hole_stack) {
> +		unsigned long hole_start = entry->start + entry->size;
> +		unsigned long hole_end = list_entry(entry->node_list.next,
> +						    struct drm_mm_node,
> +						    node_list)->start;
> +		unsigned long hole_size = hole_end - hole_start;

This feels a bit to much munging around in drm_mm internals. What about a
drm_mm_for_each_hole(entry, mm, hole_start, hole_end)
#define helper?

> +		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);
>  }
>  
>  void intel_modeset_gem_init(struct drm_device *dev)

[snap]

> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3021c18..6f0f498 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -438,12 +438,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 > dev_priv->cfb_size) {
> -		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, "
> @@ -477,6 +471,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;
> +	}

I couldn't figure out why this block here had to move ... please enlighten
me.

> +
>  	/* 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)
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48