[Intel-xe] [PATCH V2] drm/xe/xe2: Add workaround 16020292621

[Upadhyay, Tejas]

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper at intel.com>
> Sent: Thursday, November 30, 2023 6:10 AM
> To: Upadhyay, Tejas <tejas.upadhyay at intel.com>
> Cc: intel-xe at lists.freedesktop.org
> Subject: Re: [Intel-xe] [PATCH V2] drm/xe/xe2: Add workaround 16020292621
> 
> On Tue, Nov 28, 2023 at 05:12:32PM +0530, Tejas Upadhyay wrote:
> > Workaround applies to Graphics 20.04 as part of ring submission
> >
> > V2:
> >   - Marking this WA for 20.04 instead of 20.00
> >
> > Signed-off-by: Tejas Upadhyay <tejas.upadhyay at intel.com>
> > ---
> >  drivers/gpu/drm/xe/regs/xe_gpu_commands.h |  1 +
> >  drivers/gpu/drm/xe/regs/xe_gt_regs.h      |  1 +
> >  drivers/gpu/drm/xe/xe_ring_ops.c          | 18 ++++++++++++++++++
> >  drivers/gpu/drm/xe/xe_wa_oob.rules        |  2 ++
> >  4 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xe/regs/xe_gpu_commands.h
> > b/drivers/gpu/drm/xe/regs/xe_gpu_commands.h
> > index 4402f72481dc..f1c5bf203b3d 100644
> > --- a/drivers/gpu/drm/xe/regs/xe_gpu_commands.h
> > +++ b/drivers/gpu/drm/xe/regs/xe_gpu_commands.h
> > @@ -48,6 +48,7 @@
> >  #define   PIPE_CONTROL_TILE_CACHE_FLUSH
> 	(1<<28)
> >  #define   PIPE_CONTROL_AMFS_FLUSH			(1<<25)
> >  #define   PIPE_CONTROL_GLOBAL_GTT_IVB			(1<<24)
> > +#define   PIPE_CONTROL_LRI_POST_SYNC			BIT(23)
> >  #define   PIPE_CONTROL_STORE_DATA_INDEX
> 	(1<<21)
> >  #define   PIPE_CONTROL_CS_STALL				(1<<20)
> >  #define   PIPE_CONTROL_GLOBAL_SNAPSHOT_RESET		(1<<19)
> > diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > index 18b13224480d..efc05762556e 100644
> > --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > @@ -60,6 +60,7 @@
> >  #define   MTL_MCR_GROUPID			REG_GENMASK(11, 8)
> >  #define   MTL_MCR_INSTANCEID			REG_GENMASK(3, 0)
> >
> > +#define RCS_NOPID				XE_REG(0x2094)
> 
> We already have RING_NOPID() as a parameterized macro; no need to add a
> one-off definition for the RCS-specific instance.

Ok 

> 
> >  #define FF_SLICE_CS_CHICKEN1			XE_REG(0x20e0,
> XE_REG_OPTION_MASKED)
> >  #define   FFSC_PERCTX_PREEMPT_CTRL		REG_BIT(14)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c
> > b/drivers/gpu/drm/xe/xe_ring_ops.c
> > index 59e0aa2d6a4c..ed161e4ac4b2 100644
> > --- a/drivers/gpu/drm/xe/xe_ring_ops.c
> > +++ b/drivers/gpu/drm/xe/xe_ring_ops.c
> > @@ -184,6 +184,22 @@ static int emit_render_cache_flush(struct
> xe_sched_job *job, u32 *dw, int i)
> >  	return i;
> >  }
> >
> > +static int emit_pipe_control(struct xe_sched_job *job, u32 *dw, int
> > +i)
> 
> It might be worth putting something about "end of ring" in the function name
> here to make it more obvious what kind of pipe control is being emitted here.

Ok 

> 
> > +{
> > +	struct xe_gt *gt = job->q->gt;
> 
> Wouldn't it make more sense to pass GT or hwe as a parameter rather than
> job?
> 
> This function is going to get called on both RCS and CCS engines, but we only
> need this workaround on RCS engines according to the workaround details.
> We probably want to bail out here if we're on a CCS engine.

Please see below 

> 
> 
> Matt
> 
> > +
> > +	if (XE_WA(gt, 16020292621)) {
> > +		dw[i++] = GFX_OP_PIPE_CONTROL(6);
> > +		dw[i++] = PIPE_CONTROL_LRI_POST_SYNC;
> > +		dw[i++] = RCS_NOPID.addr;
> > +		dw[i++] = 0;
> > +		dw[i++] = 0;
> > +		dw[i++] = 0;
> > +	}
> > +
> > +	return i;
> > +}
> > +
> >  static int emit_pipe_imm_ggtt(u32 addr, u32 value, bool stall_only, u32
> *dw,
> >  			      int i)
> >  {
> > @@ -342,6 +358,8 @@ static void
> __emit_job_gen12_render_compute(struct
> > xe_sched_job *job,
> >
> >  	i = emit_user_interrupt(dw, i);
> >
> > +	i = emit_pipe_control(job, dw, i);
> > +
> >  	xe_gt_assert(gt, i <= MAX_JOB_SIZE_DW);
> >
> >  	xe_lrc_write_ring(lrc, dw, i * sizeof(*dw)); diff --git
> > a/drivers/gpu/drm/xe/xe_wa_oob.rules
> > b/drivers/gpu/drm/xe/xe_wa_oob.rules
> > index 752842d734be..defae8259778 100644
> > --- a/drivers/gpu/drm/xe/xe_wa_oob.rules
> > +++ b/drivers/gpu/drm/xe/xe_wa_oob.rules
> > @@ -20,3 +20,5 @@
> >  16017236439	PLATFORM(PVC)
> >  22010954014	PLATFORM(DG2)
> >  14019821291	MEDIA_VERSION_RANGE(1300, 2000)
> > +16020292621	GRAPHICS_VERSION(2004), GRAPHICS_STEP(A0, B0)
> > +		ENGINE_CLASS(RENDER)

I have understanding that this provides filter of engine to XE_WA() call so it should be processed for RENDER only.

Tejas
> > --
> > 2.25.1
> >
> 
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation