[Intel-gfx] [PATCH 2/2] drm/i915: Remove delayed FBC activation.

[Maarten Lankhorst]

Op 27-06-18 om 16:14 schreef Ville Syrjälä:
> On Wed, Jun 27, 2018 at 01:45:04PM +0200, Maarten Lankhorst wrote:
>> Op 26-06-18 om 19:59 schreef Ville Syrjälä:
>>> On Mon, Jun 25, 2018 at 06:37:58PM +0200, Maarten Lankhorst wrote:
>>>> The only time we should start FBC is when we have waited a vblank
>>>> after the atomic update.
>>> What about front buffer tracking? Is this going to leave FBC permanently
>>> disabled unless there are flips/plane updates?
>> No, see intel_fbc_flush. If there's a race with frontbuffer tracking and page flip,
>> we will not enable FBC in intel_fbc_flush(), but in that case we would enable it from
>> intel_fbc_post_update().
> I guess what I haven't quite figured out is why the pre_plane_update()
> even leaves fbc logically enabled. I would think we should just disable
> fbc there if anything important is going to change, and then re-enable
> at post_plane_update() after reallocating the cfb.
Indeed.
>> Or the other way around, intel_fbc_post_update won't enable FBC if the fb is dirty, but
>> intel_fbc_flush() afterwards will.
>>> I think there are a few cases we need to consider:
>>> 1. plane update which doesn't need fbc disable
>>> 2. plane update which needs to disable fbc but can re-enable it after the flip
>>>    has happended (eg. need to reallocate the cfb due to plane size/format change)
>>> 3. plane update which needs to disable fbc and can't re-enable it (eg. the new
>>>    plane state no longer fbc compatible)
>>> 4. front buffer invalidate + flush
>>>
>>> HW nuke will handle case 1. Case 2 could do the fbc re-enable after the
>>> post-flip vblank wait. Case 3 would ideally let us move FBC to another
>>> plane (thinking of pre-HSW hardware here). And case 4 must re-enable fbc
>>> after the flush has happened.
>> I don't see how this code will break any case. :)
> OK, so I guess you just remove the delayed activation at
> flush/post_plane_update. So now it'll enable it immediately.
>
> Hmm, and if we just get a flush without the invalidate then we're going
> to just use the nuke msg register anyway. So I suppose this shouldn't
> cause much additional fbc on<->off ping pong.
>
> Actually, are we now effecitively forcing a synchronous vblank wait
> in pre_plane_update for every frame via the hw_deactivate() polling
> for fbc to stop? AFAICS with the re-enable now being immediate we're
> going to have to disable fbc every single time. I think the correct
> fix for that would involve a) not disabling fbc around flips when the
> hw flip nuke will suffice, and b) convert the "wait for fbc to disable"
> thing into a post_plane_update time assert (and verify whether the hw
> is actually happy with disabling both fbc and the plane at the same
> time).
Could we not block in hw_deactivate then? But only <gen5 is affected, not sure how much we still care about i8xx fbc since it's disabled by default.

I think doing it in pre_plane_update is fine? FBC is only enabled on bdw or later. It won't stall there.

~Maarten