[Intel-gfx] [PATCH 5/8] drm/i915: simplify FBC start/stop at invalidate/flush

[Chris Wilson]

On Tue, Jun 30, 2015 at 10:53:09AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
> 
> The problem with calling intel_fbc_update() at flush is that it fully
> rechecks and recomputes the FBC state, and that includes reallocating
> the CFB, which requires a struct_mutex lock that we don't always have.

Actually, that is something we should fix with a stolen_mutex (will be
needed elsewhere).
 
> The lack of struct_mutex lock can be considered a regression from:
> 
> commit dbef0f15b5c83231dacb214dbf9a6dba063ca21c
> Author: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Date:   Fri Feb 13 17:23:46 2015 -0200
>     drm/i915: add frontbuffer tracking to FBC
> 
> So introduce intel_fbc_stop() that doesn't unset fbc.crtc, then call
> stop/enable at invalidate/flush.
> 
> Notice that invalidate/flush is only called by the frontbuffer
> tracking infrastrucutre, so it's safe to not do a full
> intel_fbc_update() here.

I think intel_fbc_update() is confusing. I can understand start/stop,
but update to me is more akin to a flush. If it was called flush, then I
would not expect it to enable or disable fbc or do any reallocations at
all.

static void intel_fbc_flush(struct drm_device *dev)
{
	if (!dev_priv->fbc.busy_bits && dev_priv->fbc.crtc) {
		if (dev_priv->fbc.enabled)
			intel_fbc_stop(dev);
		intel_fbc_enable(&dev_priv->fbc.crtc->base);
	}
}

And now I look at enable/disable, start/stop and am confused as to what
all the stages actually are.

I presume that start/stop are the highest, and control the sw state. And
that enable/disable are just hw interaction. And who sets fbc.enabled?
start()? enable()? disable()? stop()?

In confusion,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre