[PATCH] drm/i915: Do not trigger the pageflip stall check too early

[PATCH] drm/i915: Do not trigger the pageflip stall check too early

[Chris Wilson]

On gen2-4, we have a separate pageflip prepare/finish phase. The intent
of the stall check is to detect when we have incurred a delay,
potentially indefinite, after the pageflip is submitted and before the
flip is processed by the hardware. However, our notion of
INTEL_FLIP_PENDING/INTEL_FLIP_COMPLETE do not tally with how we set the
values during prepare/finish and the current stall check erroneously
assumes that when the pending value >= COMPLETE, the driver has seen the
hardware completion flag. But what the driver actually means, is that it
has seen the acknowlegement that the flip is queued and is now pending
the completion event.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85888
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 70a75136bc61..0c43d970dbf9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9707,8 +9707,8 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev,
 	struct intel_unpin_work *work = intel_crtc->unpin_work;
 	u32 addr;
 
-	if (atomic_read(&work->pending) >= INTEL_FLIP_COMPLETE)
-		return true;
+	if (atomic_read(&work->pending) < INTEL_FLIP_COMPLETE)
+		return false;
 
 	if (!work->enable_stall_check)
 		return false;
-- 
2.1.3

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

Re: [PATCH] drm/i915: Do not trigger the pageflip stall check too early

[Chris Wilson]

On Wed, Dec 17, 2014 at 04:41:42PM +0000, Chris Wilson wrote:
> On gen2-4, we have a separate pageflip prepare/finish phase. The intent
> of the stall check is to detect when we have incurred a delay,
> potentially indefinite, after the pageflip is submitted and before the
> flip is processed by the hardware. However, our notion of
> INTEL_FLIP_PENDING/INTEL_FLIP_COMPLETE do not tally with how we set the
> values during prepare/finish and the current stall check erroneously
> assumes that when the pending value >= COMPLETE, the driver has seen the
> hardware completion flag. But what the driver actually means, is that it
> has seen the acknowlegement that the flip is queued and is now pending
> the completion event.

Bah, otoh, as we don't mark the flip as pending before completion on
gen5+, we know don't detect when the flip is wedged there after applying
this patch.

It is quite possible that the work->pending check is entirely bogus.
Back to the drawing board.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Do not trigger the pageflip stall check too early

[Ville Syrjälä]

On Wed, Dec 17, 2014 at 04:48:19PM +0000, Chris Wilson wrote:
> On Wed, Dec 17, 2014 at 04:41:42PM +0000, Chris Wilson wrote:
> > On gen2-4, we have a separate pageflip prepare/finish phase. The intent
> > of the stall check is to detect when we have incurred a delay,
> > potentially indefinite, after the pageflip is submitted and before the
> > flip is processed by the hardware. However, our notion of
> > INTEL_FLIP_PENDING/INTEL_FLIP_COMPLETE do not tally with how we set the
> > values during prepare/finish and the current stall check erroneously
> > assumes that when the pending value >= COMPLETE, the driver has seen the
> > hardware completion flag. But what the driver actually means, is that it
> > has seen the acknowlegement that the flip is queued and is now pending
> > the completion event.
> 
> Bah, otoh, as we don't mark the flip as pending before completion on
> gen5+, we know don't detect when the flip is wedged there after applying
> this patch.
> 
> It is quite possible that the work->pending check is entirely bogus.
> Back to the drawing board.

We just apply my fix. And then we can even get rid of the whole
prepare/finish mess.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Do not trigger the pageflip stall check too early

[Chris Wilson]

On Wed, Dec 17, 2014 at 08:19:00PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 17, 2014 at 04:48:19PM +0000, Chris Wilson wrote:
> > On Wed, Dec 17, 2014 at 04:41:42PM +0000, Chris Wilson wrote:
> > > On gen2-4, we have a separate pageflip prepare/finish phase. The intent
> > > of the stall check is to detect when we have incurred a delay,
> > > potentially indefinite, after the pageflip is submitted and before the
> > > flip is processed by the hardware. However, our notion of
> > > INTEL_FLIP_PENDING/INTEL_FLIP_COMPLETE do not tally with how we set the
> > > values during prepare/finish and the current stall check erroneously
> > > assumes that when the pending value >= COMPLETE, the driver has seen the
> > > hardware completion flag. But what the driver actually means, is that it
> > > has seen the acknowlegement that the flip is queued and is now pending
> > > the completion event.
> > 
> > Bah, otoh, as we don't mark the flip as pending before completion on
> > gen5+, we know don't detect when the flip is wedged there after applying
> > this patch.
> > 
> > It is quite possible that the work->pending check is entirely bogus.
> > Back to the drawing board.
> 
> We just apply my fix. And then we can even get rid of the whole
> prepare/finish mess.

I concur. I was trying to see if there was any value in trusting IIR but
not ISR for gen2/3, but that's silly. So with the current flip interrupt
handling we really do not have the two phases anymore.

https://bugs.freedesktop.org/attachment.cgi?id=110912
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Do not trigger the pageflip stall check too early

[Daniel Vetter]

On Wed, Dec 17, 2014 at 06:35:04PM +0000, Chris Wilson wrote:
> On Wed, Dec 17, 2014 at 08:19:00PM +0200, Ville Syrjälä wrote:
> > On Wed, Dec 17, 2014 at 04:48:19PM +0000, Chris Wilson wrote:
> > > On Wed, Dec 17, 2014 at 04:41:42PM +0000, Chris Wilson wrote:
> > > > On gen2-4, we have a separate pageflip prepare/finish phase. The intent
> > > > of the stall check is to detect when we have incurred a delay,
> > > > potentially indefinite, after the pageflip is submitted and before the
> > > > flip is processed by the hardware. However, our notion of
> > > > INTEL_FLIP_PENDING/INTEL_FLIP_COMPLETE do not tally with how we set the
> > > > values during prepare/finish and the current stall check erroneously
> > > > assumes that when the pending value >= COMPLETE, the driver has seen the
> > > > hardware completion flag. But what the driver actually means, is that it
> > > > has seen the acknowlegement that the flip is queued and is now pending
> > > > the completion event.
> > > 
> > > Bah, otoh, as we don't mark the flip as pending before completion on
> > > gen5+, we know don't detect when the flip is wedged there after applying
> > > this patch.
> > > 
> > > It is quite possible that the work->pending check is entirely bogus.
> > > Back to the drawing board.
> > 
> > We just apply my fix. And then we can even get rid of the whole
> > prepare/finish mess.
> 
> I concur. I was trying to see if there was any value in trusting IIR but
> not ISR for gen2/3, but that's silly. So with the current flip interrupt
> handling we really do not have the two phases anymore.
> 
> https://bugs.freedesktop.org/attachment.cgi?id=110912
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Jani can you please pick this on up directly? It doesn't seem to have ever
reached intel-gfx unfortunately, at least google doesn't find it.
-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: Do not trigger the pageflip stall check too early

[Ville Syrjälä]

On Wed, Dec 17, 2014 at 09:59:09PM +0100, Daniel Vetter wrote:
> On Wed, Dec 17, 2014 at 06:35:04PM +0000, Chris Wilson wrote:
> > On Wed, Dec 17, 2014 at 08:19:00PM +0200, Ville Syrjälä wrote:
> > > On Wed, Dec 17, 2014 at 04:48:19PM +0000, Chris Wilson wrote:
> > > > On Wed, Dec 17, 2014 at 04:41:42PM +0000, Chris Wilson wrote:
> > > > > On gen2-4, we have a separate pageflip prepare/finish phase. The intent
> > > > > of the stall check is to detect when we have incurred a delay,
> > > > > potentially indefinite, after the pageflip is submitted and before the
> > > > > flip is processed by the hardware. However, our notion of
> > > > > INTEL_FLIP_PENDING/INTEL_FLIP_COMPLETE do not tally with how we set the
> > > > > values during prepare/finish and the current stall check erroneously
> > > > > assumes that when the pending value >= COMPLETE, the driver has seen the
> > > > > hardware completion flag. But what the driver actually means, is that it
> > > > > has seen the acknowlegement that the flip is queued and is now pending
> > > > > the completion event.
> > > > 
> > > > Bah, otoh, as we don't mark the flip as pending before completion on
> > > > gen5+, we know don't detect when the flip is wedged there after applying
> > > > this patch.
> > > > 
> > > > It is quite possible that the work->pending check is entirely bogus.
> > > > Back to the drawing board.
> > > 
> > > We just apply my fix. And then we can even get rid of the whole
> > > prepare/finish mess.
> > 
> > I concur. I was trying to see if there was any value in trusting IIR but
> > not ISR for gen2/3, but that's silly. So with the current flip interrupt
> > handling we really do not have the two phases anymore.
> > 
> > https://bugs.freedesktop.org/attachment.cgi?id=110912
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Jani can you please pick this on up directly? It doesn't seem to have ever
> reached intel-gfx unfortunately, at least google doesn't find it.

It was waiting for test results. Posted now:
http://lists.freedesktop.org/archives/intel-gfx/2014-December/057746.html

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Do not trigger the pageflip stall check too early

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK              +1-1              364/366              364/366
SNB                                  448/450              448/450
IVB                                  497/498              497/498
BYT                                  289/289              289/289
HSW                                  563/564              563/564
BDW                                  416/417              416/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt_kms_pipe_crc_basic_bad-nb-words-1      PASS(2, M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_bcs-flip-vs-modeset-interruptible      DMESG_WARN(1, M26)PASS(2, M37M26)      PASS(1, M26)
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