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

[Rodrigo Vivi]

On Mon, Feb 23, 2015 at 6:13 PM, Matt Roper <matthew.d.roper at intel.com> wrote:
> 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.

Ah so yes, this is the bug, when moving a cursor we have many commits
without checks. So it is only one check followed by many commits... so
it commits with intel_crtc->atomic all zeroed by a previous
finish_commit.

>
> 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.

I lost those logs, but if you put one print in check and one in commit
you also should be able to see that since I heard about this false
positive from many people.

>
> 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.

>From what I heard and what I saw on the logs I thought it was ok to
have 1 check than as many commits as possible without the check.
If that was the case we would have a fail that is to zero the
structure on finish_commit. But seems this isn't the case. Sorry.

> So if you had a check that never got a corresponding
> commit, there might still be garbage left over in there.

No it is the opposite. Commits with no check. causing commit of
intel_crtc->atomic zeroed.

> 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.

Thanks! I thought about this but got afraid that clearing this on top
of check could cause a race since one plane doing a check could clean
the atomic being commited on cursor movement, unless we hat that for
planes, but then we would have to iterate over all planes on
finish_commit.

We can also put this patch that fix the only known issue so far with a
FIXME comment while we don't have the final fix.

>
>
> 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



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br