[Intel-gfx] [PATCH 01/40] drm/i915/hangcheck: Replace hangcheck.seqno with RING_HEAD

[Mika Kuoppala]

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

> Quoting Mika Kuoppala (2019-05-08 13:30:46)
>> Chris Wilson <chris at chris-wilson.co.uk> 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 <chris at chris-wilson.co.uk>
>> > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
>> > ---
>> >  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 <mika.kuoppala at linux.intel.com>