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

[Upadhyay, Tejas]

> -----Original Message-----
> From: Andi Shyti <andi.shyti at linux.intel.com>
> Sent: Wednesday, May 31, 2023 2:36 AM
> To: Upadhyay, Tejas <tejas.upadhyay at intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH V2] drm/i915/gt: Add workaround
> 14016712196
> 
> 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 at intel.com>
> > Closes:
> > https://lore.kernel.org/oe-kbuild-all/202305121525.3EWdGoBY-lkp@intel.
> > com/
> > Signed-off-by: Tejas Upadhyay <tejas.upadhyay at 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);
> 
I agree initially I had same, but problem I have is kernel bot is complaining for uninitialized "cs" passed to mtl_dummy_pipe_control. I did not get simple solution to it. Suggestions!
 
> Besides, you check the err value from intel_ring_begin inside
> mtl_dummy_pipe_control(), which looks a bit weird to me.

Yes it looks weird. I will change it.

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

As we have conditional ring increments, we cant decide ring size at start in ring begin for whole bunch at once. Though double checking.

Thanks,
Tejas

> 
> Andi