[Intel-gfx] [PATCH v2 1/2] drm/i915/mtl: Extend Wa_16014892111 to MTL A-step

[Sripada, Radhakrishna]

Hi Matt,

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper at intel.com>
> Sent: Monday, May 15, 2023 10:28 AM
> To: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>
> Cc: Sousa, Gustavo <gustavo.sousa at intel.com>; intel-
> gfx at lists.freedesktop.org; Justen, Jordan L <jordan.l.justen at intel.com>;
> Kalvala, Haridhar <haridhar.kalvala at intel.com>
> 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 at intel.com>
> > > Sent: Monday, May 15, 2023 7:45 AM
> > > To: Sripada, Radhakrishna <radhakrishna.sripada at intel.com>; intel-
> > > gfx at lists.freedesktop.org
> > > Cc: Justen, Jordan L <jordan.l.justen at intel.com>; Sripada, Radhakrishna
> > > <radhakrishna.sripada at intel.com>; Kalvala, Haridhar
> > > <haridhar.kalvala at intel.com>; Roper, Matthew D
> > > <matthew.d.roper at intel.com>
> > > 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 at intel.com>
> > > >Cc: Matt Roper <matthew.d.roper at intel.com>
> > > >Cc: Gustavo Sousa <gustavo.sousa at intel.com>
> > > >Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> > > >---
> > > > 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