| Message ID | 20220113055903.7607-1-matthew.brost@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/guc: Ensure multi-lrc fini breadcrumb math is correct | expand |
On 1/12/2022 21:59, Matthew Brost wrote: > Realized that the GuC multi-lrc fini breadcrumb emit code is very > delicate as the math this code does relies on functions it calls to emit > a certain number of DWs. Add a few GEM_BUG_ONs to assert the math is > correct. > > Signed-off-by: Matthew Brost <matthew.brost@intel.com> Looks good. There was a checkpatch warning about blank lines. With that fixed: Reviewed-by: John Harrison <John.C.Harrison@Intel.com> > --- > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 32 +++++++++++++++---- > 1 file changed, 26 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index 3ae92260f8224..d562d85f96871 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -4493,27 +4493,31 @@ static inline bool skip_handshake(struct i915_request *rq) > return test_bit(I915_FENCE_FLAG_SKIP_PARALLEL, &rq->fence.flags); > } > > +#define NON_SKIP_LEN 6 > static u32 * > emit_fini_breadcrumb_parent_no_preempt_mid_batch(struct i915_request *rq, > u32 *cs) > { > struct intel_context *ce = rq->context; > + __maybe_unused u32 *before_fini_breadcrumb_user_interrupt_cs; > + __maybe_unused u32 *start_fini_breadcrumb_cs = cs; > > GEM_BUG_ON(!intel_context_is_parent(ce)); > > if (unlikely(skip_handshake(rq))) { > /* > * NOP everything in __emit_fini_breadcrumb_parent_no_preempt_mid_batch, > - * the -6 comes from the length of the emits below. > + * the NON_SKIP_LEN comes from the length of the emits below. > */ > memset(cs, 0, sizeof(u32) * > - (ce->engine->emit_fini_breadcrumb_dw - 6)); > - cs += ce->engine->emit_fini_breadcrumb_dw - 6; > + (ce->engine->emit_fini_breadcrumb_dw - NON_SKIP_LEN)); > + cs += ce->engine->emit_fini_breadcrumb_dw - NON_SKIP_LEN; > } else { > cs = __emit_fini_breadcrumb_parent_no_preempt_mid_batch(rq, cs); > } > > /* Emit fini breadcrumb */ > + before_fini_breadcrumb_user_interrupt_cs = cs; > cs = gen8_emit_ggtt_write(cs, > rq->fence.seqno, > i915_request_active_timeline(rq)->hwsp_offset, > @@ -4523,6 +4527,12 @@ emit_fini_breadcrumb_parent_no_preempt_mid_batch(struct i915_request *rq, > *cs++ = MI_USER_INTERRUPT; > *cs++ = MI_NOOP; > > + /* Ensure our math for skip + emit is correct */ > + GEM_BUG_ON(before_fini_breadcrumb_user_interrupt_cs + NON_SKIP_LEN != > + cs); > + GEM_BUG_ON(start_fini_breadcrumb_cs + > + ce->engine->emit_fini_breadcrumb_dw != cs); > + > rq->tail = intel_ring_offset(rq, cs); > > return cs; > @@ -4565,22 +4575,25 @@ emit_fini_breadcrumb_child_no_preempt_mid_batch(struct i915_request *rq, > u32 *cs) > { > struct intel_context *ce = rq->context; > + __maybe_unused u32 *before_fini_breadcrumb_user_interrupt_cs; > + __maybe_unused u32 *start_fini_breadcrumb_cs = cs; > > GEM_BUG_ON(!intel_context_is_child(ce)); > > if (unlikely(skip_handshake(rq))) { > /* > * NOP everything in __emit_fini_breadcrumb_child_no_preempt_mid_batch, > - * the -6 comes from the length of the emits below. > + * the NON_SKIP_LEN comes from the length of the emits below. > */ > memset(cs, 0, sizeof(u32) * > - (ce->engine->emit_fini_breadcrumb_dw - 6)); > - cs += ce->engine->emit_fini_breadcrumb_dw - 6; > + (ce->engine->emit_fini_breadcrumb_dw - NON_SKIP_LEN)); > + cs += ce->engine->emit_fini_breadcrumb_dw - NON_SKIP_LEN; > } else { > cs = __emit_fini_breadcrumb_child_no_preempt_mid_batch(rq, cs); > } > > /* Emit fini breadcrumb */ > + before_fini_breadcrumb_user_interrupt_cs = cs; > cs = gen8_emit_ggtt_write(cs, > rq->fence.seqno, > i915_request_active_timeline(rq)->hwsp_offset, > @@ -4590,10 +4603,17 @@ emit_fini_breadcrumb_child_no_preempt_mid_batch(struct i915_request *rq, > *cs++ = MI_USER_INTERRUPT; > *cs++ = MI_NOOP; > > + /* Ensure our math for skip + emit is correct */ > + GEM_BUG_ON(before_fini_breadcrumb_user_interrupt_cs + NON_SKIP_LEN != > + cs); > + GEM_BUG_ON(start_fini_breadcrumb_cs + > + ce->engine->emit_fini_breadcrumb_dw != cs); > + > rq->tail = intel_ring_offset(rq, cs); > > return cs; > } > +#undef NON_SKIP_LEN > > static struct intel_context * > guc_create_virtual(struct intel_engine_cs **siblings, unsigned int count,
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index 3ae92260f8224..d562d85f96871 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -4493,27 +4493,31 @@ static inline bool skip_handshake(struct i915_request *rq) return test_bit(I915_FENCE_FLAG_SKIP_PARALLEL, &rq->fence.flags); } +#define NON_SKIP_LEN 6 static u32 * emit_fini_breadcrumb_parent_no_preempt_mid_batch(struct i915_request *rq, u32 *cs) { struct intel_context *ce = rq->context; + __maybe_unused u32 *before_fini_breadcrumb_user_interrupt_cs; + __maybe_unused u32 *start_fini_breadcrumb_cs = cs; GEM_BUG_ON(!intel_context_is_parent(ce)); if (unlikely(skip_handshake(rq))) { /* * NOP everything in __emit_fini_breadcrumb_parent_no_preempt_mid_batch, - * the -6 comes from the length of the emits below. + * the NON_SKIP_LEN comes from the length of the emits below. */ memset(cs, 0, sizeof(u32) * - (ce->engine->emit_fini_breadcrumb_dw - 6)); - cs += ce->engine->emit_fini_breadcrumb_dw - 6; + (ce->engine->emit_fini_breadcrumb_dw - NON_SKIP_LEN)); + cs += ce->engine->emit_fini_breadcrumb_dw - NON_SKIP_LEN; } else { cs = __emit_fini_breadcrumb_parent_no_preempt_mid_batch(rq, cs); } /* Emit fini breadcrumb */ + before_fini_breadcrumb_user_interrupt_cs = cs; cs = gen8_emit_ggtt_write(cs, rq->fence.seqno, i915_request_active_timeline(rq)->hwsp_offset, @@ -4523,6 +4527,12 @@ emit_fini_breadcrumb_parent_no_preempt_mid_batch(struct i915_request *rq, *cs++ = MI_USER_INTERRUPT; *cs++ = MI_NOOP; + /* Ensure our math for skip + emit is correct */ + GEM_BUG_ON(before_fini_breadcrumb_user_interrupt_cs + NON_SKIP_LEN != + cs); + GEM_BUG_ON(start_fini_breadcrumb_cs + + ce->engine->emit_fini_breadcrumb_dw != cs); + rq->tail = intel_ring_offset(rq, cs); return cs; @@ -4565,22 +4575,25 @@ emit_fini_breadcrumb_child_no_preempt_mid_batch(struct i915_request *rq, u32 *cs) { struct intel_context *ce = rq->context; + __maybe_unused u32 *before_fini_breadcrumb_user_interrupt_cs; + __maybe_unused u32 *start_fini_breadcrumb_cs = cs; GEM_BUG_ON(!intel_context_is_child(ce)); if (unlikely(skip_handshake(rq))) { /* * NOP everything in __emit_fini_breadcrumb_child_no_preempt_mid_batch, - * the -6 comes from the length of the emits below. + * the NON_SKIP_LEN comes from the length of the emits below. */ memset(cs, 0, sizeof(u32) * - (ce->engine->emit_fini_breadcrumb_dw - 6)); - cs += ce->engine->emit_fini_breadcrumb_dw - 6; + (ce->engine->emit_fini_breadcrumb_dw - NON_SKIP_LEN)); + cs += ce->engine->emit_fini_breadcrumb_dw - NON_SKIP_LEN; } else { cs = __emit_fini_breadcrumb_child_no_preempt_mid_batch(rq, cs); } /* Emit fini breadcrumb */ + before_fini_breadcrumb_user_interrupt_cs = cs; cs = gen8_emit_ggtt_write(cs, rq->fence.seqno, i915_request_active_timeline(rq)->hwsp_offset, @@ -4590,10 +4603,17 @@ emit_fini_breadcrumb_child_no_preempt_mid_batch(struct i915_request *rq, *cs++ = MI_USER_INTERRUPT; *cs++ = MI_NOOP; + /* Ensure our math for skip + emit is correct */ + GEM_BUG_ON(before_fini_breadcrumb_user_interrupt_cs + NON_SKIP_LEN != + cs); + GEM_BUG_ON(start_fini_breadcrumb_cs + + ce->engine->emit_fini_breadcrumb_dw != cs); + rq->tail = intel_ring_offset(rq, cs); return cs; } +#undef NON_SKIP_LEN static struct intel_context * guc_create_virtual(struct intel_engine_cs **siblings, unsigned int count,
Realized that the GuC multi-lrc fini breadcrumb emit code is very delicate as the math this code does relies on functions it calls to emit a certain number of DWs. Add a few GEM_BUG_ONs to assert the math is correct. Signed-off-by: Matthew Brost <matthew.brost@intel.com> --- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 32 +++++++++++++++---- 1 file changed, 26 insertions(+), 6 deletions(-)