[Intel-gfx] [PATCH 2/7] drm/i915: move FBC code out of i915_gem_stolen.c

[Chris Wilson]

On Thu, Jul 02, 2015 at 10:39:05AM -0300, Paulo Zanoni wrote:
> 2015-07-01 17:44 GMT-03:00 Chris Wilson <chris at chris-wilson.co.uk>:
> > On Wed, Jul 01, 2015 at 05:15:21PM -0300, Paulo Zanoni wrote:
> >
> > Looks much cleaner with the split.
> >
> >> +void intel_fbc_cleanup_cfb(struct drm_device *dev)
> >> +{
> >> +     struct drm_i915_private *dev_priv = dev->dev_private;
> >> +
> >> +     if (dev_priv->fbc.uncompressed_size == 0)
> >> +             return;
> >> +
> >> +     i915_gem_stolen_remove_node(&dev_priv->fbc.compressed_fb);
> >> +
> >> +     if (dev_priv->fbc.compressed_llb) {
> >> +             i915_gem_stolen_remove_node(dev_priv->fbc.compressed_llb);
> >> +             kfree(dev_priv->fbc.compressed_llb);
> >> +     }
> >
> > Any reason why one node is embedded and the other allocated? Just feels
> > a little inconsistent, so lacks an explanation. Just that one is always
> > used, and the other on rare gen would probably suffice.
> 
> I really didn't stop to pay attention to the ancient FBC pieces. IMHO
> reasoning/explanation/change about this should be on a separate patch,
> since this one is just moving the code around.
> 
> I only think about the gen2-4 FBC code when I remember it has the
> "disable FBC when more than one pipe is visible" restriction which I
> can't even find on the documentation I have. I wish we could either
> remove it or just remove the whole gen2-4 FBC support (will we ever
> have the courage to enable it by default?).

Oh, no worries, that is how the code was is no problem. It just stood
out as inconsistent and hence made me stop and think when reviewing.

Might as well make them the same whilst we are here so I don't ask
stupid questions.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre