Re: [PATCH v2 2/2] drm/i915/gt: Move LSC_CHICKEN_BIT* workarounds to correct function

Lucas De Marchi <lucas.demarchi@xxxxxxxxx> · Fri, 20 Jan 2023 14:46:05 -0800

On Thu, Jan 19, 2023 at 03:03:30PM -0800, Matt Roper wrote:
On Thu, Jan 19, 2023 at 02:28:13PM -0800, Vivi, Rodrigo wrote:
On Thu, 2023-01-19 at 19:24 -0300, Gustavo Sousa wrote:
> On Thu, Jan 19, 2023 at 04:57:09PM -0500, Rodrigo Vivi wrote:
> > On Wed, Jan 18, 2023 at 12:52:49PM -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().
> > >
> > > Signed-off-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx>
> >
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>
> Thanks for the review, Rodrigo!
>
> Last time I talked with the team, Lucas suspected there could be a
> reason why
> the workarounds were in their current places. I'll include him and
> Matt Roper
> here, since they had assigned themselves to check this.
>
> I think we should wait for their input before applying this patch.

ops, I have just pushed them...

I didn't review each workaround in detail, but at a high level the patch
below looks correct to me.  The intention was always to go back and find
all the workarounds touching non-RCS, non-CCS registers that are part of
the shared render/compute domain and move them to this
general_render_compute_wa_init() function.  The effort just stalled out
because the developer working on it left and nobody else has had time to
pick it up yet; it's been on our todo list for a while.

Moving these workarounds from rcs_engine_wa_init() to
general_render_compute_wa_init() would become very important if a SKU of
these platforms ever shows up with a fused-off render engine.  In that
case the workarounds would get missed (due to lack of RCS), which would
negatively impact the behavior of the remaining CCS engines.  However
that's just a theoretical problem today; in practice all DG2 and MTL
platforms have an RCS engine, so the code movement below will not cause
any functional change.  But it's still good to have Gustavo finally
cleaning this up because we never know what the future holds.

it may be that my fuzzy memory about this is that we didn't have yet the
general_render_compute_* function to add this kind of WA to, so some of
the workarounds got added to the engine because these registers reset
when either compute or render reset. So, yes... I agree this should be
fine.

Lucas De Marchi

Matt

>
> >
> > > ---
> > >  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 ef6065ce8267..918a271447e2 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > @@ -2341,10 +2341,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) ||
> > > @@ -2373,21 +2369,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)) {
> > > @@ -2412,16 +2393,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)) {
> > > @@ -3006,6 +2977,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) ||
> > > @@ -3027,6 +3007,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
> > >

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