[Intel-gfx] [PATCH v2] drm/i915: Added Wa_18022495364

[Bhadane, Dnyaneshwar]

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper at intel.com>
> Sent: Saturday, September 9, 2023 1:13 AM
> To: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; De Marchi, Lucas
> <lucas.demarchi at intel.com>; Garg, Nemesa <nemesa.garg at intel.com>;
> Atwood, Matthew S <matthew.s.atwood at intel.com>
> Subject: Re: [PATCH v2] drm/i915: Added Wa_18022495364
> 
> On Fri, Sep 08, 2023 at 12:39:18PM -0700, Matt Roper wrote:
> > On Fri, Sep 08, 2023 at 03:11:42PM +0530, Dnyaneshwar Bhadane wrote:
> > > This workaround has two different implementations, one for gen 12 to
> > > DG2 and another for DG2 and later.
> > > BSpec: 11354, 56551
> >
> > Side note:  it's generally not worth adding bspec references for
> > simple register pages these days.  Anyone who has internal bspec
> > access can look up the page just as easily using the register offset
> > or name as they can using this number, so this doesn't help anyone.
> > And in this case it looks like the page numbers you gave are wrong for
> > the platforms this workaround is supposed to apply to:  11354 is for
> > the pre-gen12 version of the register and 56551 is for the Xe2 version
> > of the instruction.
> >
> > Bspec references are still good when there are narrative pages
> > describing general software flows, because those can often be
> > difficult to locate in the large table of contents.
> >
> > >
> > > v2:
> > > - Removed extra parentheses from the condition (Lucas)
> > > - Fixed spacing and new line (Lucas)
> > >
> > > Signed-off-by: Dnyaneshwar Bhadane
> <dnyaneshwar.bhadane at intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > > Reviewed-by: Garg, Nemesa <nemesa.garg at intel.com>
> 
> One more minor thing:  Always use "First Last" instead of "Last, First"
> notation for names in r-b, s-o-b, etc.; otherwise the commas get
> misinterpreted when git parses these for email and it causes problems when
> sending/replying on the mailing list.
> 
> 
> Matt
> 
> > > ---
> > >  drivers/gpu/drm/i915/gt/gen8_engine_cs.c    | 5 +++++
> > >  drivers/gpu/drm/i915/gt/intel_gt_regs.h     | 2 ++
> > >  drivers/gpu/drm/i915/gt/intel_workarounds.c | 4 ++++
> > >  3 files changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > > b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > > index 0143445dba83..e260defdfc23 100644
> > > --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > > +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > > @@ -271,6 +271,11 @@ int gen12_emit_flush_rcs(struct i915_request
> *rq, u32 mode)
> > >  		if (GRAPHICS_VER_FULL(rq->i915) >= IP_VER(12, 70))
> > >  			bit_group_0 |= PIPE_CONTROL_CCS_FLUSH;
> > >
> > > +		/* Wa_18022495364 */
> > > +		if (GRAPHICS_VER_FULL(rq->i915) >= IP_VER(12, 70) ||
> > > +		    IS_DG2(rq->i915))
> >
> > This is going to apply the workaround not only to DG2, but also to
> > *all* platforms MTL and later forever.  Generally we should never have
> > workarounds marked this way...the expectation is that any hardware
> > workaround is going to go away eventually, and workaround conditions
> > in the code should only match the specific platforms listed as being
> > applicable in the workaround database.
> >
> > Also, when I look in the workaround database, it doesn't appear that
> > this workaround is listed as applying to DG2, MTL (Xe_LPG), or LNL
> > (Xe2_LPG).  Is there some other workaround that requires the same
> > programming (but has a different workaround lineage #)?  If not, then
> > this part of the patch should just go away and only the gen12lp change
> > below should remain..

Thank you for correcting me. This is not applicable to DG2 and newer platforms. 
Therefore, I should remove this part from the patch and only keeping the 
pre-Gen12 portion for this workaround. 

Dnyaneshwar
> >
> >
> > Matt
> >
> > > +			bit_group_1 |=
> PIPE_CONTROL_CONST_CACHE_INVALIDATE;
> > > +
> > >  		bit_group_1 |= PIPE_CONTROL_TILE_CACHE_FLUSH;
> > >  		bit_group_1 |= PIPE_CONTROL_FLUSH_L3;
> > >  		bit_group_1 |=
> PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH;
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > index 0e4c638fcbbf..4c0cb3a3d0aa 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > @@ -164,6 +164,8 @@
> > >  #define GEN9_CSFE_CHICKEN1_RCS
> 	_MMIO(0x20d4)
> > >  #define   GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE	(1 << 2)
> > >  #define   GEN11_ENABLE_32_PLANE_MODE		(1 << 7)
> > > +#define GEN12_CS_DEBUG_MODE2
> 	_MMIO(0x20d8)
> > > +#define   GEN12_GLOBAL_DEBUG_ENABLE			BIT(6)
> > >
> > >  #define GEN7_FF_SLICE_CS_CHICKEN1		_MMIO(0x20e0)
> > >  #define   GEN9_FFSC_PERCTX_PREEMPT_CTRL		(1 << 14)
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > index 864d41bcf6bb..efdb4bbf8083 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > @@ -712,6 +712,10 @@ static void gen12_ctx_workarounds_init(struct
> intel_engine_cs *engine,
> > >  			    GEN9_PREEMPT_GPGPU_LEVEL_MASK,
> > >  			    GEN9_PREEMPT_GPGPU_THREAD_GROUP_LEVEL);
> > >
> > > +	/* Wa_18022495364 */
> > > +	wa_masked_en(wal, GEN12_CS_DEBUG_MODE2,
> > > +		     GEN12_GLOBAL_DEBUG_ENABLE);
> > > +
> > >  	/*
> > >  	 * Wa_16011163337 - GS_TIMER
> > >  	 *
> > > --
> > > 2.34.1
> > >
> >
> > --
> > Matt Roper
> > Graphics Software Engineer
> > Linux GPU Platform Enablement
> > Intel Corporation
> 
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation