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

[Chris Wilson <chris@chris-wilson.co.uk>]

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

-- 
Chris Wilson, Intel Open Source Technology Centre