Re: [Intel-gfx] [PATCH] drm/i915: Flush pending interrupt following a GPU reset

[Jeff McGee]

On Wed, Mar 21, 2018 at 04:42:32PM +, Chris Wilson wrote:
> Quoting Jeff McGee (2018-03-21 15:55:16)
> > On Wed, Mar 21, 2018 at 03:00:23PM +, Chris Wilson wrote:
> > > After resetting the GPU (or subset of engines), call synchronize_irq()
> > > to flush any pending irq before proceeding with the cleanup. For a
> > > device level reset, we disable the interupts around the reset, but when
> > > resetting just one engine, we have to avoid such global disabling. This
> > > leaves us open to an interrupt arriving for the engine as we try to
> > > reset it. We already do try to flush the IIR following the reset, but we
> > > have to ensure that the in-flight interrupt does not land after we start
> > > cleaning up after the reset; enter synchronize_irq().
> > > 
> > > As it current stands, we very rarely, but fatally, see sequences such as:
> > > 
> > > 2 57964564us : execlists_reset_prepare: rcs0
> > > 2 57964613us : execlists_reset: rcs0 seqno=424
> > > 0d.h1 57964615us : gen8_cs_irq_handler: rcs0 CS active=1
> > > 2d..1 57964617us : __i915_request_unsubmit: rcs0 fence 29:1056 <- 
> > > global_seqno 1060
> > > 2 57964703us : execlists_reset_finish: rcs0
> > > 0..s. 57964705us : execlists_submission_tasklet: rcs0 awake?=1, 
> > > active=0, irq-posted?=1
> > > 
> > I can repro this sequence easily with force preemption IGT.
> 
> With the sequence I suggested?
> -Chris

Yes. Your approach to protecting port[1] context is working well. This is
the only issue I'm still hitting. I'll post my updated RFC set in a sec.
-Jeff
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Flush pending interrupt following a GPU reset

[Chris Wilson]

Quoting Jeff McGee (2018-03-21 15:55:16)
> On Wed, Mar 21, 2018 at 03:00:23PM +, Chris Wilson wrote:
> > After resetting the GPU (or subset of engines), call synchronize_irq()
> > to flush any pending irq before proceeding with the cleanup. For a
> > device level reset, we disable the interupts around the reset, but when
> > resetting just one engine, we have to avoid such global disabling. This
> > leaves us open to an interrupt arriving for the engine as we try to
> > reset it. We already do try to flush the IIR following the reset, but we
> > have to ensure that the in-flight interrupt does not land after we start
> > cleaning up after the reset; enter synchronize_irq().
> > 
> > As it current stands, we very rarely, but fatally, see sequences such as:
> > 
> > 2 57964564us : execlists_reset_prepare: rcs0
> > 2 57964613us : execlists_reset: rcs0 seqno=424
> > 0d.h1 57964615us : gen8_cs_irq_handler: rcs0 CS active=1
> > 2d..1 57964617us : __i915_request_unsubmit: rcs0 fence 29:1056 <- 
> > global_seqno 1060
> > 2 57964703us : execlists_reset_finish: rcs0
> > 0..s. 57964705us : execlists_submission_tasklet: rcs0 awake?=1, 
> > active=0, irq-posted?=1
> > 
> I can repro this sequence easily with force preemption IGT.

With the sequence I suggested?
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Flush pending interrupt following a GPU reset

[Chris Wilson]

Quoting Jeff McGee (2018-03-21 15:55:16)
> On Wed, Mar 21, 2018 at 03:00:23PM +, Chris Wilson wrote:
> > After resetting the GPU (or subset of engines), call synchronize_irq()
> > to flush any pending irq before proceeding with the cleanup. For a
> > device level reset, we disable the interupts around the reset, but when
> > resetting just one engine, we have to avoid such global disabling. This
> > leaves us open to an interrupt arriving for the engine as we try to
> > reset it. We already do try to flush the IIR following the reset, but we
> > have to ensure that the in-flight interrupt does not land after we start
> > cleaning up after the reset; enter synchronize_irq().
> > 
> > As it current stands, we very rarely, but fatally, see sequences such as:
> > 
> > 2 57964564us : execlists_reset_prepare: rcs0
> > 2 57964613us : execlists_reset: rcs0 seqno=424
> > 0d.h1 57964615us : gen8_cs_irq_handler: rcs0 CS active=1
> > 2d..1 57964617us : __i915_request_unsubmit: rcs0 fence 29:1056 <- 
> > global_seqno 1060
> > 2 57964703us : execlists_reset_finish: rcs0
> > 0..s. 57964705us : execlists_submission_tasklet: rcs0 awake?=1, 
> > active=0, irq-posted?=1
> > 
> I can repro this sequence easily with force preemption IGT. Just tried this
> patch and the issue is still there. For the moment I can just mitigate with
> https://patchwork.freedesktop.org/patch/211086/

These patch is purely to make sure that the irq_handler is not called
after execlists_reset, as that is what we rely on.

We *should* never be in a situation where CSB tail is invalid, that
implies either we screwed up or the hw is incoherent.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Flush pending interrupt following a GPU reset

[Jeff McGee]

On Wed, Mar 21, 2018 at 03:00:23PM +, Chris Wilson wrote:
> After resetting the GPU (or subset of engines), call synchronize_irq()
> to flush any pending irq before proceeding with the cleanup. For a
> device level reset, we disable the interupts around the reset, but when
> resetting just one engine, we have to avoid such global disabling. This
> leaves us open to an interrupt arriving for the engine as we try to
> reset it. We already do try to flush the IIR following the reset, but we
> have to ensure that the in-flight interrupt does not land after we start
> cleaning up after the reset; enter synchronize_irq().
> 
> As it current stands, we very rarely, but fatally, see sequences such as:
> 
> 2 57964564us : execlists_reset_prepare: rcs0
> 2 57964613us : execlists_reset: rcs0 seqno=424
> 0d.h1 57964615us : gen8_cs_irq_handler: rcs0 CS active=1
> 2d..1 57964617us : __i915_request_unsubmit: rcs0 fence 29:1056 <- 
> global_seqno 1060
> 2 57964703us : execlists_reset_finish: rcs0
> 0..s. 57964705us : execlists_submission_tasklet: rcs0 awake?=1, active=0, 
> irq-posted?=1
> 
I can repro this sequence easily with force preemption IGT. Just tried this
patch and the issue is still there. For the moment I can just mitigate with
https://patchwork.freedesktop.org/patch/211086/

> Signed-off-by: Chris Wilson 
> Cc: Mika Kuoppala 
> Cc: Michel Thierry 
> Cc: Michał Winiarski 
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
> b/drivers/gpu/drm/i915/intel_uncore.c
> index 4c616d074a97..04830d6125d6 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -2116,11 +2116,14 @@ int intel_gpu_reset(struct drm_i915_private 
> *dev_priv, unsigned engine_mask)
>   i915_stop_engines(dev_priv, engine_mask);
>  
>   ret = -ENODEV;
> - if (reset)
> + if (reset) {
> + GEM_TRACE("engine_mask=%x\n", engine_mask);
>   ret = reset(dev_priv, engine_mask);
> + }
>   if (ret != -ETIMEDOUT)
>   break;
>  
> + synchronize_irq(dev_priv->drm.irq);
>   cond_resched();
>   }
>   intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> -- 
> 2.16.2
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Flush pending interrupt following a GPU reset

[Chris Wilson]

After resetting the GPU (or subset of engines), call synchronize_irq()
to flush any pending irq before proceeding with the cleanup. For a
device level reset, we disable the interupts around the reset, but when
resetting just one engine, we have to avoid such global disabling. This
leaves us open to an interrupt arriving for the engine as we try to
reset it. We already do try to flush the IIR following the reset, but we
have to ensure that the in-flight interrupt does not land after we start
cleaning up after the reset; enter synchronize_irq().

As it current stands, we very rarely, but fatally, see sequences such as:

2 57964564us : execlists_reset_prepare: rcs0
2 57964613us : execlists_reset: rcs0 seqno=424
0d.h1 57964615us : gen8_cs_irq_handler: rcs0 CS active=1
2d..1 57964617us : __i915_request_unsubmit: rcs0 fence 29:1056 <- 
global_seqno 1060
2 57964703us : execlists_reset_finish: rcs0
0..s. 57964705us : execlists_submission_tasklet: rcs0 awake?=1, active=0, 
irq-posted?=1

Signed-off-by: Chris Wilson 
Cc: Mika Kuoppala 
Cc: Michel Thierry 
Cc: Michał Winiarski 
---
 drivers/gpu/drm/i915/intel_uncore.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
b/drivers/gpu/drm/i915/intel_uncore.c
index 4c616d074a97..04830d6125d6 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -2116,11 +2116,14 @@ int intel_gpu_reset(struct drm_i915_private *dev_priv, 
unsigned engine_mask)
i915_stop_engines(dev_priv, engine_mask);
 
ret = -ENODEV;
-   if (reset)
+   if (reset) {
+   GEM_TRACE("engine_mask=%x\n", engine_mask);
ret = reset(dev_priv, engine_mask);
+   }
if (ret != -ETIMEDOUT)
break;
 
+   synchronize_irq(dev_priv->drm.irq);
cond_resched();
}
intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
-- 
2.16.2

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