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

[Paulo Zanoni]

2015-06-30 11:34 GMT-03:00 Chris Wilson <chris at chris-wilson.co.uk>:
> 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,

I understand your concerns and I agree with you that this is
confusing. I also agree that the addition of stop() makes things even
worse. One of the problems is that intel_fbc_update() does
"everything": it picks the CRTC, it can enable FBC, it can disable
FBC, it can change the CRTC, etc. So we have: update(), enable(),
disable(), flush() and invalidate(), and the patch added stop().

I had some patches that would move us to enable/disable (high level)
activate/deactivate (low level), flush/invalidate (wrappers for
activate/deactivate) and update (highest level). This would make the
naming scheme similar to PSR. I wanted to merge the locking fixes
first, but I can put everything on the same series if you want. Or
leave this patch out of the "locking" series and add it to the next
series...

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni