On Wed, Jan 25, 2023 at 04:43:29PM -0300, Gustavo Sousa wrote:
> On Tue, Jan 24, 2023 at 05:14:06PM -0800, Matt Roper wrote:
> > Register reset characteristics (i.e., whether the register maintains or
> > loses its value on engine reset) is an important factor that determines
> > which wa_list we want to add workarounds to. We recently found out that
> > the bspec documentation for the Xe_HP's "GAM" registers in the 0xC800 -
> > 0xCFFF range was misleading; these registers do not actually lose their
> > value on engine resets as the documentation implied. This means there's
> > no need to re-apply workarounds touching these registers after a reset,
> > and the corresponding workarounds should be moved from the 'engine'
> > lists back to the 'gt' list.
> >
> > While moving these GAM-related workarounds to the various platforms' GT
> > workaround functions, we should also take care to handle Wa_18018781329
> > properly for MTL's two GTs --- the render/compute setting should be set
> > on the primary GT where those engines reside, and the vd/ve/gsc setting
> > should be set on the media GT. Previously the VD/VE/GSC setting was not
> > being properly applied.
> >
> > Cc: Gustavo Sousa <gustavo.sousa@xxxxxxxxx>
> > Fixes: edf176f48d87 ("drm/i915/dg2: Move misplaced 'ctx' & 'gt' wa's to engine wa list")
> > Fixes: b2006061ae28 ("drm/i915/xehpsdv: Move render/compute engine reset domains related workarounds")
> > Fixes: 41bb543f5598 ("drm/i915/mtl: Add initial gt workarounds")
> > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 1 +
> > drivers/gpu/drm/i915/gt/intel_workarounds.c | 88 +++++++++++++--------
> > drivers/gpu/drm/i915/i915_drv.h | 4 +
> > 3 files changed, 59 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > index 2727645864db..4a37d048b512 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > @@ -1100,6 +1100,7 @@
> > #define XEHP_MERT_MOD_CTRL MCR_REG(0xcf28)
> > #define RENDER_MOD_CTRL MCR_REG(0xcf2c)
> > #define COMP_MOD_CTRL MCR_REG(0xcf30)
> > +#define GSC_MOD_CTRL MCR_REG(0xcf30) /* media GT only */
> > #define VDBX_MOD_CTRL MCR_REG(0xcf34)
> > #define VEBX_MOD_CTRL MCR_REG(0xcf38)
> > #define FORCE_MISS_FTLB REG_BIT(3)
> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > index 4efc1a532982..0e7f64bb2860 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > @@ -1559,6 +1559,19 @@ xehpsdv_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> >
> > /* Wa_14011060649:xehpsdv */
> > wa_14011060649(gt, wal);
> > +
> > + /* Wa_18018781329 */
> > + wa_mcr_write_or(wal, RENDER_MOD_CTRL, FORCE_MISS_FTLB);
> > + wa_mcr_write_or(wal, COMP_MOD_CTRL, FORCE_MISS_FTLB);
> > + wa_mcr_write_or(wal, VDBX_MOD_CTRL, FORCE_MISS_FTLB);
> > + wa_mcr_write_or(wal, VEBX_MOD_CTRL, FORCE_MISS_FTLB);
>
> Maybe worth mentioning in the commit message that Wa_18018781329 is being
> extended to XEHPSDV in this patch? This could also be on its own patch.
Yeah, it's probably better to just drop it from this patch. We could
potentially add it to xehpsdv as a separate patch down the line if
necessary.
>
> > +
> > + /* Wa_14012362059:xehpsdv */
> > + wa_mcr_write_or(wal, XEHP_MERT_MOD_CTRL, FORCE_MISS_FTLB);
> > +
> > + /* Wa_14014368820:xehpsdv */
> > + wa_write_or(wal, GEN12_GAMCNTRL_CTRL,
> > + INVALIDATION_BROADCAST_MODE_DIS | GLOBAL_INVALIDATION_MODE);
> > }
> >
> > static void
> > @@ -1599,6 +1612,12 @@ dg2_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> > DSS_ROUTER_CLKGATE_DIS);
> > }
> >
> > + if (IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_B0) ||
> > + IS_DG2_GRAPHICS_STEP(gt->i915, G11, STEP_A0, STEP_B0)) {
> > + /* Wa_14012362059:dg2 */
> > + wa_mcr_write_or(wal, XEHP_MERT_MOD_CTRL, FORCE_MISS_FTLB);
> > + }
> > +
> > if (IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_B0)) {
> > /* Wa_14010948348:dg2_g10 */
> > wa_write_or(wal, UNSLCGCTL9430, MSQDUNIT_CLKGATE_DIS);
> > @@ -1644,6 +1663,12 @@ dg2_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> >
> > /* Wa_14011028019:dg2_g10 */
> > wa_mcr_write_or(wal, SSMCGCTL9530, RTFUNIT_CLKGATE_DIS);
> > +
> > + /* Wa_14010680813:dg2_g10 */
> > + wa_write_or(wal, GEN12_GAMSTLB_CTRL,
> > + CONTROL_BLOCK_CLKGATE_DIS |
> > + EGRESS_BLOCK_CLKGATE_DIS |
> > + TAG_BLOCK_CLKGATE_DIS);
> > }
> >
> > /* Wa_14014830051:dg2 */
> > @@ -1658,6 +1683,16 @@ dg2_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> >
> > /* Wa_14015795083 */
> > wa_mcr_write_clr(wal, GEN8_MISCCPCTL, GEN12_DOP_CLOCK_GATE_RENDER_ENABLE);
> > +
> > + /* Wa_18018781329 */
> > + wa_mcr_write_or(wal, RENDER_MOD_CTRL, FORCE_MISS_FTLB);
> > + wa_mcr_write_or(wal, COMP_MOD_CTRL, FORCE_MISS_FTLB);
> > + wa_mcr_write_or(wal, VDBX_MOD_CTRL, FORCE_MISS_FTLB);
> > + wa_mcr_write_or(wal, VEBX_MOD_CTRL, FORCE_MISS_FTLB);
> > +
> > + /* Wa_1509235366:dg2 */
> > + wa_write_or(wal, GEN12_GAMCNTRL_CTRL,
> > + INVALIDATION_BROADCAST_MODE_DIS | GLOBAL_INVALIDATION_MODE);
> > }
> >
> > static void
> > @@ -1667,16 +1702,27 @@ pvc_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> >
> > /* Wa_14015795083 */
> > wa_mcr_write_clr(wal, GEN8_MISCCPCTL, GEN12_DOP_CLOCK_GATE_RENDER_ENABLE);
> > +
> > + /* Wa_18018781329 */
> > + wa_mcr_write_or(wal, RENDER_MOD_CTRL, FORCE_MISS_FTLB);
> > + wa_mcr_write_or(wal, COMP_MOD_CTRL, FORCE_MISS_FTLB);
> > + wa_mcr_write_or(wal, VDBX_MOD_CTRL, FORCE_MISS_FTLB);
> > + wa_mcr_write_or(wal, VEBX_MOD_CTRL, FORCE_MISS_FTLB);
> > }
> >
> > static void
> > xelpg_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> > {
> > - /* Wa_14014830051 */
> > if (IS_MTL_GRAPHICS_STEP(gt->i915, M, STEP_A0, STEP_B0) ||
> > - IS_MTL_GRAPHICS_STEP(gt->i915, P, STEP_A0, STEP_B0))
> > + IS_MTL_GRAPHICS_STEP(gt->i915, P, STEP_A0, STEP_B0)) {
> > + /* Wa_14014830051 */
> > wa_mcr_write_clr(wal, SARB_CHICKEN1, COMP_CKN_IN);
> >
> > + /* Wa_18018781329 */
> > + wa_mcr_write_or(wal, RENDER_MOD_CTRL, FORCE_MISS_FTLB);
> > + wa_mcr_write_or(wal, COMP_MOD_CTRL, FORCE_MISS_FTLB);
> > + }
> > +
> > /*
> > * Unlike older platforms, we no longer setup implicit steering here;
> > * all MCR accesses are explicitly steered.
> > @@ -1687,7 +1733,12 @@ xelpg_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> > static void
> > xelpmp_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> > {
> > - /* FIXME: Actual workarounds will be added in future patch(es) */
> > + if (IS_MTL_MEDIA_STEP(gt->i915, STEP_A0, STEP_B0)) {
> > + /* Wa_18018781329 */
> > + wa_mcr_write_or(wal, GSC_MOD_CTRL, FORCE_MISS_FTLB);
> > + wa_mcr_write_or(wal, VDBX_MOD_CTRL, FORCE_MISS_FTLB);
> > + wa_mcr_write_or(wal, VEBX_MOD_CTRL, FORCE_MISS_FTLB);
>
> By looking at the spec, I under the assumption that GAM registers in MTL Media
> are not multicast. Am I wrong?
Hmm, good point. And surprising too...hopefully that's legitimate and
not a bspec oversight. I'll send an update to reflect this.
Matt
>
> > + }
> >
> > debug_dump_steering(gt);
> > }
> > @@ -2351,12 +2402,6 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
> > GEN12_DISABLE_READ_SUPPRESSION);
> > }
> >
> > - if (IS_DG2(i915)) {
> > - /* Wa_1509235366:dg2 */
> > - wa_write_or(wal, GEN12_GAMCNTRL_CTRL, INVALIDATION_BROADCAST_MODE_DIS |
> > - GLOBAL_INVALIDATION_MODE);
> > - }
> > -
> > if (IS_DG2_GRAPHICS_STEP(i915, G11, STEP_A0, STEP_B0)) {
> > /* Wa_14013392000:dg2_g11 */
> > wa_mcr_masked_en(wal, GEN8_ROW_CHICKEN2, GEN12_ENABLE_LARGE_GRF_MODE);
> > @@ -2416,18 +2461,6 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
> > wa_mcr_masked_en(wal, GEN9_HALF_SLICE_CHICKEN7,
> > DG2_DISABLE_ROUND_ENABLE_ALLOW_FOR_SSLA);
> >
> > - if (IS_DG2_GRAPHICS_STEP(engine->i915, G10, STEP_A0, STEP_B0)) {
> > - /* Wa_14010680813:dg2_g10 */
> > - wa_write_or(wal, GEN12_GAMSTLB_CTRL, CONTROL_BLOCK_CLKGATE_DIS |
> > - EGRESS_BLOCK_CLKGATE_DIS | TAG_BLOCK_CLKGATE_DIS);
> > - }
> > -
> > - if (IS_DG2_GRAPHICS_STEP(engine->i915, G10, STEP_A0, STEP_B0) ||
> > - IS_DG2_GRAPHICS_STEP(engine->i915, G11, STEP_A0, STEP_B0)) {
> > - /* Wa_14012362059:dg2 */
> > - wa_mcr_write_or(wal, XEHP_MERT_MOD_CTRL, FORCE_MISS_FTLB);
> > - }
> > -
> > if (IS_DG2_GRAPHICS_STEP(i915, G11, STEP_B0, STEP_FOREVER) ||
> > IS_DG2_G10(i915)) {
> > /* Wa_22014600077:dg2 */
> > @@ -2990,12 +3023,6 @@ general_render_compute_wa_init(struct intel_engine_cs *engine, struct i915_wa_li
> > IS_MTL_GRAPHICS_STEP(i915, P, STEP_A0, STEP_B0) ||
> > IS_PONTEVECCHIO(i915) ||
> > IS_DG2(i915)) {
> > - /* Wa_18018781329 */
> > - wa_mcr_write_or(wal, RENDER_MOD_CTRL, FORCE_MISS_FTLB);
> > - wa_mcr_write_or(wal, COMP_MOD_CTRL, FORCE_MISS_FTLB);
> > - wa_mcr_write_or(wal, VDBX_MOD_CTRL, FORCE_MISS_FTLB);
> > - wa_mcr_write_or(wal, VEBX_MOD_CTRL, FORCE_MISS_FTLB);
> > -
> > /* Wa_22014226127 */
> > wa_mcr_write_or(wal, LSC_CHICKEN_BIT_0, DISABLE_D8_D16_COASLESCE);
> > }
> > @@ -3062,13 +3089,6 @@ general_render_compute_wa_init(struct intel_engine_cs *engine, struct i915_wa_li
> > wa_mcr_masked_dis(wal, MLTICTXCTL, TDONRENDER);
> > wa_mcr_write_or(wal, L3SQCREG1_CCS0, FLUSHALLNONCOH);
> > }
> > -
> > - /* Wa_14012362059:xehpsdv */
> > - wa_mcr_write_or(wal, XEHP_MERT_MOD_CTRL, FORCE_MISS_FTLB);
> > -
> > - /* Wa_14014368820:xehpsdv */
> > - wa_write_or(wal, GEN12_GAMCNTRL_CTRL, INVALIDATION_BROADCAST_MODE_DIS |
> > - GLOBAL_INVALIDATION_MODE);
> > }
> >
> > if (IS_DG2(i915) || IS_PONTEVECCHIO(i915)) {
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 2a6e212f8824..e90baa28a308 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -696,6 +696,10 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> > (IS_METEORLAKE(__i915) && \
> > IS_DISPLAY_STEP(__i915, since, until))
> >
> > +#define IS_MTL_MEDIA_STEP(__i915, since, until) \
> > + (IS_METEORLAKE(__i915) && \
> > + IS_MEDIA_STEP(__i915, since, until))
> > +
> > /*
> > * DG2 hardware steppings are a bit unusual. The hardware design was forked to
> > * create three variants (G10, G11, and G12) which each have distinct
> > --
> > 2.39.0
> >
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
