Re: [PATCH] drm/i915/execlists: Verify context register state before execution

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

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

> Check that the context's ring register state still matches our
> expectations prior to execution.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_lrc.c     | 73 ++++++++++++++++++++-----
>  drivers/gpu/drm/i915/gt/intel_lrc_reg.h |  4 +-
>  drivers/gpu/drm/i915/gt/selftest_lrc.c  |  4 +-
>  3 files changed, 63 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 5f61cd128d9c..666e70931524 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1146,6 +1146,60 @@ execlists_schedule_out(struct i915_request *rq)
>  	i915_request_put(rq);
>  }
>  
> +static int lrc_ring_mi_mode(const struct intel_engine_cs *engine)
> +{
> +	if (INTEL_GEN(engine->i915) >= 12)
> +		return 0x60;
> +	else if (INTEL_GEN(engine->i915) >= 9)
> +		return 0x54;
> +	else if (engine->class == RENDER_CLASS)
> +		return 0x58;
> +	else
> +		return -1;
> +}
> +
> +static void
> +execlists_check_context(const struct intel_context *ce,
> +			const struct intel_engine_cs *engine)
> +{
> +	const struct intel_ring *ring = ce->ring;
> +	u32 *regs = ce->lrc_reg_state;
> +	int x;
> +
> +	if (regs[CTX_RING_START] != i915_ggtt_offset(ring->vma)) {
> +		pr_err_once("%s: context submitted with incorrect RING_BUFFER_START [%08x], expected %08x\n",
> +			    engine->name,
> +			    regs[CTX_RING_START],
> +			    i915_ggtt_offset(ring->vma));
> +		regs[CTX_RING_START] = i915_ggtt_offset(ring->vma);
> +	}
> +
> +	if ((regs[CTX_RING_CTL] & ~(RING_WAIT | RING_WAIT_SEMAPHORE)) !=
> +	    (RING_CTL_SIZE(ring->size) | RING_VALID)) {
> +		pr_err_once("%s: context submitted with incorrect RING_BUFFER_CONTROL [%08x], expected %08x\n",
> +			    engine->name,
> +			    regs[CTX_RING_CTL],
> +			    (u32)(RING_CTL_SIZE(ring->size) | RING_VALID));
> +		regs[CTX_RING_CTL] = RING_CTL_SIZE(ring->size) | RING_VALID;

We are going to submit by clearing the waits. First I thought this will
lead to disaster but the hardware works so that we clear on writing '1'.


> +	}
> +
> +	if (regs[CTX_BB_STATE] != RING_BB_PPGTT) {
> +		pr_err_once("%s: context submitted with incorrect BB_STATE [%08x], expected %08x\n",
> +			    engine->name,
> +			    regs[CTX_BB_STATE],
> +			    RING_BB_PPGTT);
> +		regs[CTX_BB_STATE] = RING_BB_PPGTT;
> +	}
> +
> +	x = lrc_ring_mi_mode(engine);
> +	if (x != -1 && regs[x + 1] & STOP_RING) {
> +		pr_err_once("%s: context submitted with STOP_RING [%08x] in RING_MI_MODE\n",
> +			    engine->name, regs[x + 1]);
> +		regs[x + 1] &= ~STOP_RING;
> +		regs[x + 1] |= STOP_RING << 16;

Ok you want only to care about this one. Was pondering of restoring rest
of the state from previous.

Will the hardware even care on masks at this point or is the saving part
setting them accordingly?

Not affecting this patch tho, just curious.

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

> +	}
> +}
> +
>  static u64 execlists_update_context(const struct i915_request *rq)
>  {
>  	struct intel_context *ce = rq->context;
> @@ -1154,6 +1208,9 @@ static u64 execlists_update_context(const struct i915_request *rq)
>  	ce->lrc_reg_state[CTX_RING_TAIL] =
>  		intel_ring_set_tail(rq->ring, rq->tail);
>  
> +	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> +		execlists_check_context(ce, rq->engine);
> +
>  	/*
>  	 * Make sure the context image is complete before we submit it to HW.
>  	 *
> @@ -2355,7 +2412,7 @@ __execlists_update_reg_state(const struct intel_context *ce,
>  	GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->head));
>  	GEM_BUG_ON(!intel_ring_offset_valid(ring, ring->tail));
>  
> -	regs[CTX_RING_BUFFER_START] = i915_ggtt_offset(ring->vma);
> +	regs[CTX_RING_START] = i915_ggtt_offset(ring->vma);
>  	regs[CTX_RING_HEAD] = ring->head;
>  	regs[CTX_RING_TAIL] = ring->tail;
>  
> @@ -2942,18 +2999,6 @@ static void reset_csb_pointers(struct intel_engine_cs *engine)
>  			       &execlists->csb_status[reset_value]);
>  }
>  
> -static int lrc_ring_mi_mode(const struct intel_engine_cs *engine)
> -{
> -	if (INTEL_GEN(engine->i915) >= 12)
> -		return 0x60;
> -	else if (INTEL_GEN(engine->i915) >= 9)
> -		return 0x54;
> -	else if (engine->class == RENDER_CLASS)
> -		return 0x58;
> -	else
> -		return -1;
> -}
> -
>  static void __execlists_reset_reg_state(const struct intel_context *ce,
>  					const struct intel_engine_cs *engine)
>  {
> @@ -3881,7 +3926,7 @@ static void init_common_reg_state(u32 * const regs,
>  			_MASKED_BIT_DISABLE(CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT |
>  					    CTX_CTRL_RS_CTX_ENABLE);
>  
> -	regs[CTX_RING_BUFFER_CONTROL] = RING_CTL_SIZE(ring->size) | RING_VALID;
> +	regs[CTX_RING_CTL] = RING_CTL_SIZE(ring->size) | RING_VALID;
>  	regs[CTX_BB_STATE] = RING_BB_PPGTT;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> index 06ab0276e10e..08a3be65f700 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc_reg.h
> @@ -13,8 +13,8 @@
>  #define CTX_CONTEXT_CONTROL		(0x02 + 1)
>  #define CTX_RING_HEAD			(0x04 + 1)
>  #define CTX_RING_TAIL			(0x06 + 1)
> -#define CTX_RING_BUFFER_START		(0x08 + 1)
> -#define CTX_RING_BUFFER_CONTROL		(0x0a + 1)
> +#define CTX_RING_START			(0x08 + 1)
> +#define CTX_RING_CTL			(0x0a + 1)
>  #define CTX_BB_STATE			(0x10 + 1)
>  #define CTX_BB_PER_CTX_PTR		(0x18 + 1)
>  #define CTX_PDP3_UDW			(0x24 + 1)
> diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> index 04d0dd6a938a..7d3c7ca811b3 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> @@ -3178,12 +3178,12 @@ static int live_lrc_fixed(void *arg)
>  		} tbl[] = {
>  			{
>  				i915_mmio_reg_offset(RING_START(engine->mmio_base)),
> -				CTX_RING_BUFFER_START - 1,
> +				CTX_RING_START - 1,
>  				"RING_START"
>  			},
>  			{
>  				i915_mmio_reg_offset(RING_CTL(engine->mmio_base)),
> -				CTX_RING_BUFFER_CONTROL - 1,
> +				CTX_RING_CTL - 1,
>  				"RING_CTL"
>  			},
>  			{
> -- 
> 2.24.0.rc1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx