[PATCH] drm/i915: Reset the breadcrumbs IRQ more carefully

[PATCH] drm/i915: Reset the breadcrumbs IRQ more carefully

[Chris Wilson]

Along with the interrupt, we want to restore the fake-irq and
wait-timeout detection. If we use the breadcrumbs interface to setup the
interrupt as it wants, the auxiliary timers will also be restored.

Fixes: 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete requests")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 17 +++++++++++++++++
 drivers/gpu/drm/i915/intel_engine_cs.c   | 15 ---------------
 drivers/gpu/drm/i915/intel_lrc.c         |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c  |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h  |  2 +-
 5 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 9dba4971fb1e..d27da6d69735 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -584,6 +584,23 @@ int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
 	return 0;
 }
 
+void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
+{
+	struct intel_breadcrumbs *b = &engine->breadcrumbs;
+
+	clear_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
+
+	spin_lock(&b->lock);
+
+	__intel_breadcrumbs_disable_irq(b);
+	if (intel_engine_has_waiter(engine)) {
+		b->timeout = wait_timeout();
+		__intel_breadcrumbs_enable_irq(b);
+	}
+
+	spin_unlock(&b->lock);
+}
+
 void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
 {
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index c8ac72ba4000..755f1a8b76d8 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -210,9 +210,6 @@ void intel_engine_init_global_seqno(struct intel_engine_cs *engine, u32 seqno)
 void intel_engine_init_hangcheck(struct intel_engine_cs *engine)
 {
 	memset(&engine->hangcheck, 0, sizeof(engine->hangcheck));
-	clear_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
-	if (intel_engine_has_waiter(engine))
-		i915_queue_hangcheck(engine->i915);
 }
 
 static void intel_engine_init_timeline(struct intel_engine_cs *engine)
@@ -308,18 +305,6 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
 	return 0;
 }
 
-void intel_engine_reset_irq(struct intel_engine_cs *engine)
-{
-	struct drm_i915_private *dev_priv = engine->i915;
-
-	spin_lock_irq(&dev_priv->irq_lock);
-	if (intel_engine_has_waiter(engine))
-		engine->irq_enable(engine);
-	else
-		engine->irq_disable(engine);
-	spin_unlock_irq(&dev_priv->irq_lock);
-}
-
 /**
  * intel_engines_cleanup_common - cleans up the engine state created by
  *                                the common initiailizers.
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index bf22c94c3d53..eb162553cff2 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1199,7 +1199,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
 
 	lrc_init_hws(engine);
 
-	intel_engine_reset_irq(engine);
+	intel_engine_reset_breadcrumbs(engine);
 
 	I915_WRITE(RING_HWSTAM(engine->mmio_base), 0xffffffff);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 4bc47af68454..3abfbe3cfed9 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -548,7 +548,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
 	else
 		intel_ring_setup_status_page(engine);
 
-	intel_engine_reset_irq(engine);
+	intel_engine_reset_breadcrumbs(engine);
 
 	/* Enforce ordering by reading HEAD register back */
 	I915_READ_HEAD(engine);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 29d37b7c6021..a888f68d63d9 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -482,7 +482,6 @@ int __intel_ring_space(int head, int tail, int size);
 void intel_ring_update_space(struct intel_ring *ring);
 
 void intel_engine_init_global_seqno(struct intel_engine_cs *engine, u32 seqno);
-void intel_engine_reset_irq(struct intel_engine_cs *engine);
 
 void intel_engine_setup_common(struct intel_engine_cs *engine);
 int intel_engine_init_common(struct intel_engine_cs *engine);
@@ -568,6 +567,7 @@ static inline bool intel_engine_wakeup(const struct intel_engine_cs *engine)
 	return wakeup;
 }
 
+void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
 void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
 unsigned int intel_kick_waiters(struct drm_i915_private *i915);
 unsigned int intel_kick_signalers(struct drm_i915_private *i915);
-- 
2.9.3

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

Re: [PATCH] drm/i915: Reset the breadcrumbs IRQ more carefully

[Mika Kuoppala]

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

> Along with the interrupt, we want to restore the fake-irq and
> wait-timeout detection. If we use the breadcrumbs interface to setup the
> interrupt as it wants, the auxiliary timers will also be restored.
>
> Fixes: 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete requests")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 17 +++++++++++++++++
>  drivers/gpu/drm/i915/intel_engine_cs.c   | 15 ---------------
>  drivers/gpu/drm/i915/intel_lrc.c         |  2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c  |  2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.h  |  2 +-
>  5 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 9dba4971fb1e..d27da6d69735 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -584,6 +584,23 @@ int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
>  	return 0;
>  }
>  
> +void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
> +{
> +	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> +

Should we kill the timer before proceeding in here?

Not relevant to this patch but I also noticed that the period
is identical to hangcheck period. Multiple of hangcheck period
would be better, as our kicking might help and we don't
want to fallback to fake irqs just so easily.

-Mika

> +	clear_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
> +
> +	spin_lock(&b->lock);
> +
> +	__intel_breadcrumbs_disable_irq(b);
> +	if (intel_engine_has_waiter(engine)) {
> +		b->timeout = wait_timeout();
> +		__intel_breadcrumbs_enable_irq(b);
> +	}
> +
> +	spin_unlock(&b->lock);
> +}
> +
>  void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
>  {
>  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index c8ac72ba4000..755f1a8b76d8 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -210,9 +210,6 @@ void intel_engine_init_global_seqno(struct intel_engine_cs *engine, u32 seqno)
>  void intel_engine_init_hangcheck(struct intel_engine_cs *engine)
>  {
>  	memset(&engine->hangcheck, 0, sizeof(engine->hangcheck));
> -	clear_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
> -	if (intel_engine_has_waiter(engine))
> -		i915_queue_hangcheck(engine->i915);
>  }
>  
>  static void intel_engine_init_timeline(struct intel_engine_cs *engine)
> @@ -308,18 +305,6 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
>  	return 0;
>  }
>  
> -void intel_engine_reset_irq(struct intel_engine_cs *engine)
> -{
> -	struct drm_i915_private *dev_priv = engine->i915;
> -
> -	spin_lock_irq(&dev_priv->irq_lock);
> -	if (intel_engine_has_waiter(engine))
> -		engine->irq_enable(engine);
> -	else
> -		engine->irq_disable(engine);
> -	spin_unlock_irq(&dev_priv->irq_lock);
> -}
> -
>  /**
>   * intel_engines_cleanup_common - cleans up the engine state created by
>   *                                the common initiailizers.
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index bf22c94c3d53..eb162553cff2 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1199,7 +1199,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
>  
>  	lrc_init_hws(engine);
>  
> -	intel_engine_reset_irq(engine);
> +	intel_engine_reset_breadcrumbs(engine);
>  
>  	I915_WRITE(RING_HWSTAM(engine->mmio_base), 0xffffffff);
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 4bc47af68454..3abfbe3cfed9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -548,7 +548,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
>  	else
>  		intel_ring_setup_status_page(engine);
>  
> -	intel_engine_reset_irq(engine);
> +	intel_engine_reset_breadcrumbs(engine);
>  
>  	/* Enforce ordering by reading HEAD register back */
>  	I915_READ_HEAD(engine);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 29d37b7c6021..a888f68d63d9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -482,7 +482,6 @@ int __intel_ring_space(int head, int tail, int size);
>  void intel_ring_update_space(struct intel_ring *ring);
>  
>  void intel_engine_init_global_seqno(struct intel_engine_cs *engine, u32 seqno);
> -void intel_engine_reset_irq(struct intel_engine_cs *engine);
>  
>  void intel_engine_setup_common(struct intel_engine_cs *engine);
>  int intel_engine_init_common(struct intel_engine_cs *engine);
> @@ -568,6 +567,7 @@ static inline bool intel_engine_wakeup(const struct intel_engine_cs *engine)
>  	return wakeup;
>  }
>  
> +void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
>  void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
>  unsigned int intel_kick_waiters(struct drm_i915_private *i915);
>  unsigned int intel_kick_signalers(struct drm_i915_private *i915);
> -- 
> 2.9.3
>
> _______________________________________________
> 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

Re: [PATCH] drm/i915: Reset the breadcrumbs IRQ more carefully

[Chris Wilson]

On Thu, Oct 06, 2016 at 04:32:37PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Along with the interrupt, we want to restore the fake-irq and
> > wait-timeout detection. If we use the breadcrumbs interface to setup the
> > interrupt as it wants, the auxiliary timers will also be restored.
> >
> > Fixes: 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete requests")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_breadcrumbs.c | 17 +++++++++++++++++
> >  drivers/gpu/drm/i915/intel_engine_cs.c   | 15 ---------------
> >  drivers/gpu/drm/i915/intel_lrc.c         |  2 +-
> >  drivers/gpu/drm/i915/intel_ringbuffer.c  |  2 +-
> >  drivers/gpu/drm/i915/intel_ringbuffer.h  |  2 +-
> >  5 files changed, 20 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > index 9dba4971fb1e..d27da6d69735 100644
> > --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > @@ -584,6 +584,23 @@ int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
> >  	return 0;
> >  }
> >  
> > +void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
> > +{
> > +	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> > +
> 
> Should we kill the timer before proceeding in here?

Which timer? In breadcrumbs.c, we are concerned with the fake_irq and
the wait-timeout. The wait-timeout is reset below, we should add the
code to cancel the fake_irq along with clearing the bit.
 
> Not relevant to this patch but I also noticed that the period
> is identical to hangcheck period. Multiple of hangcheck period
> would be better, as our kicking might help and we don't
> want to fallback to fake irqs just so easily.

?

The main GPU hangcheck is kicked off by the wait timeout. Keeping the
two pieces independent (fake-irq, hangcheck) is quite nice, and the
jiffie wake up serves as a backup, and either it is required or it will
be disabled by the reset.
-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

Re: [PATCH] drm/i915: Reset the breadcrumbs IRQ more carefully

[Mika Kuoppala]

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

> On Thu, Oct 06, 2016 at 04:32:37PM +0300, Mika Kuoppala wrote:
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > Along with the interrupt, we want to restore the fake-irq and
>> > wait-timeout detection. If we use the breadcrumbs interface to setup the
>> > interrupt as it wants, the auxiliary timers will also be restored.
>> >
>> > Fixes: 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete requests")
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_breadcrumbs.c | 17 +++++++++++++++++
>> >  drivers/gpu/drm/i915/intel_engine_cs.c   | 15 ---------------
>> >  drivers/gpu/drm/i915/intel_lrc.c         |  2 +-
>> >  drivers/gpu/drm/i915/intel_ringbuffer.c  |  2 +-
>> >  drivers/gpu/drm/i915/intel_ringbuffer.h  |  2 +-
>> >  5 files changed, 20 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> > index 9dba4971fb1e..d27da6d69735 100644
>> > --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> > +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> > @@ -584,6 +584,23 @@ int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
>> >  	return 0;
>> >  }
>> >  
>> > +void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
>> > +{
>> > +	struct intel_breadcrumbs *b = &engine->breadcrumbs;
>> > +
>> 
>> Should we kill the timer before proceeding in here?
>
> Which timer? In breadcrumbs.c, we are concerned with the fake_irq and
> the wait-timeout. The wait-timeout is reset below, we should add the
> code to cancel the fake_irq along with clearing the bit.

I was considering that irqs are enabled and we have a
active breadcrumbs timer, triggering at the same time as
reset happens. So we would enable the fake irq as a post reset
race between reset/breadcrumbs hangcheck.

As in why not cancel and postpone the timer and only after
clear the missed_irq?

>  
>> Not relevant to this patch but I also noticed that the period
>> is identical to hangcheck period. Multiple of hangcheck period
>> would be better, as our kicking might help and we don't
>> want to fallback to fake irqs just so easily.
>
> ?
>
> The main GPU hangcheck is kicked off by the wait timeout. Keeping the
> two pieces independent (fake-irq, hangcheck) is quite nice, and the
> jiffie wake up serves as a backup, and either it is required or it will
> be disabled by the reset.

But we queue hangcheck also from retire work. So it could be that
we fallback to fake irqs, even if next hangcheck might have
managed to kick the wait and make forward progress?

And perhaps we should rename the breadcrumb hangcheck
as wait_watchdog to avoid confusion between different independant
'hangchecks'

-Mika

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

Re: [PATCH] drm/i915: Reset the breadcrumbs IRQ more carefully

[Chris Wilson]

On Thu, Oct 06, 2016 at 05:02:44PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > On Thu, Oct 06, 2016 at 04:32:37PM +0300, Mika Kuoppala wrote:
> >> Chris Wilson <chris@chris-wilson.co.uk> writes:
> >> 
> >> > Along with the interrupt, we want to restore the fake-irq and
> >> > wait-timeout detection. If we use the breadcrumbs interface to setup the
> >> > interrupt as it wants, the auxiliary timers will also be restored.
> >> >
> >> > Fixes: 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete requests")
> >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/intel_breadcrumbs.c | 17 +++++++++++++++++
> >> >  drivers/gpu/drm/i915/intel_engine_cs.c   | 15 ---------------
> >> >  drivers/gpu/drm/i915/intel_lrc.c         |  2 +-
> >> >  drivers/gpu/drm/i915/intel_ringbuffer.c  |  2 +-
> >> >  drivers/gpu/drm/i915/intel_ringbuffer.h  |  2 +-
> >> >  5 files changed, 20 insertions(+), 18 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >> > index 9dba4971fb1e..d27da6d69735 100644
> >> > --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >> > +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >> > @@ -584,6 +584,23 @@ int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
> >> >  	return 0;
> >> >  }
> >> >  
> >> > +void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
> >> > +{
> >> > +	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> >> > +
> >> 
> >> Should we kill the timer before proceeding in here?
> >
> > Which timer? In breadcrumbs.c, we are concerned with the fake_irq and
> > the wait-timeout. The wait-timeout is reset below, we should add the
> > code to cancel the fake_irq along with clearing the bit.
> 
> I was considering that irqs are enabled and we have a
> active breadcrumbs timer, triggering at the same time as
> reset happens. So we would enable the fake irq as a post reset
> race between reset/breadcrumbs hangcheck.
> 
> As in why not cancel and postpone the timer and only after
> clear the missed_irq?

So just picking up that we don't cancel the fake irq along with the
clear_bit() (currently justing for the wait to complete before
cancelling).

> >> Not relevant to this patch but I also noticed that the period
> >> is identical to hangcheck period. Multiple of hangcheck period
> >> would be better, as our kicking might help and we don't
> >> want to fallback to fake irqs just so easily.
> >
> > ?
> >
> > The main GPU hangcheck is kicked off by the wait timeout. Keeping the
> > two pieces independent (fake-irq, hangcheck) is quite nice, and the
> > jiffie wake up serves as a backup, and either it is required or it will
> > be disabled by the reset.
> 
> But we queue hangcheck also from retire work. So it could be that
> we fallback to fake irqs, even if next hangcheck might have
> managed to kick the wait and make forward progress?

Below the level of care. The limited kicking that hangcheck does is
immaterial to deciding whether or not we might need fake user interrupts.
-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

[PATCH v2] drm/i915: Reset the breadcrumbs IRQ more carefully

[Chris Wilson]

Along with the interrupt, we want to restore the fake-irq and
wait-timeout detection. If we use the breadcrumbs interface to setup the
interrupt as it wants, the auxiliary timers will also be restored.

v2: Cancel both timers as well, sanitize the IMR.

Fixes: 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete requests")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 31 +++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_engine_cs.c   | 15 ---------------
 drivers/gpu/drm/i915/intel_lrc.c         |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c  |  2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h  |  2 +-
 5 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 9bad14d22c95..3dd23c16bea1 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -578,6 +578,34 @@ int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
 	return 0;
 }
 
+static void cancel_fake_irq(struct intel_engine_cs *engine)
+{
+	struct intel_breadcrumbs *b = &engine->breadcrumbs;
+
+	del_timer_sync(&b->hangcheck);
+	del_timer_sync(&b->fake_irq);
+	clear_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
+}
+
+void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
+{
+	struct intel_breadcrumbs *b = &engine->breadcrumbs;
+
+	cancel_fake_irq(engine);
+	spin_lock(&b->lock);
+
+	__intel_breadcrumbs_disable_irq(b);
+	if (intel_engine_has_waiter(engine)) {
+		b->timeout = wait_timeout();
+		__intel_breadcrumbs_enable_irq(b);
+	} else {
+		/* sanitize the IMR and unmask any auxiliary interrupts */
+		irq_disable(engine);
+	}
+
+	spin_unlock(&b->lock);
+}
+
 void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
 {
 	struct intel_breadcrumbs *b = &engine->breadcrumbs;
@@ -585,8 +613,7 @@ void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
 	if (!IS_ERR_OR_NULL(b->signaler))
 		kthread_stop(b->signaler);
 
-	del_timer_sync(&b->hangcheck);
-	del_timer_sync(&b->fake_irq);
+	cancel_fake_irq(engine);
 }
 
 unsigned int intel_kick_waiters(struct drm_i915_private *i915)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index d00ec805f93d..480584c09306 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -210,9 +210,6 @@ void intel_engine_init_seqno(struct intel_engine_cs *engine, u32 seqno)
 void intel_engine_init_hangcheck(struct intel_engine_cs *engine)
 {
 	memset(&engine->hangcheck, 0, sizeof(engine->hangcheck));
-	clear_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
-	if (intel_engine_has_waiter(engine))
-		i915_queue_hangcheck(engine->i915);
 }
 
 static void intel_engine_init_requests(struct intel_engine_cs *engine)
@@ -307,18 +304,6 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
 	return 0;
 }
 
-void intel_engine_reset_irq(struct intel_engine_cs *engine)
-{
-	struct drm_i915_private *dev_priv = engine->i915;
-
-	spin_lock_irq(&dev_priv->irq_lock);
-	if (intel_engine_has_waiter(engine))
-		engine->irq_enable(engine);
-	else
-		engine->irq_disable(engine);
-	spin_unlock_irq(&dev_priv->irq_lock);
-}
-
 /**
  * intel_engines_cleanup_common - cleans up the engine state created by
  *                                the common initiailizers.
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 936f6f63f626..44904e298bfc 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1230,7 +1230,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
 
 	lrc_init_hws(engine);
 
-	intel_engine_reset_irq(engine);
+	intel_engine_reset_breadcrumbs(engine);
 
 	I915_WRITE(RING_HWSTAM(engine->mmio_base), 0xffffffff);
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 35f359e38f4d..729f373782e2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -548,7 +548,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
 	else
 		intel_ring_setup_status_page(engine);
 
-	intel_engine_reset_irq(engine);
+	intel_engine_reset_breadcrumbs(engine);
 
 	/* Enforce ordering by reading HEAD register back */
 	I915_READ_HEAD(engine);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 34954ca03a4a..124f4646958d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -520,7 +520,6 @@ int __intel_ring_space(int head, int tail, int size);
 void intel_ring_update_space(struct intel_ring *ring);
 
 void intel_engine_init_seqno(struct intel_engine_cs *engine, u32 seqno);
-void intel_engine_reset_irq(struct intel_engine_cs *engine);
 
 void intel_engine_setup_common(struct intel_engine_cs *engine);
 int intel_engine_init_common(struct intel_engine_cs *engine);
@@ -614,6 +613,7 @@ static inline bool intel_engine_wakeup(const struct intel_engine_cs *engine)
 	return wakeup;
 }
 
+void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
 void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
 unsigned int intel_kick_waiters(struct drm_i915_private *i915);
 unsigned int intel_kick_signalers(struct drm_i915_private *i915);
-- 
2.9.3

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

✗ Fi.CI.BAT: warning for drm/i915: Reset the breadcrumbs IRQ more carefully (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Reset the breadcrumbs IRQ more carefully (rev2)
URL   : https://patchwork.freedesktop.org/series/13375/
State : warning

== Summary ==

Series 13375v2 drm/i915: Reset the breadcrumbs IRQ more carefully
https://patchwork.freedesktop.org/api/1.0/series/13375/revisions/2/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> PASS       (fi-byt-j1900)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                pass       -> DMESG-WARN (fi-skl-6700hq)

fi-bdw-5557u     total:248  pass:231  dwarn:0   dfail:0   fail:0   skip:17 
fi-bsw-n3050     total:248  pass:204  dwarn:0   dfail:0   fail:0   skip:44 
fi-bxt-t5700     total:248  pass:217  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-j1900     total:248  pass:214  dwarn:1   dfail:0   fail:1   skip:32 
fi-byt-n2820     total:248  pass:211  dwarn:0   dfail:0   fail:1   skip:36 
fi-hsw-4770      total:248  pass:224  dwarn:0   dfail:0   fail:0   skip:24 
fi-hsw-4770r     total:248  pass:224  dwarn:0   dfail:0   fail:0   skip:24 
fi-ilk-650       total:248  pass:184  dwarn:0   dfail:0   fail:2   skip:62 
fi-ivb-3520m     total:248  pass:221  dwarn:0   dfail:0   fail:0   skip:27 
fi-ivb-3770      total:248  pass:208  dwarn:0   dfail:0   fail:0   skip:40 
fi-kbl-7200u     total:248  pass:222  dwarn:0   dfail:0   fail:0   skip:26 
fi-skl-6260u     total:248  pass:232  dwarn:0   dfail:0   fail:0   skip:16 
fi-skl-6700hq    total:248  pass:223  dwarn:1   dfail:0   fail:0   skip:24 
fi-skl-6700k     total:248  pass:221  dwarn:1   dfail:0   fail:0   skip:26 
fi-skl-6770hq    total:248  pass:231  dwarn:1   dfail:0   fail:1   skip:15 
fi-snb-2520m     total:248  pass:211  dwarn:0   dfail:0   fail:0   skip:37 
fi-snb-2600      total:248  pass:209  dwarn:0   dfail:0   fail:0   skip:39 

Results at /archive/results/CI_IGT_test/Patchwork_2640/

2dff18acaa95a26b882a5f9910d7ded514f18415 drm-intel-nightly: 2016y-10m-05d-13h-58m-08s UTC integration manifest
9ac590f drm/i915: Reset the breadcrumbs IRQ more carefully

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

Re: [PATCH v2] drm/i915: Reset the breadcrumbs IRQ more carefully

[Mika Kuoppala]

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

> Along with the interrupt, we want to restore the fake-irq and
> wait-timeout detection. If we use the breadcrumbs interface to setup the
> interrupt as it wants, the auxiliary timers will also be restored.
>
> v2: Cancel both timers as well, sanitize the IMR.
>
> Fixes: 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete requests")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 31 +++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_engine_cs.c   | 15 ---------------
>  drivers/gpu/drm/i915/intel_lrc.c         |  2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c  |  2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.h  |  2 +-
>  5 files changed, 32 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 9bad14d22c95..3dd23c16bea1 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -578,6 +578,34 @@ int intel_engine_init_breadcrumbs(struct intel_engine_cs *engine)
>  	return 0;
>  }
>  
> +static void cancel_fake_irq(struct intel_engine_cs *engine)
> +{
> +	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> +
> +	del_timer_sync(&b->hangcheck);
> +	del_timer_sync(&b->fake_irq);
> +	clear_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
> +}
> +
> +void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine)
> +{
> +	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> +
> +	cancel_fake_irq(engine);
> +	spin_lock(&b->lock);
> +
> +	__intel_breadcrumbs_disable_irq(b);
> +	if (intel_engine_has_waiter(engine)) {
> +		b->timeout = wait_timeout();
> +		__intel_breadcrumbs_enable_irq(b);
> +	} else {
> +		/* sanitize the IMR and unmask any auxiliary interrupts */
> +		irq_disable(engine);
> +	}
> +
> +	spin_unlock(&b->lock);
> +}
> +
>  void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
>  {
>  	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> @@ -585,8 +613,7 @@ void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine)
>  	if (!IS_ERR_OR_NULL(b->signaler))
>  		kthread_stop(b->signaler);
>  
> -	del_timer_sync(&b->hangcheck);
> -	del_timer_sync(&b->fake_irq);
> +	cancel_fake_irq(engine);
>  }
>  
>  unsigned int intel_kick_waiters(struct drm_i915_private *i915)
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index d00ec805f93d..480584c09306 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -210,9 +210,6 @@ void intel_engine_init_seqno(struct intel_engine_cs *engine, u32 seqno)
>  void intel_engine_init_hangcheck(struct intel_engine_cs *engine)
>  {
>  	memset(&engine->hangcheck, 0, sizeof(engine->hangcheck));
> -	clear_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
> -	if (intel_engine_has_waiter(engine))
> -		i915_queue_hangcheck(engine->i915);
>  }
>  
>  static void intel_engine_init_requests(struct intel_engine_cs *engine)
> @@ -307,18 +304,6 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
>  	return 0;
>  }
>  
> -void intel_engine_reset_irq(struct intel_engine_cs *engine)
> -{
> -	struct drm_i915_private *dev_priv = engine->i915;
> -
> -	spin_lock_irq(&dev_priv->irq_lock);
> -	if (intel_engine_has_waiter(engine))
> -		engine->irq_enable(engine);
> -	else
> -		engine->irq_disable(engine);
> -	spin_unlock_irq(&dev_priv->irq_lock);
> -}
> -
>  /**
>   * intel_engines_cleanup_common - cleans up the engine state created by
>   *                                the common initiailizers.
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 936f6f63f626..44904e298bfc 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1230,7 +1230,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
>  
>  	lrc_init_hws(engine);
>  
> -	intel_engine_reset_irq(engine);
> +	intel_engine_reset_breadcrumbs(engine);
>  
>  	I915_WRITE(RING_HWSTAM(engine->mmio_base), 0xffffffff);
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 35f359e38f4d..729f373782e2 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -548,7 +548,7 @@ static int init_ring_common(struct intel_engine_cs *engine)
>  	else
>  		intel_ring_setup_status_page(engine);
>  
> -	intel_engine_reset_irq(engine);
> +	intel_engine_reset_breadcrumbs(engine);
>  
>  	/* Enforce ordering by reading HEAD register back */
>  	I915_READ_HEAD(engine);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 34954ca03a4a..124f4646958d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -520,7 +520,6 @@ int __intel_ring_space(int head, int tail, int size);
>  void intel_ring_update_space(struct intel_ring *ring);
>  
>  void intel_engine_init_seqno(struct intel_engine_cs *engine, u32 seqno);
> -void intel_engine_reset_irq(struct intel_engine_cs *engine);
>  
>  void intel_engine_setup_common(struct intel_engine_cs *engine);
>  int intel_engine_init_common(struct intel_engine_cs *engine);
> @@ -614,6 +613,7 @@ static inline bool intel_engine_wakeup(const struct intel_engine_cs *engine)
>  	return wakeup;
>  }
>  
> +void intel_engine_reset_breadcrumbs(struct intel_engine_cs *engine);
>  void intel_engine_fini_breadcrumbs(struct intel_engine_cs *engine);
>  unsigned int intel_kick_waiters(struct drm_i915_private *i915);
>  unsigned int intel_kick_signalers(struct drm_i915_private *i915);
> -- 
> 2.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx