Re: [Intel-gfx] [PATCH V2] drm/i915/gt: Add workaround 14016712196

[Andi Shyti <andi.shyti@linux.intel.com>]

Hi Tejas,

On Wed, May 17, 2023 at 06:52:30PM +0530, Tejas Upadhyay wrote:
> Wa_14016712196 implementation for mtl
> 
> Bspec: 72197
> 
> V2:
>   - Fix  kernel test robot warnings
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202305121525.3EWdGoBY-lkp@intel.com/
> Signed-off-by: Tejas Upadhyay <tejas.upadhyay@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 41 ++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> index e1c76e5bfa82..737eb515544b 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> @@ -177,14 +177,38 @@ u32 *gen12_emit_aux_table_inv(struct intel_gt *gt, u32 *cs, const i915_reg_t inv
>  	return cs;
>  }
>  
> +static int mtl_dummy_pipe_control(struct i915_request *rq, u32 *cs)
> +{
> +	if (IS_ERR(cs))
> +		return PTR_ERR(cs);
> +	cs = gen12_emit_pipe_control(cs,
> +				     0,
> +				     PIPE_CONTROL_DEPTH_CACHE_FLUSH,
> +				     LRC_PPHWSP_SCRATCH_ADDR);
> +	intel_ring_advance(rq, cs);
> +
> +	return 0;
> +}
> +
>  int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
>  {
>  	struct intel_engine_cs *engine = rq->engine;
> +	int err;
>  
>  	if (mode & EMIT_FLUSH) {
>  		u32 flags = 0;
>  		u32 *cs;
>  
> +		/* Wa_14016712196 */
> +		if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, STEP_B0) ||
> +		    IS_MTL_GRAPHICS_STEP(engine->i915, P, STEP_A0, STEP_B0)) {
> +			/* dummy PIPE_CONTROL + depth flush */
> +			cs = intel_ring_begin(rq, 6);
> +			err = mtl_dummy_pipe_control(rq, cs);
> +			if (err)
> +				return err;
> +		}
> +
>  		flags |= PIPE_CONTROL_TILE_CACHE_FLUSH;
>  		flags |= PIPE_CONTROL_FLUSH_L3;
>  		flags |= PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH;
> @@ -218,6 +242,16 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
>  		u32 flags = 0;
>  		u32 *cs, count;
>  
> +		/* Wa_14016712196 */
> +		if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, STEP_B0) ||
> +		    IS_MTL_GRAPHICS_STEP(engine->i915, P, STEP_A0, STEP_B0)) {
> +			/* dummy PIPE_CONTROL + depth flush */
> +			cs = intel_ring_begin(rq, 6);
> +			err = mtl_dummy_pipe_control(rq, cs);
> +			if (err)
> +				return err;
> +		}
> +

as the two blocks are basically identical, why not put both of
them inside mtl_dummy_pipe_control() and you would basically have
something like:

		/* Wa_14016712196 */
		cs = mtl_dummy_pipe_control(rq, cs);
		if (IS_ERR(cs))
			return PTR_ERR(cs);

Besides, you check the err value from intel_ring_begin inside
mtl_dummy_pipe_control(), which looks a bit weird to me.

I'm wondering, though, if we can put both dummy pipe and real
pipe in the same command and advance the ring only once at the
end... nevermind.

Andi