| Message ID | 20230908094142.4119379-1-dnyaneshwar.bhadane@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915: Added Wa_18022495364 | expand |
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@intel.com> > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Reviewed-by: Garg, Nemesa <nemesa.garg@intel.com> > --- > 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.. 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 >
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@intel.com> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > Reviewed-by: Garg, Nemesa <nemesa.garg@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.. > > > 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
> -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@intel.com> > Sent: Saturday, September 9, 2023 1:13 AM > To: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com> > Cc: intel-gfx@lists.freedesktop.org; De Marchi, Lucas > <lucas.demarchi@intel.com>; Garg, Nemesa <nemesa.garg@intel.com>; > Atwood, Matthew S <matthew.s.atwood@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@intel.com> > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > > Reviewed-by: Garg, Nemesa <nemesa.garg@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
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)) + 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 *