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

[Matt Roper]

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.


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