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

[Matt Roper]

On Mon, Feb 23, 2015 at 05:52:24PM -0800, Rodrigo Vivi wrote:
> Hi Daniel,
> 
> It seems that one check with one good commit followed by many zeroed
> intel_crtc->atomic commits is again in place.

Can you elaborate on what you mean by this?  With atomic it's possible
to have a check with no commit after it (if the check or prepare fail,
or if it's a 'test only' operation), but if you're seeing commits
without corresponding checks, that sounds like a bug.

Can you provide a dmesg with drm.debug turned on so we can see what's
going on?  Or add some dump_stack()'s so that we can see the backtrace
that led us to this situation.

Actually, I wonder if the problem is actually the opposite of what you
say above.  Now that I look at this again, we only zero out
intel_crtc->atomic in intel_finish_crtc_commit which is part of the
commit path.  So if you had a check that never got a corresponding
commit, there might still be garbage left over in there.  Ultimately we
should be handling all of this with real intel_crtc_state handling which
we don't quite have yet.  Maybe in the meantime we need to be clearing
out intel_crtc->atomic at the beginning of a top-level atomic
transaction?  I'll send a patch that makes this change for you to try
shortly.


Matt

> 
> So I'm getting that annoying false positives on latest -nightly.
> 
> Shouldn't we just merge this patch while atomic modeset design doesn't
> get fixed at all?
> 
> Unfortunately nothing comes to my mind than moving all
> intel_crtc->atomic set to commit time and let pre_commit just with
> pm_get...
> Ohter than that just a full redesign of atomic modeset.
> 
> Thanks,
> Rodrigo.
> 
> 
> On Fri, Feb 13, 2015 at 12:48 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> > On Thu, Feb 12, 2015 at 05:17:04PM -0800, Rodrigo Vivi wrote:
> >> No, we had solved old frontbuffer false positives... some missing
> >> flush somewhere at that time...
> >>
> >> So, I added a bunch of printk and I insist that it is conceptually
> >> wrong to set intel_crtc_atomic_commit on check times when you do
> >> memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic));
> >> on every finish_commit.
> >>
> >> With exception of atomic.disabled_planes I believe the rest shouldn't
> >> work in the way it is implemented because you can have one check
> >> followed by many commits, but after the first commit all atomic
> >> variables are zeroed, except the disabled_planes that is set outside
> >> check...
> >
> > Ok here's the trouble: Every commit should have at exactly one check for
> > the new state objects. Unfortunately in the transition that seems to have
> > been lost for some cases.
> >
> >> For instance: on every cursor movement atomic.fb_bits was 0x000 when
> >> it should be 0x002. This is why this patch solved the false positive,
> >> i.e. setting it on commit instead on check time we get it propperly
> >> set. One of the problems is the false positive but also it breaks
> >> entirely SW tracking on VLV/CHV....
> >>
> >> I believe wait_for flips, update_fbc, watermarks, etc should keep the
> >> value got on check for the commit or the check should be done at
> >> commit plane instead of on check.
> >>
> >> I started doing a patch here to move all atomic sets from check to
> >> commit functions but gave up on middle when I noticed the
> >> prepare_commit would almost get empty...
> >
> > All state precomputation must be done in check, at commit time you have a
> > lot less information since the old state is somewhat gone. You can still
> > get at it, but as soon as we add an async flip queue that will get really
> > ugly. The current placement is imo the correct one. Instead we need to
> > figure out where we're doing a ->commit without properly calling ->check
> > beforehand.
> >
> >> Another idea was to make a atomic set per plane and just memset(0) on
> >> begin of every check... But this would require reliable access to the
> >> plane being updated on finish_commit... I believe loop on all planes
> >> would be messy and cause other issues...
> >>
> >> So, I'll be out returning only next wed. Please let me know if you
> >> have any suggestion of best changes to do that I can implement the
> >> changes.
> >
> > Since you've done this testing I've landed Matt's patches to switch legacy
> > plane entry points over to atomic. Which means cursor updates should now
> > be done properly using atomic, always. But even then the old transitional
> > plane helpers should have called the check functions ... So not sure where
> > exactly we're loosing that check call.
> >
> > Matt Roper might have more insights.
> >
> > Thanks, Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795