Re: [PATCH 2/2] drm/i915: Remove delayed FBC activation.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Jun 28, 2018 at 04:10:09PM +0200, Maarten Lankhorst wrote:
> 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.

Hmm. I thought everyone waits there. Apparently I'm mistaken. Never mind
then. We should probably remove the poll from the fbc1 code too then,
and convert it to a post_plane_update assert. But we can defer to that
to when we fix the other remaining issues in that code.

Still a bit of redundant work to deactive+re-activate, but whatever.

OK, I think this series should be fine as is:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx