Hi Matt, > -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx> > Sent: Monday, May 15, 2023 10:28 AM > To: Sripada, Radhakrishna <radhakrishna.sripada@xxxxxxxxx> > Cc: Sousa, Gustavo <gustavo.sousa@xxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx; Justen, Jordan L <jordan.l.justen@xxxxxxxxx>; > Kalvala, Haridhar <haridhar.kalvala@xxxxxxxxx> > Subject: Re: [PATCH v2 1/2] drm/i915/mtl: Extend Wa_16014892111 to MTL A- > step > > On Mon, May 15, 2023 at 08:42:25AM -0700, Sripada, Radhakrishna wrote: > > Hi Gustavo, > > > > > -----Original Message----- > > > From: Sousa, Gustavo <gustavo.sousa@xxxxxxxxx> > > > Sent: Monday, May 15, 2023 7:45 AM > > > To: Sripada, Radhakrishna <radhakrishna.sripada@xxxxxxxxx>; intel- > > > gfx@xxxxxxxxxxxxxxxxxxxxx > > > Cc: Justen, Jordan L <jordan.l.justen@xxxxxxxxx>; Sripada, Radhakrishna > > > <radhakrishna.sripada@xxxxxxxxx>; Kalvala, Haridhar > > > <haridhar.kalvala@xxxxxxxxx>; Roper, Matthew D > > > <matthew.d.roper@xxxxxxxxx> > > > Subject: Re: [PATCH v2 1/2] drm/i915/mtl: Extend Wa_16014892111 to MTL > A- > > > step > > > > > > Quoting Radhakrishna Sripada (2023-05-12 23:14:37) > > > >The dg2 workaround which is used for performance tuning > > > >is needed for Meteorlake A-step. > > > > > > > >v2: Limit the WA for A-step > > > > > > I think what Matt meant in the review for v1 was that this commit should > > > be rather about the tuning setting rather than the workaround itself. As > > > such, maybe we should change the commit message so that it focus on the > > > recommended tuning setting, i.e., instead of "Extend Wa_16014892111 to > > > MTL A-step" as subject, we should write something like "Apply > > > recommended tuning setting for ..." and give details. > > > > > > That said, since we are focusing on the tuning settings here, I guess > > > this could be squashed with the second patch and we could add a note > > > about DRAW_WATERMARK needing Wa_16014892111 for A steps of MTL. > > > > There are 2 aspects wrt. DRAW_WATERMARK. One that is a workaround > > which is applied on each context switch and is only applicable for DG2 > > and MTL-A step which is what this patch does. > > So just to be clear --- the workaround doesn't directly ask us to do > anything specific with DRAW_WATERMARK. What the workaround says is that > *if* we wind up needing to change the value of DRAW_WATERMARK away from > the hardware default, then we need to handle the save/restore on each > context switch ourselves since the hardware doesn't process this > register properly on context switch and will lose its value. > > It turns out that MTL has a tuning setting that suggests changing > DRAW_WATERMARK to a non-default value. Since the tuning setting is > constant (i.e., it doesn't change at runtime based on other factors), we > can ignore the "save" part of the workaround and just hardcode the > "restore" part to the value specified by the tuning setting. But what > we're programming here is still the tuning setting, it's just that the > way we implement the tuning is adjusted by the workaround's guidance. > > It might make sense to swap the order of these patches --- make the > first patch add the tuning setting (in the normal manner) for all > steppings not impacted by the workaround. Then come back and apply the > tuning setting in the special way on the A-step hardware to satisfy the > guidance of Wa_16014892111. Or maybe it's simpler to just ignore the > tuning setting on A-step entirely; that's a pre-production stepping of > the platform, so it's not really going to get used for performance work > anyway. If we don't bother programming the tuning on A-step, then we > also don't need to worry about the workaround either. Thank you for the explanation. I was inclined to drop the WA but we do have B-step parts in CI which have A-step GT hence trying to keep this around. I resend the patches swapping the order and adding better explanation. - Radhakrishna Sripada > > > Matt > > > > > The other is the tuning parameter setting which is applicable for all > > of MTL which is a onetime configuration Handled by the next patch > > during ctx_workarounds_init. > > > > - Radhakrishna Sripada > > > > > > > > > > -- > > > Gustavo Sousa > > > > > > > > > > >Bspec: 68331 > > > >Cc: Haridhar Kalvala <haridhar.kalvala@xxxxxxxxx> > > > >Cc: Matt Roper <matthew.d.roper@xxxxxxxxx> > > > >Cc: Gustavo Sousa <gustavo.sousa@xxxxxxxxx> > > > >Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx> > > > >--- > > > > drivers/gpu/drm/i915/gt/intel_lrc.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > >diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c > > > b/drivers/gpu/drm/i915/gt/intel_lrc.c > > > >index 81a96c52a92b..9c1007c44298 100644 > > > >--- a/drivers/gpu/drm/i915/gt/intel_lrc.c > > > >+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > > > >@@ -1370,7 +1370,9 @@ gen12_emit_indirect_ctx_rcs(const struct > > > intel_context *ce, u32 *cs) > > > > cs, GEN12_GFX_CCS_AUX_NV); > > > > > > > > /* Wa_16014892111 */ > > > >- if (IS_DG2(ce->engine->i915)) > > > >+ if (IS_DG2(ce->engine->i915) || > > > >+ IS_MTL_GRAPHICS_STEP(ce->engine->i915, M, STEP_A0, STEP_B0) > || > > > >+ IS_MTL_GRAPHICS_STEP(ce->engine->i915, P, STEP_A0, STEP_B0)) > > > > cs = dg2_emit_draw_watermark_setting(cs); > > > > > > > > return cs; > > > >-- > > > >2.34.1 > > > > > > -- > Matt Roper > Graphics Software Engineer > Linux GPU Platform Enablement > Intel Corporation
