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

[Daniel Vetter]

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