[Intel-gfx] [PATCH] drm/i915: Fix frontbuffer false positve.

[Daniel Vetter]

On Tue, Feb 03, 2015 at 08:21:33AM -0800, Matt Roper wrote:
> On Tue, Feb 03, 2015 at 12:57:31PM +0100, Daniel Vetter wrote:
> > On Mon, Feb 02, 2015 at 03:38:16PM -0800, Rodrigo Vivi wrote:
> > > frontbuffer bits must be updated during commit times not on atomica prepare
> > > one, otherwise we have a risk of false positive.
> > >
> > > Cc Daniel Vetter <daniel.vetter at ffwll.ch>
> > > Cc: Sonika Jindal <sonika.jindal at intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > 
> > atomic.fb_bits isn't used at all right now, instead the
> > begin_crtc_commit function recomputes them. That looks wrong. 
> 
> We build up the collection of bits in atomic.fb_bits while going through
> the atomic pipeline for each plane, then do a single call to
> 
>         intel_frontbuffer_flip(dev, intel_crtc->atomic.fb_bits);
> 
> in intel_finish_crtc_commit to flush them all out together (and if
> check/prepare fail, we never actually get to that flush).

Hm right, no idea why I didn't spot this. And the code in
begin_crtc_commit is needed too.

> > Also for async commits we need
> > to do the proper 2-stage flip stuff that current page_flip code does using
> > frontbuffer_flip_prepare/complete.
> 
> I need to look at the frontbuffer stuff again...maybe we don't need
> atomic.fb_bits at all and should just use intel_frontbuffer_flip_prepare
> in the places we set the bits now and intel_frontbuffer_flip_complete
> where we're calling the flip mentioned above?

We need the same mask for both prepare and complete, so precomputing it
doesn't seem like a stupid idea.

One thing that does look a bit fishy though is the handling of
i915_gem_track_fb. Doing that in prepare_planes isn't correct since if a
later plane fails the prepare step we don't undo this.

And for simplicity it might be better to consolidate this all into the
loop in begin_crtc_commit, and maybe also push out the call to grab the
mutex - in most cases buffers will changes, so optimizing for moving
cursors doesn't seem to be a good idea.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch