Re: [PATCH] drm/i915/guc: Ensure multi-lrc fini breadcrumb math is correct

[John Harrison <john.c.harrison@xxxxxxxxx>]

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@xxxxxxxxx>
Looks good. There was a checkpatch warning about blank lines. With that 
fixed:
Reviewed-by: John Harrison <John.C.Harrison@xxxxxxxxx>

> ---
>   .../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,