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

[Rodrigo Vivi]

Hi Daniel,

It seems that one check with one good commit followed by many zeroed
intel_crtc->atomic commits is again in place.

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