On Tue, Jun 30, 2015 at 10:53:09AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
