| Message ID | 20230113011947.9631-1-gustavo.sousa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gt: Move LSC_CHICKEN_BIT* workarounds to correct function | expand |
On Thu, Jan 12, 2023 at 10:19:47PM -0300, Gustavo Sousa wrote: > That register doesn't belong to a specific engine, so the proper > placement for workarounds programming it should be > general_render_compute_wa_init(). Looking to the surrounds it seems like we have more registers that are not per engine specific being touched there. So, a few questions came to my mind: - do we need to a bigger clean up and move other cases as well? - do we need to review one by one and see if the bug is really for all the engines or affect one specific engine hence the function was initially placed here? - do we have any clean documentation on how to split things around and when or where to place things here or there? > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_workarounds.c | 65 ++++++++++++--------- > 1 file changed, 36 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c > index 6dacd0dc5c2c..bd40b8c93d24 100644 > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > @@ -2325,10 +2325,6 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal) > /* Wa_1509727124 */ > wa_mcr_masked_en(wal, GEN10_SAMPLER_MODE, > SC_DISABLE_POWER_OPTIMIZATION_EBB); > - > - /* Wa_22013037850 */ > - wa_mcr_write_or(wal, LSC_CHICKEN_BIT_0_UDW, > - DISABLE_128B_EVICTION_COMMAND_UDW); > } > > if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_B0, STEP_FOREVER) || > @@ -2357,21 +2353,6 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal) > GEN12_DISABLE_HDR_PAST_PAYLOAD_HOLD_FIX); > } > > - if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_B0, STEP_C0) || > - IS_DG2_G11(i915)) { > - /* > - * Wa_22012826095:dg2 > - * Wa_22013059131:dg2 > - */ > - wa_mcr_write_clr_set(wal, LSC_CHICKEN_BIT_0_UDW, > - MAXREQS_PER_BANK, > - REG_FIELD_PREP(MAXREQS_PER_BANK, 2)); > - > - /* Wa_22013059131:dg2 */ > - wa_mcr_write_or(wal, LSC_CHICKEN_BIT_0, > - FORCE_1_SUB_MESSAGE_PER_FRAGMENT); > - } > - > /* Wa_1308578152:dg2_g10 when first gslice is fused off */ > if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_B0, STEP_C0) && > needs_wa_1308578152(engine)) { > @@ -2396,16 +2377,6 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal) > */ > wa_mcr_masked_en(wal, GEN8_ROW_CHICKEN, > MDQ_ARBITRATION_MODE | UGM_BACKUP_MODE); > - > - /* > - * Wa_14010918519:dg2_g10 > - * > - * LSC_CHICKEN_BIT_0 always reads back as 0 is this stepping, > - * so ignoring verification. > - */ > - wa_mcr_add(wal, LSC_CHICKEN_BIT_0_UDW, 0, > - FORCE_SLM_FENCE_SCOPE_TO_TILE | FORCE_UGM_FENCE_SCOPE_TO_TILE, > - 0, false); > } > > if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_A0, STEP_B0)) { > @@ -2990,6 +2961,15 @@ general_render_compute_wa_init(struct intel_engine_cs *engine, struct i915_wa_li > > add_render_compute_tuning_settings(i915, wal); > > + if (IS_MTL_GRAPHICS_STEP(i915, M, STEP_A0, STEP_B0) || > + IS_MTL_GRAPHICS_STEP(i915, P, STEP_A0, STEP_B0) || > + IS_DG2_GRAPHICS_STEP(i915, G10, STEP_B0, STEP_FOREVER) || > + IS_DG2_G11(i915) || IS_DG2_G12(i915)) { > + /* Wa_22013037850 */ > + wa_mcr_write_or(wal, LSC_CHICKEN_BIT_0_UDW, > + DISABLE_128B_EVICTION_COMMAND_UDW); > + } > + > if (IS_MTL_GRAPHICS_STEP(i915, M, STEP_A0, STEP_B0) || > IS_MTL_GRAPHICS_STEP(i915, P, STEP_A0, STEP_B0) || > IS_PONTEVECCHIO(i915) || > @@ -3011,6 +2991,33 @@ general_render_compute_wa_init(struct intel_engine_cs *engine, struct i915_wa_li > wa_masked_en(wal, VFG_PREEMPTION_CHICKEN, POLYGON_TRIFAN_LINELOOP_DISABLE); > } > > + if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_B0, STEP_C0) || > + IS_DG2_G11(i915)) { > + /* > + * Wa_22012826095:dg2 > + * Wa_22013059131:dg2 > + */ > + wa_mcr_write_clr_set(wal, LSC_CHICKEN_BIT_0_UDW, > + MAXREQS_PER_BANK, > + REG_FIELD_PREP(MAXREQS_PER_BANK, 2)); > + > + /* Wa_22013059131:dg2 */ > + wa_mcr_write_or(wal, LSC_CHICKEN_BIT_0, > + FORCE_1_SUB_MESSAGE_PER_FRAGMENT); > + } > + > + if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_A0, STEP_B0)) { > + /* > + * Wa_14010918519:dg2_g10 > + * > + * LSC_CHICKEN_BIT_0 always reads back as 0 is this stepping, > + * so ignoring verification. > + */ > + wa_mcr_add(wal, LSC_CHICKEN_BIT_0_UDW, 0, > + FORCE_SLM_FENCE_SCOPE_TO_TILE | FORCE_UGM_FENCE_SCOPE_TO_TILE, > + 0, false); > + } > + > if (IS_PONTEVECCHIO(i915)) { > /* Wa_16016694945 */ > wa_masked_en(wal, XEHPC_LNCFMISCCFGREG0, XEHPC_OVRLSCCC); > -- > 2.39.0 >
On Tue, Jan 17, 2023 at 01:06:17PM -0500, Rodrigo Vivi wrote: > On Thu, Jan 12, 2023 at 10:19:47PM -0300, Gustavo Sousa wrote: > > That register doesn't belong to a specific engine, so the proper > > placement for workarounds programming it should be > > general_render_compute_wa_init(). > > > Looking to the surrounds it seems like we have more registers > that are not per engine specific being touched there. > > So, a few questions came to my mind: > - do we need to a bigger clean up and move other cases as well? I think so. I just happens that I was already working on a workaround for that register and realized the inconsistency. > - do we need to review one by one and see if the bug is really > for all the engines or affect one specific engine hence the > function was initially placed here? As far as I know, the general_render_compute_wa_init() is actually applied for a single engine and is for registers that are not tied to a specific engine but share the common render/compute reset domain. I'm including Matt Roper here, in case I'm missing something. > - do we have any clean documentation on how to split things > around and when or where to place things here or there? I have sent a v2 with an additional patch extending our existing documentation to make it clear where workarounds should be implemented according to their classification: https://patchwork.freedesktop.org/series/113036/ -- Gustavo Sousa > > > > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> > > --- > > drivers/gpu/drm/i915/gt/intel_workarounds.c | 65 ++++++++++++--------- > > 1 file changed, 36 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > index 6dacd0dc5c2c..bd40b8c93d24 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > > @@ -2325,10 +2325,6 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal) > > /* Wa_1509727124 */ > > wa_mcr_masked_en(wal, GEN10_SAMPLER_MODE, > > SC_DISABLE_POWER_OPTIMIZATION_EBB); > > - > > - /* Wa_22013037850 */ > > - wa_mcr_write_or(wal, LSC_CHICKEN_BIT_0_UDW, > > - DISABLE_128B_EVICTION_COMMAND_UDW); > > } > > > > if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_B0, STEP_FOREVER) || > > @@ -2357,21 +2353,6 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal) > > GEN12_DISABLE_HDR_PAST_PAYLOAD_HOLD_FIX); > > } > > > > - if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_B0, STEP_C0) || > > - IS_DG2_G11(i915)) { > > - /* > > - * Wa_22012826095:dg2 > > - * Wa_22013059131:dg2 > > - */ > > - wa_mcr_write_clr_set(wal, LSC_CHICKEN_BIT_0_UDW, > > - MAXREQS_PER_BANK, > > - REG_FIELD_PREP(MAXREQS_PER_BANK, 2)); > > - > > - /* Wa_22013059131:dg2 */ > > - wa_mcr_write_or(wal, LSC_CHICKEN_BIT_0, > > - FORCE_1_SUB_MESSAGE_PER_FRAGMENT); > > - } > > - > > /* Wa_1308578152:dg2_g10 when first gslice is fused off */ > > if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_B0, STEP_C0) && > > needs_wa_1308578152(engine)) { > > @@ -2396,16 +2377,6 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal) > > */ > > wa_mcr_masked_en(wal, GEN8_ROW_CHICKEN, > > MDQ_ARBITRATION_MODE | UGM_BACKUP_MODE); > > - > > - /* > > - * Wa_14010918519:dg2_g10 > > - * > > - * LSC_CHICKEN_BIT_0 always reads back as 0 is this stepping, > > - * so ignoring verification. > > - */ > > - wa_mcr_add(wal, LSC_CHICKEN_BIT_0_UDW, 0, > > - FORCE_SLM_FENCE_SCOPE_TO_TILE | FORCE_UGM_FENCE_SCOPE_TO_TILE, > > - 0, false); > > } > > > > if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_A0, STEP_B0)) { > > @@ -2990,6 +2961,15 @@ general_render_compute_wa_init(struct intel_engine_cs *engine, struct i915_wa_li > > > > add_render_compute_tuning_settings(i915, wal); > > > > + if (IS_MTL_GRAPHICS_STEP(i915, M, STEP_A0, STEP_B0) || > > + IS_MTL_GRAPHICS_STEP(i915, P, STEP_A0, STEP_B0) || > > + IS_DG2_GRAPHICS_STEP(i915, G10, STEP_B0, STEP_FOREVER) || > > + IS_DG2_G11(i915) || IS_DG2_G12(i915)) { > > + /* Wa_22013037850 */ > > + wa_mcr_write_or(wal, LSC_CHICKEN_BIT_0_UDW, > > + DISABLE_128B_EVICTION_COMMAND_UDW); > > + } > > + > > if (IS_MTL_GRAPHICS_STEP(i915, M, STEP_A0, STEP_B0) || > > IS_MTL_GRAPHICS_STEP(i915, P, STEP_A0, STEP_B0) || > > IS_PONTEVECCHIO(i915) || > > @@ -3011,6 +2991,33 @@ general_render_compute_wa_init(struct intel_engine_cs *engine, struct i915_wa_li > > wa_masked_en(wal, VFG_PREEMPTION_CHICKEN, POLYGON_TRIFAN_LINELOOP_DISABLE); > > } > > > > + if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_B0, STEP_C0) || > > + IS_DG2_G11(i915)) { > > + /* > > + * Wa_22012826095:dg2 > > + * Wa_22013059131:dg2 > > + */ > > + wa_mcr_write_clr_set(wal, LSC_CHICKEN_BIT_0_UDW, > > + MAXREQS_PER_BANK, > > + REG_FIELD_PREP(MAXREQS_PER_BANK, 2)); > > + > > + /* Wa_22013059131:dg2 */ > > + wa_mcr_write_or(wal, LSC_CHICKEN_BIT_0, > > + FORCE_1_SUB_MESSAGE_PER_FRAGMENT); > > + } > > + > > + if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_A0, STEP_B0)) { > > + /* > > + * Wa_14010918519:dg2_g10 > > + * > > + * LSC_CHICKEN_BIT_0 always reads back as 0 is this stepping, > > + * so ignoring verification. > > + */ > > + wa_mcr_add(wal, LSC_CHICKEN_BIT_0_UDW, 0, > > + FORCE_SLM_FENCE_SCOPE_TO_TILE | FORCE_UGM_FENCE_SCOPE_TO_TILE, > > + 0, false); > > + } > > + > > if (IS_PONTEVECCHIO(i915)) { > > /* Wa_16016694945 */ > > wa_masked_en(wal, XEHPC_LNCFMISCCFGREG0, XEHPC_OVRLSCCC); > > -- > > 2.39.0 > >
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c index 6dacd0dc5c2c..bd40b8c93d24 100644 --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c @@ -2325,10 +2325,6 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal) /* Wa_1509727124 */ wa_mcr_masked_en(wal, GEN10_SAMPLER_MODE, SC_DISABLE_POWER_OPTIMIZATION_EBB); - - /* Wa_22013037850 */ - wa_mcr_write_or(wal, LSC_CHICKEN_BIT_0_UDW, - DISABLE_128B_EVICTION_COMMAND_UDW); } if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_B0, STEP_FOREVER) || @@ -2357,21 +2353,6 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal) GEN12_DISABLE_HDR_PAST_PAYLOAD_HOLD_FIX); } - if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_B0, STEP_C0) || - IS_DG2_G11(i915)) { - /* - * Wa_22012826095:dg2 - * Wa_22013059131:dg2 - */ - wa_mcr_write_clr_set(wal, LSC_CHICKEN_BIT_0_UDW, - MAXREQS_PER_BANK, - REG_FIELD_PREP(MAXREQS_PER_BANK, 2)); - - /* Wa_22013059131:dg2 */ - wa_mcr_write_or(wal, LSC_CHICKEN_BIT_0, - FORCE_1_SUB_MESSAGE_PER_FRAGMENT); - } - /* Wa_1308578152:dg2_g10 when first gslice is fused off */ if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_B0, STEP_C0) && needs_wa_1308578152(engine)) { @@ -2396,16 +2377,6 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal) */ wa_mcr_masked_en(wal, GEN8_ROW_CHICKEN, MDQ_ARBITRATION_MODE | UGM_BACKUP_MODE); - - /* - * Wa_14010918519:dg2_g10 - * - * LSC_CHICKEN_BIT_0 always reads back as 0 is this stepping, - * so ignoring verification. - */ - wa_mcr_add(wal, LSC_CHICKEN_BIT_0_UDW, 0, - FORCE_SLM_FENCE_SCOPE_TO_TILE | FORCE_UGM_FENCE_SCOPE_TO_TILE, - 0, false); } if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_A0, STEP_B0)) { @@ -2990,6 +2961,15 @@ general_render_compute_wa_init(struct intel_engine_cs *engine, struct i915_wa_li add_render_compute_tuning_settings(i915, wal); + if (IS_MTL_GRAPHICS_STEP(i915, M, STEP_A0, STEP_B0) || + IS_MTL_GRAPHICS_STEP(i915, P, STEP_A0, STEP_B0) || + IS_DG2_GRAPHICS_STEP(i915, G10, STEP_B0, STEP_FOREVER) || + IS_DG2_G11(i915) || IS_DG2_G12(i915)) { + /* Wa_22013037850 */ + wa_mcr_write_or(wal, LSC_CHICKEN_BIT_0_UDW, + DISABLE_128B_EVICTION_COMMAND_UDW); + } + if (IS_MTL_GRAPHICS_STEP(i915, M, STEP_A0, STEP_B0) || IS_MTL_GRAPHICS_STEP(i915, P, STEP_A0, STEP_B0) || IS_PONTEVECCHIO(i915) || @@ -3011,6 +2991,33 @@ general_render_compute_wa_init(struct intel_engine_cs *engine, struct i915_wa_li wa_masked_en(wal, VFG_PREEMPTION_CHICKEN, POLYGON_TRIFAN_LINELOOP_DISABLE); } + if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_B0, STEP_C0) || + IS_DG2_G11(i915)) { + /* + * Wa_22012826095:dg2 + * Wa_22013059131:dg2 + */ + wa_mcr_write_clr_set(wal, LSC_CHICKEN_BIT_0_UDW, + MAXREQS_PER_BANK, + REG_FIELD_PREP(MAXREQS_PER_BANK, 2)); + + /* Wa_22013059131:dg2 */ + wa_mcr_write_or(wal, LSC_CHICKEN_BIT_0, + FORCE_1_SUB_MESSAGE_PER_FRAGMENT); + } + + if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_A0, STEP_B0)) { + /* + * Wa_14010918519:dg2_g10 + * + * LSC_CHICKEN_BIT_0 always reads back as 0 is this stepping, + * so ignoring verification. + */ + wa_mcr_add(wal, LSC_CHICKEN_BIT_0_UDW, 0, + FORCE_SLM_FENCE_SCOPE_TO_TILE | FORCE_UGM_FENCE_SCOPE_TO_TILE, + 0, false); + } + if (IS_PONTEVECCHIO(i915)) { /* Wa_16016694945 */ wa_masked_en(wal, XEHPC_LNCFMISCCFGREG0, XEHPC_OVRLSCCC);
That register doesn't belong to a specific engine, so the proper placement for workarounds programming it should be general_render_compute_wa_init(). Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> --- drivers/gpu/drm/i915/gt/intel_workarounds.c | 65 ++++++++++++--------- 1 file changed, 36 insertions(+), 29 deletions(-)