Re: [Intel-gfx] [PATCH 01/40] drm/i915/hangcheck: Replace hangcheck.seqno with RING_HEAD
Quoting Mika Kuoppala (2019-05-08 15:00:11) > Chris Wilson writes: > > > Quoting Mika Kuoppala (2019-05-08 13:30:46) > >> Chris Wilson writes: > >> > >> > After realising we need to sample RING_START to detect context switches > >> > from preemption events that do not allow for the seqno to advance, we > >> > can also realise that the seqno itself is just a distance along the ring > >> > and so can be replaced by sampling RING_HEAD. > >> > > >> > Signed-off-by: Chris Wilson > >> > Cc: Mika Kuoppala > >> > --- > >> > static enum intel_engine_hangcheck_action > >> > @@ -156,7 +156,7 @@ hangcheck_get_action(struct intel_engine_cs *engine, > >> > if (engine->hangcheck.last_ring != hc->ring) > >> > return ENGINE_ACTIVE_SEQNO; > >> > > >> > - if (engine->hangcheck.last_seqno != hc->seqno) > >> > + if (engine->hangcheck.last_head != hc->head) > >> > return ENGINE_ACTIVE_SEQNO; > >> > >> Change the enum also? > > > > Pffifle. As far as hangcheck goes RING_START:RING_HEAD comprise its > > seqno. > > > > Makes for a good talking point in a few years time :) > > Fair enough. > > > > >> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c > >> > b/drivers/gpu/drm/i915/gt/intel_lrc.c > >> > index d1a54d2c3d5d..f1d62746e066 100644 > >> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > >> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > >> > @@ -2275,12 +2275,6 @@ static u32 *gen8_emit_fini_breadcrumb(struct > >> > i915_request *request, u32 *cs) > >> > request->timeline->hwsp_offset, > >> > 0); > >> > > >> > - cs = gen8_emit_ggtt_write(cs, > >> > - > >> > intel_engine_next_hangcheck_seqno(request->engine), > >> > - I915_GEM_HWS_HANGCHECK_ADDR, > >> > - MI_FLUSH_DW_STORE_INDEX); > >> > - > >> > - > >> > *cs++ = MI_USER_INTERRUPT; > >> > *cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE; > >> > > >> > @@ -2297,14 +2291,11 @@ static u32 *gen8_emit_fini_breadcrumb_rcs(struct > >> > i915_request *request, u32 *cs) > >> > request->timeline->hwsp_offset, > >> > > >> > PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH | > >> > PIPE_CONTROL_DEPTH_CACHE_FLUSH | > >> > - PIPE_CONTROL_DC_FLUSH_ENABLE | > >> > - PIPE_CONTROL_FLUSH_ENABLE | > >> > - PIPE_CONTROL_CS_STALL); > >> > >> ??? > > > > Kabylake sends the interrupt too early otherwise. The hangcheck write > > saved us by pure accident. > > I read the diff wrong at first try also, was concerned that we lost cs stall. > Regardless this could benefit from a comment explaining the need to > force sync for the intr. Indeed, that is sensible since it's an empirical result and worth validating again later. Done and pushed, thanks. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/40] drm/i915/hangcheck: Replace hangcheck.seqno with RING_HEAD
Chris Wilson writes: > Quoting Mika Kuoppala (2019-05-08 13:30:46) >> Chris Wilson writes: >> >> > After realising we need to sample RING_START to detect context switches >> > from preemption events that do not allow for the seqno to advance, we >> > can also realise that the seqno itself is just a distance along the ring >> > and so can be replaced by sampling RING_HEAD. >> > >> > Signed-off-by: Chris Wilson >> > Cc: Mika Kuoppala >> > --- >> > static enum intel_engine_hangcheck_action >> > @@ -156,7 +156,7 @@ hangcheck_get_action(struct intel_engine_cs *engine, >> > if (engine->hangcheck.last_ring != hc->ring) >> > return ENGINE_ACTIVE_SEQNO; >> > >> > - if (engine->hangcheck.last_seqno != hc->seqno) >> > + if (engine->hangcheck.last_head != hc->head) >> > return ENGINE_ACTIVE_SEQNO; >> >> Change the enum also? > > Pffifle. As far as hangcheck goes RING_START:RING_HEAD comprise its > seqno. > > Makes for a good talking point in a few years time :) Fair enough. > >> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c >> > b/drivers/gpu/drm/i915/gt/intel_lrc.c >> > index d1a54d2c3d5d..f1d62746e066 100644 >> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c >> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c >> > @@ -2275,12 +2275,6 @@ static u32 *gen8_emit_fini_breadcrumb(struct >> > i915_request *request, u32 *cs) >> > request->timeline->hwsp_offset, >> > 0); >> > >> > - cs = gen8_emit_ggtt_write(cs, >> > - >> > intel_engine_next_hangcheck_seqno(request->engine), >> > - I915_GEM_HWS_HANGCHECK_ADDR, >> > - MI_FLUSH_DW_STORE_INDEX); >> > - >> > - >> > *cs++ = MI_USER_INTERRUPT; >> > *cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE; >> > >> > @@ -2297,14 +2291,11 @@ static u32 *gen8_emit_fini_breadcrumb_rcs(struct >> > i915_request *request, u32 *cs) >> > request->timeline->hwsp_offset, >> > PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH >> > | >> > PIPE_CONTROL_DEPTH_CACHE_FLUSH | >> > - PIPE_CONTROL_DC_FLUSH_ENABLE | >> > - PIPE_CONTROL_FLUSH_ENABLE | >> > - PIPE_CONTROL_CS_STALL); >> >> ??? > > Kabylake sends the interrupt too early otherwise. The hangcheck write > saved us by pure accident. I read the diff wrong at first try also, was concerned that we lost cs stall. Regardless this could benefit from a comment explaining the need to force sync for the intr. Was happy to see measure_breadcrumb_dw() paying off. Also leaning to it for alignment forcing. Reviewed-by: Mika Kuoppala ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/40] drm/i915/hangcheck: Replace hangcheck.seqno with RING_HEAD
Quoting Mika Kuoppala (2019-05-08 13:30:46) > Chris Wilson writes: > > > After realising we need to sample RING_START to detect context switches > > from preemption events that do not allow for the seqno to advance, we > > can also realise that the seqno itself is just a distance along the ring > > and so can be replaced by sampling RING_HEAD. > > > > Signed-off-by: Chris Wilson > > Cc: Mika Kuoppala > > --- > > static enum intel_engine_hangcheck_action > > @@ -156,7 +156,7 @@ hangcheck_get_action(struct intel_engine_cs *engine, > > if (engine->hangcheck.last_ring != hc->ring) > > return ENGINE_ACTIVE_SEQNO; > > > > - if (engine->hangcheck.last_seqno != hc->seqno) > > + if (engine->hangcheck.last_head != hc->head) > > return ENGINE_ACTIVE_SEQNO; > > Change the enum also? Pffifle. As far as hangcheck goes RING_START:RING_HEAD comprise its seqno. Makes for a good talking point in a few years time :) > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c > > b/drivers/gpu/drm/i915/gt/intel_lrc.c > > index d1a54d2c3d5d..f1d62746e066 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > > @@ -2275,12 +2275,6 @@ static u32 *gen8_emit_fini_breadcrumb(struct > > i915_request *request, u32 *cs) > > request->timeline->hwsp_offset, > > 0); > > > > - cs = gen8_emit_ggtt_write(cs, > > - > > intel_engine_next_hangcheck_seqno(request->engine), > > - I915_GEM_HWS_HANGCHECK_ADDR, > > - MI_FLUSH_DW_STORE_INDEX); > > - > > - > > *cs++ = MI_USER_INTERRUPT; > > *cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE; > > > > @@ -2297,14 +2291,11 @@ static u32 *gen8_emit_fini_breadcrumb_rcs(struct > > i915_request *request, u32 *cs) > > request->timeline->hwsp_offset, > > PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH | > > PIPE_CONTROL_DEPTH_CACHE_FLUSH | > > - PIPE_CONTROL_DC_FLUSH_ENABLE | > > - PIPE_CONTROL_FLUSH_ENABLE | > > - PIPE_CONTROL_CS_STALL); > > ??? Kabylake sends the interrupt too early otherwise. The hangcheck write saved us by pure accident. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/40] drm/i915/hangcheck: Replace hangcheck.seqno with RING_HEAD
Chris Wilson writes:
> After realising we need to sample RING_START to detect context switches
> from preemption events that do not allow for the seqno to advance, we
> can also realise that the seqno itself is just a distance along the ring
> and so can be replaced by sampling RING_HEAD.
>
> Signed-off-by: Chris Wilson
> Cc: Mika Kuoppala
> ---
> drivers/gpu/drm/i915/gt/intel_engine.h | 15 -
> drivers/gpu/drm/i915/gt/intel_engine_cs.c| 5 ++-
> drivers/gpu/drm/i915/gt/intel_engine_types.h | 3 +-
> drivers/gpu/drm/i915/gt/intel_hangcheck.c| 8 ++---
> drivers/gpu/drm/i915/gt/intel_lrc.c | 19 +++-
> drivers/gpu/drm/i915/gt/intel_ringbuffer.c | 32 ++--
> drivers/gpu/drm/i915/i915_debugfs.c | 12 ++--
> 7 files changed, 17 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h
> b/drivers/gpu/drm/i915/gt/intel_engine.h
> index 06d785533502..9359b3a7ad9c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -215,8 +215,6 @@ intel_write_status_page(struct intel_engine_cs *engine,
> int reg, u32 value)
> */
> #define I915_GEM_HWS_PREEMPT 0x32
> #define I915_GEM_HWS_PREEMPT_ADDR(I915_GEM_HWS_PREEMPT * sizeof(u32))
> -#define I915_GEM_HWS_HANGCHECK 0x34
> -#define I915_GEM_HWS_HANGCHECK_ADDR (I915_GEM_HWS_HANGCHECK * sizeof(u32))
> #define I915_GEM_HWS_SEQNO 0x40
> #define I915_GEM_HWS_SEQNO_ADDR (I915_GEM_HWS_SEQNO *
> sizeof(u32))
> #define I915_GEM_HWS_SCRATCH 0x80
> @@ -548,17 +546,4 @@ static inline bool inject_preempt_hang(struct
> intel_engine_execlists *execlists)
>
> #endif
>
> -static inline u32
> -intel_engine_next_hangcheck_seqno(struct intel_engine_cs *engine)
> -{
> - return engine->hangcheck.next_seqno =
> - next_pseudo_random32(engine->hangcheck.next_seqno);
> -}
> -
> -static inline u32
> -intel_engine_get_hangcheck_seqno(struct intel_engine_cs *engine)
> -{
> - return intel_read_status_page(engine, I915_GEM_HWS_HANGCHECK);
> -}
> -
> #endif /* _INTEL_RINGBUFFER_H_ */
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 416d7e2e6f8c..4c3753c1b573 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -721,6 +721,7 @@ static int measure_breadcrumb_dw(struct intel_engine_cs
> *engine)
> goto out_timeline;
>
> dw = engine->emit_fini_breadcrumb(>rq, frame->cs) - frame->cs;
> + GEM_BUG_ON(dw & 1); /* RING_TAIL must be qword aligned */
>
> i915_timeline_unpin(>timeline);
>
> @@ -1444,9 +1445,7 @@ void intel_engine_dump(struct intel_engine_cs *engine,
> drm_printf(m, "*** WEDGED ***\n");
>
> drm_printf(m, "\tAwake? %d\n", atomic_read(>wakeref.count));
> - drm_printf(m, "\tHangcheck %x:%x [%d ms]\n",
> -engine->hangcheck.last_seqno,
> -engine->hangcheck.next_seqno,
> + drm_printf(m, "\tHangcheck: %d ms ago\n",
> jiffies_to_msecs(jiffies -
> engine->hangcheck.action_timestamp));
> drm_printf(m, "\tReset count: %d (global %d)\n",
> i915_reset_engine_count(error, engine),
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index c0ab11b12e14..e381c1c73902 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -54,8 +54,7 @@ struct intel_instdone {
> struct intel_engine_hangcheck {
> u64 acthd;
> u32 last_ring;
> - u32 last_seqno;
> - u32 next_seqno;
> + u32 last_head;
> unsigned long action_timestamp;
> struct intel_instdone instdone;
> };
> diff --git a/drivers/gpu/drm/i915/gt/intel_hangcheck.c
> b/drivers/gpu/drm/i915/gt/intel_hangcheck.c
> index 721ab74a382f..3a4d09b80fa0 100644
> --- a/drivers/gpu/drm/i915/gt/intel_hangcheck.c
> +++ b/drivers/gpu/drm/i915/gt/intel_hangcheck.c
> @@ -28,7 +28,7 @@
> struct hangcheck {
> u64 acthd;
> u32 ring;
> - u32 seqno;
> + u32 head;
> enum intel_engine_hangcheck_action action;
> unsigned long action_timestamp;
> int deadlock;
> @@ -134,16 +134,16 @@ static void hangcheck_load_sample(struct
> intel_engine_cs *engine,
> struct hangcheck *hc)
> {
> hc->acthd = intel_engine_get_active_head(engine);
> - hc->seqno = intel_engine_get_hangcheck_seqno(engine);
> hc->ring = ENGINE_READ(engine, RING_START);
> + hc->head = ENGINE_READ(engine, RING_HEAD);
> }
>
> static void hangcheck_store_sample(struct intel_engine_cs *engine,
> const struct hangcheck *hc)
> {
> engine->hangcheck.acthd = hc->acthd;
> - engine->hangcheck.last_seqno = hc->seqno;
>
[Intel-gfx] [PATCH 01/40] drm/i915/hangcheck: Replace hangcheck.seqno with RING_HEAD
After realising we need to sample RING_START to detect context switches
from preemption events that do not allow for the seqno to advance, we
can also realise that the seqno itself is just a distance along the ring
and so can be replaced by sampling RING_HEAD.
Signed-off-by: Chris Wilson
Cc: Mika Kuoppala
---
drivers/gpu/drm/i915/gt/intel_engine.h | 15 -
drivers/gpu/drm/i915/gt/intel_engine_cs.c| 5 ++-
drivers/gpu/drm/i915/gt/intel_engine_types.h | 3 +-
drivers/gpu/drm/i915/gt/intel_hangcheck.c| 8 ++---
drivers/gpu/drm/i915/gt/intel_lrc.c | 19 +++-
drivers/gpu/drm/i915/gt/intel_ringbuffer.c | 32 ++--
drivers/gpu/drm/i915/i915_debugfs.c | 12 ++--
7 files changed, 17 insertions(+), 77 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h
b/drivers/gpu/drm/i915/gt/intel_engine.h
index 06d785533502..9359b3a7ad9c 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -215,8 +215,6 @@ intel_write_status_page(struct intel_engine_cs *engine, int
reg, u32 value)
*/
#define I915_GEM_HWS_PREEMPT 0x32
#define I915_GEM_HWS_PREEMPT_ADDR (I915_GEM_HWS_PREEMPT * sizeof(u32))
-#define I915_GEM_HWS_HANGCHECK 0x34
-#define I915_GEM_HWS_HANGCHECK_ADDR(I915_GEM_HWS_HANGCHECK * sizeof(u32))
#define I915_GEM_HWS_SEQNO 0x40
#define I915_GEM_HWS_SEQNO_ADDR(I915_GEM_HWS_SEQNO *
sizeof(u32))
#define I915_GEM_HWS_SCRATCH 0x80
@@ -548,17 +546,4 @@ static inline bool inject_preempt_hang(struct
intel_engine_execlists *execlists)
#endif
-static inline u32
-intel_engine_next_hangcheck_seqno(struct intel_engine_cs *engine)
-{
- return engine->hangcheck.next_seqno =
- next_pseudo_random32(engine->hangcheck.next_seqno);
-}
-
-static inline u32
-intel_engine_get_hangcheck_seqno(struct intel_engine_cs *engine)
-{
- return intel_read_status_page(engine, I915_GEM_HWS_HANGCHECK);
-}
-
#endif /* _INTEL_RINGBUFFER_H_ */
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 416d7e2e6f8c..4c3753c1b573 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -721,6 +721,7 @@ static int measure_breadcrumb_dw(struct intel_engine_cs
*engine)
goto out_timeline;
dw = engine->emit_fini_breadcrumb(>rq, frame->cs) - frame->cs;
+ GEM_BUG_ON(dw & 1); /* RING_TAIL must be qword aligned */
i915_timeline_unpin(>timeline);
@@ -1444,9 +1445,7 @@ void intel_engine_dump(struct intel_engine_cs *engine,
drm_printf(m, "*** WEDGED ***\n");
drm_printf(m, "\tAwake? %d\n", atomic_read(>wakeref.count));
- drm_printf(m, "\tHangcheck %x:%x [%d ms]\n",
- engine->hangcheck.last_seqno,
- engine->hangcheck.next_seqno,
+ drm_printf(m, "\tHangcheck: %d ms ago\n",
jiffies_to_msecs(jiffies -
engine->hangcheck.action_timestamp));
drm_printf(m, "\tReset count: %d (global %d)\n",
i915_reset_engine_count(error, engine),
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h
b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index c0ab11b12e14..e381c1c73902 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -54,8 +54,7 @@ struct intel_instdone {
struct intel_engine_hangcheck {
u64 acthd;
u32 last_ring;
- u32 last_seqno;
- u32 next_seqno;
+ u32 last_head;
unsigned long action_timestamp;
struct intel_instdone instdone;
};
diff --git a/drivers/gpu/drm/i915/gt/intel_hangcheck.c
b/drivers/gpu/drm/i915/gt/intel_hangcheck.c
index 721ab74a382f..3a4d09b80fa0 100644
--- a/drivers/gpu/drm/i915/gt/intel_hangcheck.c
+++ b/drivers/gpu/drm/i915/gt/intel_hangcheck.c
@@ -28,7 +28,7 @@
struct hangcheck {
u64 acthd;
u32 ring;
- u32 seqno;
+ u32 head;
enum intel_engine_hangcheck_action action;
unsigned long action_timestamp;
int deadlock;
@@ -134,16 +134,16 @@ static void hangcheck_load_sample(struct intel_engine_cs
*engine,
struct hangcheck *hc)
{
hc->acthd = intel_engine_get_active_head(engine);
- hc->seqno = intel_engine_get_hangcheck_seqno(engine);
hc->ring = ENGINE_READ(engine, RING_START);
+ hc->head = ENGINE_READ(engine, RING_HEAD);
}
static void hangcheck_store_sample(struct intel_engine_cs *engine,
const struct hangcheck *hc)
{
engine->hangcheck.acthd = hc->acthd;
- engine->hangcheck.last_seqno = hc->seqno;
engine->hangcheck.last_ring = hc->ring;
+ engine->hangcheck.last_head = hc->head;
}
static enum intel_engine_hangcheck_action
@@ -156,7 +156,7 @@ hangcheck_get_action(struct intel_engine_cs
