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

[Rodrigo Vivi]

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

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

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.

Thanks,
Rodrigo.


On Tue, Feb 3, 2015 at 11:40 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Tue, Feb 03, 2015 at 10:46:49AM -0800, Rodrigo Vivi wrote:
>> On Tue, Feb 3, 2015 at 8:21 AM, Matt Roper <matthew.d.roper at intel.com> 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).
>> >
>> >> 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?
>> >
>> >
>> > Matt
>> >
>> >>
>> >> This patch here should have 0 effect (presuming I'm reading code
>> >> correctly), so what kinf of bug exactly are you seeing?
>>
>> Yeah, after I sent the patch I was thinking about that: that it should
>> have 0 effect,
>> but the symptom that this apparently fixed here is that PSR wasn't
>> starting at all without
>> doing something like going to fbcon and come back to X.
>> Something similar what Sonika had told on that psr-skl thread where
>> she couldn't get psr working on login screen.
>> After this patch I didn't' have to change back and forth to fbcon to
>> make psr work.
>>
>> Maybe just a coincidence, but anyway I believe the way it is nowadays
>> it is wrong. I believe frontbuffer bits and calls should be done at
>> commit step, not under prepare.
>
> Hm, I think we've had this issue for a long time, even on older
> frontbuffer tracking code. I think it's time to throw lots of debug printk
> at the problem and figure out why psr is disabled in this case.
> -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