[PATCH] drm/i915/execlists: Clear STOP_RING bit before restoring the context

[PATCH] drm/i915/execlists: Clear STOP_RING bit before restoring the context

[Chris Wilson]

Before a reset, we set the STOP_RING bit of RING_MI_MODE to freeze the
engine. However, sometimes we observe that upon restart, the engine
freezes again with the STOP_RING bit still asserted. By inspection, we
know that the register itself is cleared by the GPU reset, so that bit
must be preserved inside the context image and reloaded from there. A
simple fix (as the RING_MI_MODE is at a fixed offset in a valid context)
is to clobber the STOP_RING bit inside the image before replaying the
request.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c     | 17 +++++++++++++++--
 drivers/gpu/drm/i915/intel_lrc_reg.h |  2 ++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 3f90c74038ef..37fe842de639 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1918,6 +1918,20 @@ static void execlists_reset(struct intel_engine_cs *engine,
 
 	spin_unlock_irqrestore(&engine->timeline.lock, flags);
 
+	if (!request)
+		return;
+
+	regs = request->hw_context->lrc_reg_state;
+
+	/*
+	 * After a reset, the context may have preserved the STOP bit
+	 * of RING_MI_MODE we used to freeze the active engine before
+	 * the reset. If that bit is restored the ring stops instead
+	 * of being executed.
+	 */
+	regs[CTX_MI_MODE + 1] |= STOP_RING << 16;
+	regs[CTX_MI_MODE + 1] &= ~STOP_RING;
+
 	/*
 	 * If the request was innocent, we leave the request in the ELSP
 	 * and will try to replay it on restarting. The context image may
@@ -1929,7 +1943,7 @@ static void execlists_reset(struct intel_engine_cs *engine,
 	 * and have to at least restore the RING register in the context
 	 * image back to the expected values to skip over the guilty request.
 	 */
-	if (!request || request->fence.error != -EIO)
+	if (request->fence.error != -EIO)
 		return;
 
 	/*
@@ -1940,7 +1954,6 @@ static void execlists_reset(struct intel_engine_cs *engine,
 	 * future request will be after userspace has had the opportunity
 	 * to recreate its own state.
 	 */
-	regs = request->hw_context->lrc_reg_state;
 	if (engine->pinned_default_state) {
 		memcpy(regs, /* skip restoring the vanilla PPHWSP */
 		       engine->pinned_default_state + LRC_STATE_PN * PAGE_SIZE,
diff --git a/drivers/gpu/drm/i915/intel_lrc_reg.h b/drivers/gpu/drm/i915/intel_lrc_reg.h
index 5ef932d810a7..3b155ecbfa92 100644
--- a/drivers/gpu/drm/i915/intel_lrc_reg.h
+++ b/drivers/gpu/drm/i915/intel_lrc_reg.h
@@ -39,6 +39,8 @@
 #define CTX_R_PWR_CLK_STATE		0x42
 #define CTX_END				0x44
 
+#define CTX_MI_MODE			0x54
+
 #define CTX_REG(reg_state, pos, reg, val) do { \
 	u32 *reg_state__ = (reg_state); \
 	const u32 pos__ = (pos); \
-- 
2.18.0

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

Re: [PATCH] drm/i915/execlists: Clear STOP_RING bit before restoring the context

[Mika Kuoppala]

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

> Before a reset, we set the STOP_RING bit of RING_MI_MODE to freeze the
> engine. However, sometimes we observe that upon restart, the engine
> freezes again with the STOP_RING bit still asserted. By inspection, we
> know that the register itself is cleared by the GPU reset, so that bit
> must be preserved inside the context image and reloaded from there. A
> simple fix (as the RING_MI_MODE is at a fixed offset in a valid context)
> is to clobber the STOP_RING bit inside the image before replaying the
> request.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c     | 17 +++++++++++++++--
>  drivers/gpu/drm/i915/intel_lrc_reg.h |  2 ++
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 3f90c74038ef..37fe842de639 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1918,6 +1918,20 @@ static void execlists_reset(struct intel_engine_cs *engine,
>  
>  	spin_unlock_irqrestore(&engine->timeline.lock, flags);
>  
> +	if (!request)
> +		return;
> +
> +	regs = request->hw_context->lrc_reg_state;
> +
> +	/*
> +	 * After a reset, the context may have preserved the STOP bit
> +	 * of RING_MI_MODE we used to freeze the active engine before
> +	 * the reset. If that bit is restored the ring stops instead
> +	 * of being executed.
> +	 */
> +	regs[CTX_MI_MODE + 1] |= STOP_RING << 16;
> +	regs[CTX_MI_MODE + 1] &= ~STOP_RING;

Ok, I did go and pondered if this is truely the simplest
way. Forcing the start outside a context would not
be simpler and would be slower.

Nice find and it will has potential to explain our some troubles
of re-enabling engines.

Did you find out this by looking at the error states or what?

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

> +
>  	/*
>  	 * If the request was innocent, we leave the request in the ELSP
>  	 * and will try to replay it on restarting. The context image may
> @@ -1929,7 +1943,7 @@ static void execlists_reset(struct intel_engine_cs *engine,
>  	 * and have to at least restore the RING register in the context
>  	 * image back to the expected values to skip over the guilty request.
>  	 */
> -	if (!request || request->fence.error != -EIO)
> +	if (request->fence.error != -EIO)
>  		return;
>  
>  	/*
> @@ -1940,7 +1954,6 @@ static void execlists_reset(struct intel_engine_cs *engine,
>  	 * future request will be after userspace has had the opportunity
>  	 * to recreate its own state.
>  	 */
> -	regs = request->hw_context->lrc_reg_state;
>  	if (engine->pinned_default_state) {
>  		memcpy(regs, /* skip restoring the vanilla PPHWSP */
>  		       engine->pinned_default_state + LRC_STATE_PN * PAGE_SIZE,
> diff --git a/drivers/gpu/drm/i915/intel_lrc_reg.h b/drivers/gpu/drm/i915/intel_lrc_reg.h
> index 5ef932d810a7..3b155ecbfa92 100644
> --- a/drivers/gpu/drm/i915/intel_lrc_reg.h
> +++ b/drivers/gpu/drm/i915/intel_lrc_reg.h
> @@ -39,6 +39,8 @@
>  #define CTX_R_PWR_CLK_STATE		0x42
>  #define CTX_END				0x44
>  
> +#define CTX_MI_MODE			0x54
> +
>  #define CTX_REG(reg_state, pos, reg, val) do { \
>  	u32 *reg_state__ = (reg_state); \
>  	const u32 pos__ = (pos); \
> -- 
> 2.18.0
>
> _______________________________________________
> 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/execlists: Clear STOP_RING bit before restoring the context

[Chris Wilson]

Quoting Chris Wilson (2018-08-14 14:11:50)
> Before a reset, we set the STOP_RING bit of RING_MI_MODE to freeze the
> engine. However, sometimes we observe that upon restart, the engine
> freezes again with the STOP_RING bit still asserted. By inspection, we
> know that the register itself is cleared by the GPU reset, so that bit
> must be preserved inside the context image and reloaded from there. A
> simple fix (as the RING_MI_MODE is at a fixed offset in a valid context)
> is to clobber the STOP_RING bit inside the image before replaying the
> request.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>

Note that we need to improve the active request tracking before this is
100% reliable. One patch at a time; though you can look at the
i915_reset.c restructuring series to see what I have in mind.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/execlists: Clear STOP_RING bit before restoring the context

[Chris Wilson]

Quoting Mika Kuoppala (2018-08-14 14:42:41)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Before a reset, we set the STOP_RING bit of RING_MI_MODE to freeze the
> > engine. However, sometimes we observe that upon restart, the engine
> > freezes again with the STOP_RING bit still asserted. By inspection, we
> > know that the register itself is cleared by the GPU reset, so that bit
> > must be preserved inside the context image and reloaded from there. A
> > simple fix (as the RING_MI_MODE is at a fixed offset in a valid context)
> > is to clobber the STOP_RING bit inside the image before replaying the
> > request.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> > Cc: Michał Winiarski <michal.winiarski@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_lrc.c     | 17 +++++++++++++++--
> >  drivers/gpu/drm/i915/intel_lrc_reg.h |  2 ++
> >  2 files changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 3f90c74038ef..37fe842de639 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -1918,6 +1918,20 @@ static void execlists_reset(struct intel_engine_cs *engine,
> >  
> >       spin_unlock_irqrestore(&engine->timeline.lock, flags);
> >  
> > +     if (!request)
> > +             return;
> > +
> > +     regs = request->hw_context->lrc_reg_state;
> > +
> > +     /*
> > +      * After a reset, the context may have preserved the STOP bit
> > +      * of RING_MI_MODE we used to freeze the active engine before
> > +      * the reset. If that bit is restored the ring stops instead
> > +      * of being executed.
> > +      */
> > +     regs[CTX_MI_MODE + 1] |= STOP_RING << 16;
> > +     regs[CTX_MI_MODE + 1] &= ~STOP_RING;
> 
> Ok, I did go and pondered if this is truely the simplest
> way. Forcing the start outside a context would not
> be simpler and would be slower.
> 
> Nice find and it will has potential to explain our some troubles
> of re-enabling engines.
> 
> Did you find out this by looking at the error states or what?

We're dumping sufficient info between the set-wedged and GEM_TRACE to
track the MI_MODE bits; e.g.

commit 9a4dc80399b1630cea0f1ad8ef0417436cbb95d0
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Fri May 18 11:09:33 2018 +0100

    drm/i915: Flush the ring stop bit after clearing RING_HEAD in reset

    Inside the live_hangcheck (reset) selftests, we occasionally see
    failures like

    <7>[  239.094840] i915_gem_set_wedged rcs0
    <7>[  239.094843] i915_gem_set_wedged   current seqno 19a98, last 19a9a, hangcheck 0 [5158 ms]
    <7>[  239.094846] i915_gem_set_wedged   Reset count: 6239 (global 1)
    <7>[  239.094848] i915_gem_set_wedged   Requests:
    <7>[  239.095052] i915_gem_set_wedged           first  19a99 [e8c:5f] prio=1024 @ 5159ms: (null)
    <7>[  239.095056] i915_gem_set_wedged           last   19a9a [e81:1a] prio=139 @ 5159ms: igt/rcs0[5977]/1
    <7>[  239.095059] i915_gem_set_wedged           active 19a99 [e8c:5f] prio=1024 @ 5159ms: (null)
    <7>[  239.095062] i915_gem_set_wedged           [head 0220, postfix 0280, tail 02a8, batch 0xffffffff_ffffffff]
    <7>[  239.100050] i915_gem_set_wedged           ring->start:  0x00283000
    <7>[  239.100053] i915_gem_set_wedged           ring->head:   0x000001f8
    <7>[  239.100055] i915_gem_set_wedged           ring->tail:   0x000002a8
    <7>[  239.100057] i915_gem_set_wedged           ring->emit:   0x000002a8
    <7>[  239.100059] i915_gem_set_wedged           ring->space:  0x00000f10
    <7>[  239.100085] i915_gem_set_wedged   RING_START: 0x00283000
    <7>[  239.100088] i915_gem_set_wedged   RING_HEAD:  0x00000260
    <7>[  239.100091] i915_gem_set_wedged   RING_TAIL:  0x000002a8
    <7>[  239.100094] i915_gem_set_wedged   RING_CTL:   0x00000001
    <7>[  239.100097] i915_gem_set_wedged   RING_MODE:  0x00000300 [idle]
    <7>[  239.100100] i915_gem_set_wedged   RING_IMR: fffffefe
    <7>[  239.100104] i915_gem_set_wedged   ACTHD:  0x00000000_0000609c
    <7>[  239.100108] i915_gem_set_wedged   BBADDR: 0x00000000_0000609d
    <7>[  239.100111] i915_gem_set_wedged   DMA_FADDR: 0x00000000_00283260
    <7>[  239.100114] i915_gem_set_wedged   IPEIR: 0x00000000
    <7>[  239.100117] i915_gem_set_wedged   IPEHR: 0x02800000
    <7>[  239.100120] i915_gem_set_wedged   Execlist status: 0x00044052 00000002
    <7>[  239.100124] i915_gem_set_wedged   Execlist CSB read 5 [5 cached], write 5 [5 from hws], interrupt posted? no, tasklet queued? no (enabled)
    <7>[  239.100128] i915_gem_set_wedged           ELSP[0] count=1, ring->start=00283000, rq: 19a99 [e8c:5f] prio=1024 @ 5164ms: (null)
    <7>[  239.100132] i915_gem_set_wedged           ELSP[1] count=1, ring->start=00257000, rq: 19a9a [e81:1a] prio=139 @ 5164ms: igt/rcs0[5977]/1
    <7>[  239.100135] i915_gem_set_wedged           HW active? 0x5
    <7>[  239.100250] i915_gem_set_wedged           E 19a99 [e8c:5f] prio=1024 @ 5164ms: (null)
    <7>[  239.100338] i915_gem_set_wedged           E 19a9a [e81:1a] prio=139 @ 5164ms: igt/rcs0[5977]/1
    <7>[  239.100340] i915_gem_set_wedged           Queue priority: 139
    <7>[  239.100343] i915_gem_set_wedged           Q 0 [e98:19] prio=132 @ 5164ms: igt/rcs0[5977]/8
    <7>[  239.100346] i915_gem_set_wedged           Q 0 [e84:19] prio=121 @ 5165ms: igt/rcs0[5977]/2
    <7>[  239.100349] i915_gem_set_wedged           Q 0 [e87:19] prio=82 @ 5165ms: igt/rcs0[5977]/3
    <7>[  239.100352] i915_gem_set_wedged           Q 0 [e84:1a] prio=44 @ 5164ms: igt/rcs0[5977]/2
    <7>[  239.100356] i915_gem_set_wedged           Q 0 [e8b:19] prio=20 @ 5165ms: igt/rcs0[5977]/4
    <7>[  239.100362] i915_gem_set_wedged   drv_selftest [5894] waiting for 19a99

    where the GPU saw an arbitration point and idles; AND HAS NOT BEEN RESET!
    The RING_MODE indicates that is idle and has the STOP_RING bit set, so
    try clearing it.

    v2: Only clear the bit on restarting the ring, as we want to be sure the
    STOP_RING bit is kept if reset fails on wedging.
    v3: Spot when the ring state doesn't make sense when re-initialising the
    engine and dump it to the logs so that we don't have to wait for an
    error later and try to guess what happened earlier.
    v4: Prepare to print all the unexpected state, not just the first.

and that patch proves that the register state across reset is fine; ergo
context image it is.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/execlists: Clear STOP_RING bit before restoring the context

[Patchwork]

== Series Details ==

Series: drm/i915/execlists: Clear STOP_RING bit before restoring the context
URL   : https://patchwork.freedesktop.org/series/48187/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4665 -> Patchwork_9938 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/48187/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9938 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_hangcheck:
      fi-kbl-7567u:       PASS -> DMESG-FAIL (fdo#106947, fdo#106560)

    igt@kms_frontbuffer_tracking@basic:
      {fi-byt-clapper}:   PASS -> FAIL (fdo#103167)

    
    ==== Possible fixes ====

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    igt@drv_selftest@live_hangcheck:
      fi-skl-guc:         DMESG-FAIL (fdo#107174) -> PASS

    igt@drv_selftest@live_workarounds:
      fi-cnl-psr:         DMESG-FAIL (fdo#107292) -> PASS
      fi-skl-6700k2:      DMESG-FAIL (fdo#107292) -> PASS

    igt@kms_chamelium@dp-crc-fast:
      fi-kbl-7500u:       DMESG-FAIL (fdo#103841) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

    
    ==== Warnings ====

    {igt@kms_psr@primary_page_flip}:
      fi-cnl-psr:         DMESG-FAIL (fdo#107372) -> DMESG-WARN (fdo#107372)

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292
  fdo#107372 https://bugs.freedesktop.org/show_bug.cgi?id=107372


== Participating hosts (54 -> 49) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4665 -> Patchwork_9938

  CI_DRM_4665: 3a43bf5efa33be9f88d70427dbfba4db4377eaf6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4594: b0263e5d0563a81a42cf66e7d3b84662d3222862 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9938: 3155a2f9cb8cd0b001fbaae9ab67afa10051d1d8 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

3155a2f9cb8c drm/i915/execlists: Clear STOP_RING bit before restoring the context

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9938/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915/execlists: Clear STOP_RING bit before restoring the context

[Patchwork]

== Series Details ==

Series: drm/i915/execlists: Clear STOP_RING bit before restoring the context
URL   : https://patchwork.freedesktop.org/series/48187/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4665_full -> Patchwork_9938_full =

== Summary - SUCCESS ==

  No regressions found.

  

== Known issues ==

  Here are the changes found in Patchwork_9938_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_workarounds@suspend-resume:
      shard-glk:          NOTRUN -> FAIL (fdo#103375)

    
    ==== Possible fixes ====

    igt@drv_suspend@shrink:
      shard-glk:          INCOMPLETE (k.org#198133, fdo#103359, fdo#106886) -> PASS

    igt@gem_workarounds@suspend-resume:
      shard-apl:          FAIL (fdo#103375) -> PASS

    igt@kms_setmode@basic:
      shard-kbl:          FAIL (fdo#99912) -> PASS

    
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4665 -> Patchwork_9938

  CI_DRM_4665: 3a43bf5efa33be9f88d70427dbfba4db4377eaf6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4594: b0263e5d0563a81a42cf66e7d3b84662d3222862 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9938: 3155a2f9cb8cd0b001fbaae9ab67afa10051d1d8 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9938/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx