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

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

[Rodrigo Vivi]

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@ffwll.ch>
Cc: Sonika Jindal <sonika.jindal@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 73b5923..1e83124 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12176,9 +12176,6 @@ finish:
 	if (intel_crtc->active) {
 		if (intel_crtc->cursor_width != state->base.crtc_w)
 			intel_crtc->atomic.update_wm = true;
-
-		intel_crtc->atomic.fb_bits |=
-			INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
 	}
 
 	return ret;
@@ -12220,8 +12217,11 @@ update:
 	intel_crtc->cursor_width = state->base.crtc_w;
 	intel_crtc->cursor_height = state->base.crtc_h;
 
-	if (intel_crtc->active)
+	if (intel_crtc->active) {
 		intel_crtc_update_cursor(crtc, state->visible);
+		intel_crtc->atomic.fb_bits |=
+			INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe);
+	}
 }
 
 static const struct drm_plane_funcs intel_cursor_plane_funcs = {
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5699
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  353/353              353/353
ILK                                  353/353              353/353
SNB              +1                 400/422              401/422
IVB              +1-1              485/487              485/487
BYT                                  296/296              296/296
HSW              +1-1              507/508              507/508
BDW                                  401/402              401/402
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*SNB  igt_kms_flip_event_leak      NSPT(6, M35M22)      PASS(1, M35)
 IVB  igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance      DMESG_WARN(9, M34M21)PASS(10, M4M34)      PASS(1, M34)
 IVB  igt_gem_storedw_batches_loop_secure-dispatch      DMESG_WARN(1, M34)PASS(9, M34M4)      DMESG_WARN(1, M34)
*HSW  igt_gem_pwrite_pread_display-copy-performance      PASS(7, M40M20)      DMESG_WARN(1, M40)
 HSW  igt_gem_pwrite_pread_snooped-pwrite-blt-cpu_mmap-performance      DMESG_WARN(2, M40)PASS(23, M40M20)      PASS(1, M40)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[Daniel Vetter]

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@ffwll.ch>
> Cc: Sonika Jindal <sonika.jindal@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

atomic.fb_bits isn't used at all right now, instead the begin_crtc_commit
function recomputes them. That looks wrong. 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.

This patch here should have 0 effect (presuming I'm reading code
correctly), so what kinf of bug exactly are you seeing?

Adding Matt&Ander.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[Matt Roper]

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@ffwll.ch>
> > Cc: Sonika Jindal <sonika.jindal@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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?
> 
> Adding Matt&Ander.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[Rodrigo Vivi]

On Tue, Feb 3, 2015 at 8:21 AM, Matt Roper <matthew.d.roper@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@ffwll.ch>
>> > Cc: Sonika Jindal <sonika.jindal@intel.com>
>> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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.

>>
>> Adding Matt&Ander.
>> -Daniel
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[Matt Roper]

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@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@ffwll.ch>
> >> > Cc: Sonika Jindal <sonika.jindal@intel.com>
> >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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.

I may be misunderstanding what you're saying, but I think this is the
way things already work?  We plan out which bits we're going to update
in the 'check' step (and record those bits in atomic.fb_bits), but then
we only actually set them with intel_frontbuffer_flip() in commit if
everything looks good.  If we wind up rejecting the update because we
fail to check/prepare, those bits in atomic.fb_bits should just get
thrown away, unless I'm missing something.


Matt

> 
> >>
> >> Adding Matt&Ander.
> >> -Daniel
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >
> > --
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[Daniel Vetter]

On Tue, Feb 03, 2015 at 08:21:33AM -0800, Matt Roper 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@ffwll.ch>
> > > Cc: Sonika Jindal <sonika.jindal@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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).

Hm right, no idea why I didn't spot this. And the code in
begin_crtc_commit is needed too.

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

We need the same mask for both prepare and complete, so precomputing it
doesn't seem like a stupid idea.

One thing that does look a bit fishy though is the handling of
i915_gem_track_fb. Doing that in prepare_planes isn't correct since if a
later plane fails the prepare step we don't undo this.

And for simplicity it might be better to consolidate this all into the
loop in begin_crtc_commit, and maybe also push out the call to grab the
mutex - in most cases buffers will changes, so optimizing for moving
cursors doesn't seem to be a good idea.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[Daniel Vetter]

On Tue, Feb 03, 2015 at 11:14:10AM -0800, Matt Roper 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@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@ffwll.ch>
> > >> > Cc: Sonika Jindal <sonika.jindal@intel.com>
> > >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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.
> 
> I may be misunderstanding what you're saying, but I think this is the
> way things already work?  We plan out which bits we're going to update
> in the 'check' step (and record those bits in atomic.fb_bits), but then
> we only actually set them with intel_frontbuffer_flip() in commit if
> everything looks good.  If we wind up rejecting the update because we
> fail to check/prepare, those bits in atomic.fb_bits should just get
> thrown away, unless I'm missing something.

What we could do is store the fb_bits in intel_plane and then move all the
code to handle them into shared code, out of the cursor/primary/sprite
specific stuff. That helps with new hw, in case we get 2 sprite planes and
so can't use the drm type any more to figure out which one it is.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[Daniel Vetter]

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@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@ffwll.ch>
> >> > Cc: Sonika Jindal <sonika.jindal@intel.com>
> >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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@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@ffwll.ch>
>> >> > Cc: Sonika Jindal <sonika.jindal@intel.com>
>> >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Clear crtc atomic flags at beginning of transaction

[Matt Roper]

Once we have full atomic modeset, these kind of flags should be in a
real intel_crtc_state that's tracked properly.  In the meantime, make
sure we clear out any old flags at the beginning of a transaction so
that we don't wind up seeing leftover flags from old transactions that
were checked, but never went to the commit step.

Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
Rodrigo, does this solve the frontbuffer problems you're seeing?  I haven't had
time to test this myself, but I think this will fix at least one problem that
may or may not have been the source of your troubles.

 drivers/gpu/drm/i915/intel_atomic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 011b896..fe51e23 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -76,6 +76,8 @@ int intel_atomic_check(struct drm_device *dev,
 	state->allow_modeset = false;
 	for (i = 0; i < ncrtcs; i++) {
 		struct intel_crtc *crtc = to_intel_crtc(state->crtcs[i]);
+		if (crtc)
+			memset(&crtc->atomic, 0, sizeof(crtc->atomic));
 		if (crtc && crtc->pipe != nuclear_pipe)
 			not_nuclear = true;
 	}
-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[Rodrigo Vivi]

On Mon, Feb 23, 2015 at 6:13 PM, Matt Roper <matthew.d.roper@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Clear crtc atomic flags at beginning of transaction

[Rodrigo Vivi]

Hi Matt,

It probably doesn't fix because the issue is to have many commits
without a check, having many commits with intel_crtc->atomic zeroed
already.
imho cleaning it more wouldn't solve it... but anyway, let me try.

Thanks,
Rodrigo.

On Mon, Feb 23, 2015 at 6:14 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
> Once we have full atomic modeset, these kind of flags should be in a
> real intel_crtc_state that's tracked properly.  In the meantime, make
> sure we clear out any old flags at the beginning of a transaction so
> that we don't wind up seeing leftover flags from old transactions that
> were checked, but never went to the commit step.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> Rodrigo, does this solve the frontbuffer problems you're seeing?  I haven't had
> time to test this myself, but I think this will fix at least one problem that
> may or may not have been the source of your troubles.
>
>  drivers/gpu/drm/i915/intel_atomic.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 011b896..fe51e23 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -76,6 +76,8 @@ int intel_atomic_check(struct drm_device *dev,
>         state->allow_modeset = false;
>         for (i = 0; i < ncrtcs; i++) {
>                 struct intel_crtc *crtc = to_intel_crtc(state->crtcs[i]);
> +               if (crtc)
> +                       memset(&crtc->atomic, 0, sizeof(crtc->atomic));
>                 if (crtc && crtc->pipe != nuclear_pipe)
>                         not_nuclear = true;
>         }
> --
> 1.8.5.1
>



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Clear crtc atomic flags at beginning of transaction

[Rodrigo Vivi]

It doesn't fix.

Let's me grab some new logs...

On Tue, Feb 24, 2015 at 9:43 AM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> Hi Matt,
>
> It probably doesn't fix because the issue is to have many commits
> without a check, having many commits with intel_crtc->atomic zeroed
> already.
> imho cleaning it more wouldn't solve it... but anyway, let me try.
>
> Thanks,
> Rodrigo.
>
> On Mon, Feb 23, 2015 at 6:14 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
>> Once we have full atomic modeset, these kind of flags should be in a
>> real intel_crtc_state that's tracked properly.  In the meantime, make
>> sure we clear out any old flags at the beginning of a transaction so
>> that we don't wind up seeing leftover flags from old transactions that
>> were checked, but never went to the commit step.
>>
>> Cc: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> ---
>> Rodrigo, does this solve the frontbuffer problems you're seeing?  I haven't had
>> time to test this myself, but I think this will fix at least one problem that
>> may or may not have been the source of your troubles.
>>
>>  drivers/gpu/drm/i915/intel_atomic.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 011b896..fe51e23 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -76,6 +76,8 @@ int intel_atomic_check(struct drm_device *dev,
>>         state->allow_modeset = false;
>>         for (i = 0; i < ncrtcs; i++) {
>>                 struct intel_crtc *crtc = to_intel_crtc(state->crtcs[i]);
>> +               if (crtc)
>> +                       memset(&crtc->atomic, 0, sizeof(crtc->atomic));
>>                 if (crtc && crtc->pipe != nuclear_pipe)
>>                         not_nuclear = true;
>>         }
>> --
>> 1.8.5.1
>>
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[Matt Roper]

On Tue, Feb 24, 2015 at 09:32:25AM -0800, Rodrigo Vivi wrote:
> On Mon, Feb 23, 2015 at 6:13 PM, Matt Roper <matthew.d.roper@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.

Hmm.  Are all of these people using a specific platform?  I only have
access to IVB hardware at the moment, so it could be that the problem
lies on codepaths I just don't exercise on my system.  Most of the
atomic stuff is pretty platform-agnostic, but maybe there's something I
overlooked.

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

I guess I should clarify terminology a little bit since this gets kind
of confusing...

For atomic, we have a top-level atomic_state structure that contains
everything that we want to update as part of an atomic transaction
(multiple planes, crtc's, etc.).  At the moment, we only support a
subset of atomic, so you'll only ever get one crtc being modified, but
there could be multiple planes (primary, cursor, 1 or more sprite
planes).  There's a sub-state structure (plane_state, crtc_state, etc.)
for each DRM object being updated.

At the top level of atomic handling we do "check" and then "commit" with
the top-level atomic state.  If you trace down through the codeflow, the
top-level check here will actually call a plane-specific check function
on each plane state during the top-level check, and then the top-level
commit function will call the plane-specific commit function on each
plane involved.

So when I say "1 or more checks, followed by exactly one commit" I'm
referring to the top-level check that deals with the top-level atomic
state.  If you're looking at the plane state specifically, you might see
more of an n:n relationship if your transaction is updating multiple
planes together (one check call per plane being updated, possibly
followed by one commit call for each of them), but the clearing of
intel_crtc->atomic should still happen at the end, after all of the
individual planes have been committed.

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

Well, based on what you say above, it sounds like it probably won't
solve your issue, so we'll need to explore some more.  I still haven't
managed to reproduce this myself, so I'm not sure whether I'm working on
a hardware platform that won't trigger the bug, or whether I'm just
unlucky.

Is there a specific igt test you've been using that triggers this
reliably for you?


Matt

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

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

Atomic bits needs to be set when cursor check function is returning 0
and intel_crtc is active.

v2: When putting more debug prints I notice the solution was simpler
than I thought. AMS design is solid, just this return was wrong.
Sorry for the noise.

Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8ccf033..5fb83ba 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12198,7 +12198,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
 	}
 
 	if (fb == crtc->cursor->fb)
-		return 0;
+		goto finish;
 
 	if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
 		DRM_DEBUG_KMS("cursor cannot be tiled\n");
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[Matt Roper]

On Tue, Feb 24, 2015 at 10:36:32AM -0800, Rodrigo Vivi wrote:
> Atomic bits needs to be set when cursor check function is returning 0
> and intel_crtc is active.
> 
> v2: When putting more debug prints I notice the solution was simpler
> than I thought. AMS design is solid, just this return was wrong.
> Sorry for the noise.
> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8ccf033..5fb83ba 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12198,7 +12198,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  	}
>  
>  	if (fb == crtc->cursor->fb)
> -		return 0;
> +		goto finish;
>  
>  	if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
>  		DRM_DEBUG_KMS("cursor cannot be tiled\n");
> -- 
> 2.1.0
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[Daniel Vetter]

On Tue, Feb 24, 2015 at 10:44:19AM -0800, Matt Roper wrote:
> On Tue, Feb 24, 2015 at 10:36:32AM -0800, Rodrigo Vivi wrote:
> > Atomic bits needs to be set when cursor check function is returning 0
> > and intel_crtc is active.
> > 
> > v2: When putting more debug prints I notice the solution was simpler
> > than I thought. AMS design is solid, just this return was wrong.
> > Sorry for the noise.
> > 
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 8ccf033..5fb83ba 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12198,7 +12198,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
> >  	}
> >  
> >  	if (fb == crtc->cursor->fb)
> > -		return 0;
> > +		goto finish;

Shouldn't we just delete this check entirely? gcc will do it anyway for
us. When you do that you can also append the history I've dug out for
this.

The original regression seems to have been introduced in the original
check/commit split:

commit 757f9a3e5b8a812af0c213099a5b31cb423f4d3c
Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Date:   Wed Sep 24 14:20:24 2014 -0300

    drm/i915: move check of intel_crtc_cursor_set_obj() out

Which already cause other trouble, resulting in the check getting moved in

commit e391ea882b1a04fb3f559287ac694652a3cd9da9
Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Date:   Wed Sep 24 14:20:25 2014 -0300

    drm/i915: Fix not checking cursor and object sizes

The frontbuffer tracking itself only was broken when we shifted it into
the check/commit logic with:

commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
Author: Matt Roper <matthew.d.roper@intel.com>
Date:   Wed Dec 24 07:59:06 2014 -0800

    drm/i915: Refactor work that can sleep out of commit (v7)

Cheers, Daniel
> >  
> >  	if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
> >  		DRM_DEBUG_KMS("cursor cannot be tiled\n");
> > -- 
> > 2.1.0
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[Matt Roper]

On Tue, Feb 24, 2015 at 09:38:25PM +0100, Daniel Vetter wrote:
> On Tue, Feb 24, 2015 at 10:44:19AM -0800, Matt Roper wrote:
> > On Tue, Feb 24, 2015 at 10:36:32AM -0800, Rodrigo Vivi wrote:
> > > Atomic bits needs to be set when cursor check function is returning 0
> > > and intel_crtc is active.
> > > 
> > > v2: When putting more debug prints I notice the solution was simpler
> > > than I thought. AMS design is solid, just this return was wrong.
> > > Sorry for the noise.
> > > 
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 8ccf033..5fb83ba 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -12198,7 +12198,7 @@ intel_check_cursor_plane(struct drm_plane *plane,
> > >  	}
> > >  
> > >  	if (fb == crtc->cursor->fb)
> > > -		return 0;
> > > +		goto finish;
> 
> Shouldn't we just delete this check entirely? gcc will do it anyway for
> us. When you do that you can also append the history I've dug out for
> this.

Yeah, that seems reasonable too.  The only thing we're actually skipping
at the moment is a tiling check which should never come true in this
case (otherwise we would have already failed on a previous cursor
update).


Matt

> 
> The original regression seems to have been introduced in the original
> check/commit split:
> 
> commit 757f9a3e5b8a812af0c213099a5b31cb423f4d3c
> Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Date:   Wed Sep 24 14:20:24 2014 -0300
> 
>     drm/i915: move check of intel_crtc_cursor_set_obj() out
> 
> Which already cause other trouble, resulting in the check getting moved in
> 
> commit e391ea882b1a04fb3f559287ac694652a3cd9da9
> Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Date:   Wed Sep 24 14:20:25 2014 -0300
> 
>     drm/i915: Fix not checking cursor and object sizes
> 
> The frontbuffer tracking itself only was broken when we shifted it into
> the check/commit logic with:
> 
> commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
> Author: Matt Roper <matthew.d.roper@intel.com>
> Date:   Wed Dec 24 07:59:06 2014 -0800
> 
>     drm/i915: Refactor work that can sleep out of commit (v7)
> 
> Cheers, Daniel
> > >  
> > >  	if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
> > >  		DRM_DEBUG_KMS("cursor cannot be tiled\n");
> > > -- 
> > > 2.1.0
> > > 
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

This return 0 without setting atomic bits on fb == crtc->cursor->fb
where causing frontbuffer false positives.

According to Daniel:

The original regression seems to have been introduced in the original
check/commit split:

commit 757f9a3e5b8a812af0c213099a5b31cb423f4d3c
Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Date:   Wed Sep 24 14:20:24 2014 -0300

    drm/i915: move check of intel_crtc_cursor_set_obj() out

Which already cause other trouble, resulting in the check getting moved in

commit e391ea882b1a04fb3f559287ac694652a3cd9da9
Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Date:   Wed Sep 24 14:20:25 2014 -0300

    drm/i915: Fix not checking cursor and object sizes

The frontbuffer tracking itself only was broken when we shifted it into
the check/commit logic with:

commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
Author: Matt Roper <matthew.d.roper@intel.com>
Date:   Wed Dec 24 07:59:06 2014 -0800

    drm/i915: Refactor work that can sleep out of commit (v7)

v2: When putting more debug prints I notice the solution was simpler
than I thought. AMS design is solid, just this return was wrong.
Sorry for the noise.

v3: Remove the entire chunck that would probably
    be removed by gcc anyway. (by Daniel)

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Cc: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8ccf033..900dcaa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12197,9 +12197,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
 		return -ENOMEM;
 	}
 
-	if (fb == crtc->cursor->fb)
-		return 0;
-
 	if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
 		DRM_DEBUG_KMS("cursor cannot be tiled\n");
 		ret = -EINVAL;
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[Matt Roper]

On Tue, Feb 24, 2015 at 01:37:54PM -0800, Rodrigo Vivi wrote:
> This return 0 without setting atomic bits on fb == crtc->cursor->fb
> where causing frontbuffer false positives.
> 
> According to Daniel:
> 
> The original regression seems to have been introduced in the original
> check/commit split:
> 
> commit 757f9a3e5b8a812af0c213099a5b31cb423f4d3c
> Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Date:   Wed Sep 24 14:20:24 2014 -0300
> 
>     drm/i915: move check of intel_crtc_cursor_set_obj() out
> 
> Which already cause other trouble, resulting in the check getting moved in
> 
> commit e391ea882b1a04fb3f559287ac694652a3cd9da9
> Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Date:   Wed Sep 24 14:20:25 2014 -0300
> 
>     drm/i915: Fix not checking cursor and object sizes
> 
> The frontbuffer tracking itself only was broken when we shifted it into
> the check/commit logic with:
> 
> commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
> Author: Matt Roper <matthew.d.roper@intel.com>
> Date:   Wed Dec 24 07:59:06 2014 -0800
> 
>     drm/i915: Refactor work that can sleep out of commit (v7)
> 
> v2: When putting more debug prints I notice the solution was simpler
> than I thought. AMS design is solid, just this return was wrong.
> Sorry for the noise.
> 
> v3: Remove the entire chunck that would probably
>     be removed by gcc anyway. (by Daniel)
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Not sure I understand Daniel's remark about gcc optimization, but the
change still looks good to me.

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8ccf033..900dcaa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12197,9 +12197,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
>  		return -ENOMEM;
>  	}
>  
> -	if (fb == crtc->cursor->fb)
> -		return 0;
> -
>  	if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
>  		DRM_DEBUG_KMS("cursor cannot be tiled\n");
>  		ret = -EINVAL;
> -- 
> 2.1.0
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[Daniel Vetter]

On Tue, Feb 24, 2015 at 01:42:39PM -0800, Matt Roper wrote:
> On Tue, Feb 24, 2015 at 01:37:54PM -0800, Rodrigo Vivi wrote:
> > This return 0 without setting atomic bits on fb == crtc->cursor->fb
> > where causing frontbuffer false positives.
> > 
> > According to Daniel:
> > 
> > The original regression seems to have been introduced in the original
> > check/commit split:
> > 
> > commit 757f9a3e5b8a812af0c213099a5b31cb423f4d3c
> > Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > Date:   Wed Sep 24 14:20:24 2014 -0300
> > 
> >     drm/i915: move check of intel_crtc_cursor_set_obj() out
> > 
> > Which already cause other trouble, resulting in the check getting moved in
> > 
> > commit e391ea882b1a04fb3f559287ac694652a3cd9da9
> > Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > Date:   Wed Sep 24 14:20:25 2014 -0300
> > 
> >     drm/i915: Fix not checking cursor and object sizes
> > 
> > The frontbuffer tracking itself only was broken when we shifted it into
> > the check/commit logic with:
> > 
> > commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
> > Author: Matt Roper <matthew.d.roper@intel.com>
> > Date:   Wed Dec 24 07:59:06 2014 -0800
> > 
> >     drm/i915: Refactor work that can sleep out of commit (v7)
> > 
> > v2: When putting more debug prints I notice the solution was simpler
> > than I thought. AMS design is solid, just this return was wrong.
> > Sorry for the noise.
> > 
> > v3: Remove the entire chunck that would probably
> >     be removed by gcc anyway. (by Daniel)
> > 
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Not sure I understand Daniel's remark about gcc optimization, but the
> change still looks good to me.

Yeah gcc is not clever enought to optimize this away since we can't tell
it that fb->modifier is invariant forever once assigned.
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Jani, this one is for 4.0-rc.

Thanks, Daniel

> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 8ccf033..900dcaa 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12197,9 +12197,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
> >  		return -ENOMEM;
> >  	}
> >  
> > -	if (fb == crtc->cursor->fb)
> > -		return 0;
> > -
> >  	if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
> >  		DRM_DEBUG_KMS("cursor cannot be tiled\n");
> >  		ret = -EINVAL;
> > -- 
> > 2.1.0
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[Jani Nikula]

On Wed, 25 Feb 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Feb 24, 2015 at 01:42:39PM -0800, Matt Roper wrote:
>> On Tue, Feb 24, 2015 at 01:37:54PM -0800, Rodrigo Vivi wrote:
>> > This return 0 without setting atomic bits on fb == crtc->cursor->fb
>> > where causing frontbuffer false positives.
>> > 
>> > According to Daniel:
>> > 
>> > The original regression seems to have been introduced in the original
>> > check/commit split:
>> > 
>> > commit 757f9a3e5b8a812af0c213099a5b31cb423f4d3c
>> > Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> > Date:   Wed Sep 24 14:20:24 2014 -0300
>> > 
>> >     drm/i915: move check of intel_crtc_cursor_set_obj() out
>> > 
>> > Which already cause other trouble, resulting in the check getting moved in
>> > 
>> > commit e391ea882b1a04fb3f559287ac694652a3cd9da9
>> > Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> > Date:   Wed Sep 24 14:20:25 2014 -0300
>> > 
>> >     drm/i915: Fix not checking cursor and object sizes
>> > 
>> > The frontbuffer tracking itself only was broken when we shifted it into
>> > the check/commit logic with:
>> > 
>> > commit 32b7eeec4d1e861230b09d437e95d76c86ff4a68
>> > Author: Matt Roper <matthew.d.roper@intel.com>
>> > Date:   Wed Dec 24 07:59:06 2014 -0800
>> > 
>> >     drm/i915: Refactor work that can sleep out of commit (v7)
>> > 
>> > v2: When putting more debug prints I notice the solution was simpler
>> > than I thought. AMS design is solid, just this return was wrong.
>> > Sorry for the noise.
>> > 
>> > v3: Remove the entire chunck that would probably
>> >     be removed by gcc anyway. (by Daniel)
>> > 
>> > Cc: Jani Nikula <jani.nikula@intel.com>
>> > Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> > Cc: Matt Roper <matthew.d.roper@intel.com>
>> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> 
>> Not sure I understand Daniel's remark about gcc optimization, but the
>> change still looks good to me.
>
> Yeah gcc is not clever enought to optimize this away since we can't tell
> it that fb->modifier is invariant forever once assigned.
>> 
>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Jani, this one is for 4.0-rc.

Pushed to drm-intel-fixes, thanks for the patch and reviews.

I had to adjust the context for backporting to v4.0-rc1, please check
that drm-intel-fixes makes sense *and* that my merge resolution back to
drm-intel-nightly makes sense. Thanks.

BR,
Jani.


>
> Thanks, Daniel
>
>> 
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c | 3 ---
>> >  1 file changed, 3 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index 8ccf033..900dcaa 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -12197,9 +12197,6 @@ intel_check_cursor_plane(struct drm_plane *plane,
>> >  		return -ENOMEM;
>> >  	}
>> >  
>> > -	if (fb == crtc->cursor->fb)
>> > -		return 0;
>> > -
>> >  	if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) {
>> >  		DRM_DEBUG_KMS("cursor cannot be tiled\n");
>> >  		ret = -EINVAL;
>> > -- 
>> > 2.1.0
>> > 
>> 
>> -- 
>> Matt Roper
>> Graphics Software Engineer
>> IoTG Platform Enabling & Development
>> Intel Corporation
>> (916) 356-2795
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5819
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -1              281/281              280/281
ILK                                  308/308              308/308
SNB                 -1              326/326              325/326
IVB                                  380/380              380/380
BYT                                  258/258              258/258
HSW                 -1              421/421              420/421
BDW                 -1              316/316              315/316
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 PNV  igt_gem_userptr_blits_minor-normal-sync      DMESG_WARN(2)PASS(1)      DMESG_WARN(1)PASS(1)
 SNB  igt_kms_plane_plane-panning-bottom-right-pipe-B-plane-1      DMESG_WARN(12)PASS(3)      DMESG_WARN(1)PASS(1)
*HSW  igt_gem_storedw_batches_loop_normal      PASS(2)      DMESG_WARN(1)PASS(1)
*BDW  igt_gem_gtt_hog      PASS(7)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix frontbuffer false positve.

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5820
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -1              281/281              280/281
ILK                                  308/308              308/308
SNB                 -1              326/326              325/326
IVB                                  380/380              380/380
BYT                                  294/294              294/294
HSW                                  387/421              387/421
BDW                 -1              316/316              315/316
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 PNV  igt_gem_userptr_blits_minor-unsync-normal      DMESG_WARN(2)PASS(1)      DMESG_WARN(1)PASS(1)
 SNB  igt_kms_plane_plane-position-hole-pipe-B-plane-1      DMESG_WARN(12)PASS(3)      DMESG_WARN(1)PASS(1)
*BDW  igt_gem_gtt_hog      PASS(8)      DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx