[PATCH] drm/i915: Ignore stuck requests when considering hangs

[PATCH] drm/i915: Ignore stuck requests when considering hangs

[Chris Wilson]

If the engine isn't being retired (worker starvation?) then it is
possible for us to repeatedly observe that between consecutive
hangchecks the seqno on the ring to be the same and there remain
unretired requests. Ignore these completely and only regard the engine
as busy for the purpose of hang detection (not stall detection) if there
are outstanding breadcrumbs.

In recent history we have looked at using both the request and seqno as
indication of activity on the engine, but that was reduced to just
inspecting seqno in commit cffa781e5907 ("drm/i915: Simplify check for
idleness in hangcheck"). However, in commit dcff85c8443e ("drm/i915:
Enable i915_gem_wait_for_idle() without holding struct_mutex"), I made
the decision to use the new common lockless function, under the
assumption that request retirement was more frequent than hangcheck and
so we would not have a stuck busy check. The flaw there was in
forgetting that we accumulate the hang score, and so successive checks
seeing a stuck request, albeit with the GPU advancing elsewhere and so
not necessary the same stuck request, would eventually trigger the hang.

Fixes: dcff85c8443e ("drm/i915: Enable i915_gem_wait_for_idle()...")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ebb83d5a448b..7610eca4f687 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3079,6 +3079,7 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 		bool busy = intel_engine_has_waiter(engine);
 		u64 acthd;
 		u32 seqno;
+		u32 submit;
 
 		semaphore_clear_deadlocks(dev_priv);
 
@@ -3094,9 +3095,10 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 
 		acthd = intel_engine_get_active_head(engine);
 		seqno = intel_engine_get_seqno(engine);
+		submit = READ_ONCE(engine->last_submitted_seqno);
 
 		if (engine->hangcheck.seqno == seqno) {
-			if (!intel_engine_is_active(engine)) {
+			if (i915_seqno_passed(seqno, submit)) {
 				engine->hangcheck.action = HANGCHECK_IDLE;
 				if (busy) {
 					/* Safeguard against driver failure */
-- 
2.9.3

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

✗ Ro.CI.BAT: failure for drm/i915: Ignore stuck requests when considering hangs

[Patchwork]

== Series Details ==

Series: drm/i915: Ignore stuck requests when considering hangs
URL   : https://patchwork.freedesktop.org/series/11359/
State : failure

== Summary ==

Series 11359v1 drm/i915: Ignore stuck requests when considering hangs
http://patchwork.freedesktop.org/api/1.0/series/11359/revisions/1/mbox

Test kms_cursor_legacy:
        Subgroup basic-cursor-vs-flip-legacy:
                dmesg-warn -> PASS       (ro-bdw-i5-5250u)
        Subgroup basic-cursor-vs-flip-varying-size:
                dmesg-warn -> PASS       (ro-bdw-i5-5250u)
        Subgroup basic-flip-vs-cursor-varying-size:
                pass       -> FAIL       (ro-skl3-i5-6260u)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                skip       -> DMESG-WARN (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-c:
                skip       -> PASS       (fi-hsw-i7-4770k)

fi-hsw-i7-4770k  total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-qkkr      total:244  pass:185  dwarn:29  dfail:0   fail:4   skip:26 
fi-snb-i7-2600   total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
ro-bdw-i5-5250u  total:240  pass:220  dwarn:3   dfail:0   fail:0   skip:17 
ro-bdw-i7-5557U  total:240  pass:220  dwarn:3   dfail:0   fail:0   skip:17 
ro-bdw-i7-5600u  total:240  pass:206  dwarn:0   dfail:0   fail:2   skip:32 
ro-bsw-n3050     total:240  pass:195  dwarn:0   dfail:0   fail:3   skip:42 
ro-hsw-i3-4010u  total:240  pass:213  dwarn:0   dfail:0   fail:1   skip:26 
ro-hsw-i7-4770r  total:240  pass:185  dwarn:0   dfail:0   fail:0   skip:55 
ro-ilk1-i5-650   total:235  pass:174  dwarn:0   dfail:0   fail:2   skip:59 
ro-ivb-i7-3770   total:240  pass:204  dwarn:0   dfail:0   fail:1   skip:35 
ro-ivb2-i7-3770  total:240  pass:208  dwarn:0   dfail:0   fail:1   skip:31 
ro-skl3-i5-6260u total:240  pass:222  dwarn:0   dfail:0   fail:4   skip:14 
fi-skl-i7-6700k failed to connect after reboot
ro-byt-n2820 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1952/

f53a8d1 drm-intel-nightly: 2016y-08m-19d-16h-24m-21s UTC integration manifest
236161f drm/i915: Ignore stuck requests when considering hangs

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

Re: [PATCH] drm/i915: Ignore stuck requests when considering hangs

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> If the engine isn't being retired (worker starvation?) then it is
> possible for us to repeatedly observe that between consecutive
> hangchecks the seqno on the ring to be the same and there remain
> unretired requests. Ignore these completely and only regard the engine
> as busy for the purpose of hang detection (not stall detection) if there
> are outstanding breadcrumbs.
>
> In recent history we have looked at using both the request and seqno as
> indication of activity on the engine, but that was reduced to just
> inspecting seqno in commit cffa781e5907 ("drm/i915: Simplify check for
> idleness in hangcheck"). However, in commit dcff85c8443e ("drm/i915:
> Enable i915_gem_wait_for_idle() without holding struct_mutex"), I made
> the decision to use the new common lockless function, under the
> assumption that request retirement was more frequent than hangcheck and
> so we would not have a stuck busy check. The flaw there was in
> forgetting that we accumulate the hang score, and so successive checks
> seeing a stuck request, albeit with the GPU advancing elsewhere and so
> not necessary the same stuck request, would eventually trigger the hang.
>
> Fixes: dcff85c8443e ("drm/i915: Enable i915_gem_wait_for_idle()...")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index ebb83d5a448b..7610eca4f687 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3079,6 +3079,7 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
>  		bool busy = intel_engine_has_waiter(engine);
>  		u64 acthd;
>  		u32 seqno;
> +		u32 submit;
>  
>  		semaphore_clear_deadlocks(dev_priv);
>  
> @@ -3094,9 +3095,10 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
>  
>  		acthd = intel_engine_get_active_head(engine);
>  		seqno = intel_engine_get_seqno(engine);
> +		submit = READ_ONCE(engine->last_submitted_seqno);
>  
>  		if (engine->hangcheck.seqno == seqno) {
> -			if (!intel_engine_is_active(engine)) {
> +			if (i915_seqno_passed(seqno, submit)) {

Setting of busy could be moved in the in scope.

Also the check could be seqno == submit and warning if we see
seqno on engine past submit.

But the patch fixes what it says it does,

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

>  				engine->hangcheck.action = HANGCHECK_IDLE;
>  				if (busy) {
>  					/* Safeguard against driver failure */
> -- 
> 2.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Ignore stuck requests when considering hangs

[Chris Wilson]

On Mon, Aug 22, 2016 at 02:39:30PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > If the engine isn't being retired (worker starvation?) then it is
> > possible for us to repeatedly observe that between consecutive
> > hangchecks the seqno on the ring to be the same and there remain
> > unretired requests. Ignore these completely and only regard the engine
> > as busy for the purpose of hang detection (not stall detection) if there
> > are outstanding breadcrumbs.
> >
> > In recent history we have looked at using both the request and seqno as
> > indication of activity on the engine, but that was reduced to just
> > inspecting seqno in commit cffa781e5907 ("drm/i915: Simplify check for
> > idleness in hangcheck"). However, in commit dcff85c8443e ("drm/i915:
> > Enable i915_gem_wait_for_idle() without holding struct_mutex"), I made
> > the decision to use the new common lockless function, under the
> > assumption that request retirement was more frequent than hangcheck and
> > so we would not have a stuck busy check. The flaw there was in
> > forgetting that we accumulate the hang score, and so successive checks
> > seeing a stuck request, albeit with the GPU advancing elsewhere and so
> > not necessary the same stuck request, would eventually trigger the hang.
> >
> > Fixes: dcff85c8443e ("drm/i915: Enable i915_gem_wait_for_idle()...")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index ebb83d5a448b..7610eca4f687 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -3079,6 +3079,7 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
> >  		bool busy = intel_engine_has_waiter(engine);
> >  		u64 acthd;
> >  		u32 seqno;
> > +		u32 submit;
> >  
> >  		semaphore_clear_deadlocks(dev_priv);
> >  
> > @@ -3094,9 +3095,10 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
> >  
> >  		acthd = intel_engine_get_active_head(engine);
> >  		seqno = intel_engine_get_seqno(engine);
> > +		submit = READ_ONCE(engine->last_submitted_seqno);
> >  
> >  		if (engine->hangcheck.seqno == seqno) {
> > -			if (!intel_engine_is_active(engine)) {
> > +			if (i915_seqno_passed(seqno, submit)) {
> 
> Setting of busy could be moved in the in scope.

busy/busy_count have lost their meanings. I really did consider renaming
them to something like rearm instead. I hesistated in doing exactly the
same as you suggested here just in case the rearming is sensitive to the
ordering. (I don't think so, it just required some clear thought towards
the ordering of enabling hangcheck from the waiter vs disabling here.)
 
> Also the check could be seqno == submit and warning if we see
> seqno on engine past submit.

That's true, but I'm not concerned about such an error - it should
result in no breakages, and don't plan on fixing such a warning if it
were ever to be reported ;)
-Chris

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