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@xxxxxxxxxxxxxxx> -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx