On Fri, Feb 03, 2023 at 10:03:49AM -0800, Lucas De Marchi wrote:
> On Thu, Feb 02, 2023 at 05:12:10PM -0800, Matt Roper wrote:
> > On Thu, Feb 02, 2023 at 04:57:08PM -0800, Lucas De Marchi wrote:
> > > Register 0x9424 is not replicated on any platform, so it shouldn't be
> > > declared with REG_MCR(). Declaring it with _MMIO() is basically
> > > duplicate of the GEN7 version, so just remove the GEN8 and change all
> > > the callers to use the right functions.
> >
> > According to an old copy of bspec page 13991, 0x9400-0x97FF was an MCR
> > range on gen8 platforms. Newer copies of that bspec page forgot to even
> > include the register range table, so it's not obvious unless you dig
> > through the history and look at a version from before Aug 2020.
> >
> > However bspec page 66673 indicates that this range went back to being a
> > singleton range in gen9 (and the other forcewake pages for newer
> > platforms indicate it stayed that way), so that means BDW and CHV are
> > the _only_ platforms that should treat it as MCR. Usage for other
> > platforms should either add a new "GEN9" definition, or just go back to
> > using the GEN7 definition.
>
> sounds like more a spec mistake. This range was listed as
> "slice common". I'm not sure we'd really have to set any steering for
> specific slice. Another thing is that we didn't set any steering for a
> long time in this register and it was working. Even now there is no
> table for gen8/gen9 in drivers/gpu/drm/i915/gt/intel_gt_mcr.c, so any
> call to intel_gt_mcr_* will simply fallback to "no steering required".
>
> For me, any MCR_REG() should correspond to registers in these
> tables. I don't think there's much point in annotating the register as
> MCR in its definition and then do nothing with it. Btw, this is how I
> started getting warning wrt this register: as you knowm, in xe driver
> you added a warning for registers missing from the mcr tables,
> which I think is indeed the right thing to do for the recent platforms.
I guess that's fair. Even though gen8 had multicast registers, I
believe the two types of steering (subslice and l3bank) could always be
reconciled with a single steering value; since the IFWI took care of
initializing this in a sane way, i915 never actually needed to touch it
(except when doing unicast reads for an errorstate dump or something).
I'm not sure the same is always true for gen9 though. We should
probably add tables for those just to be safe, but that's future work
rather than something that we need to worry about for this patch.
Likewise, we should also finally kill off mcr_ranges_*[] in the
workaround file at some point; now that we have is_mcr in the workaround
itself, those range tables are redundant. But that's also work for a
future series.
>
> For gen8, this change here should not change any behavior. It
> changes for gen11+ to the correct behavior. So I don't think we need to
> care much about double checking if gen8 had a unique behavior no other
> platforms have. I think just amending the commit message with more
> information like this would be ok.
Yeah, sounds good. With a slightly updated commit message
Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>
> Lucas De Marchi
>
> >
> >
> > Matt
> >
> > >
> > > Also use intel_uncore_rmw() rather than a read + write where possible.
> > >
> > > Fixes: a9e69428b1b4 ("drm/i915: Define MCR registers explicitly")
> > > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan@xxxxxxxxx>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > Cc: Gustavo Sousa <gustavo.sousa@xxxxxxxxx>
> > > Cc: Matt Atwood <matthew.s.atwood@xxxxxxxxx>
> > > Cc: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx>
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 5 +----
> > > drivers/gpu/drm/i915/gt/intel_workarounds.c | 4 ++--
> > > drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c | 5 ++---
> > > drivers/gpu/drm/i915/intel_pm.c | 10 +++++-----
> > > 4 files changed, 10 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > index 7fa18a3b3957..cc1539c7a6b6 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > @@ -686,10 +686,7 @@
> > > #define GEN6_RSTCTL _MMIO(0x9420)
> > >
> > > #define GEN7_MISCCPCTL _MMIO(0x9424)
> > > -#define GEN7_DOP_CLOCK_GATE_ENABLE (1 << 0)
> > > -
> > > -#define GEN8_MISCCPCTL MCR_REG(0x9424)
> > > -#define GEN8_DOP_CLOCK_GATE_ENABLE REG_BIT(0)
> > > +#define GEN7_DOP_CLOCK_GATE_ENABLE REG_BIT(0)
> > > #define GEN12_DOP_CLOCK_GATE_RENDER_ENABLE REG_BIT(1)
> > > #define GEN8_DOP_CLOCK_GATE_CFCLK_ENABLE (1 << 2)
> > > #define GEN8_DOP_CLOCK_GATE_GUC_ENABLE (1 << 4)
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > index 29718d0595f4..cfc122c17e28 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > @@ -1645,7 +1645,7 @@ dg2_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> > > wa_mcr_write_or(wal, XEHP_SQCM, EN_32B_ACCESS);
> > >
> > > /* Wa_14015795083 */
> > > - wa_mcr_write_clr(wal, GEN8_MISCCPCTL, GEN12_DOP_CLOCK_GATE_RENDER_ENABLE);
> > > + wa_write_clr(wal, GEN7_MISCCPCTL, GEN12_DOP_CLOCK_GATE_RENDER_ENABLE);
> > >
> > > /* Wa_18018781329 */
> > > wa_mcr_write_or(wal, RENDER_MOD_CTRL, FORCE_MISS_FTLB);
> > > @@ -1664,7 +1664,7 @@ pvc_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> > > pvc_init_mcr(gt, wal);
> > >
> > > /* Wa_14015795083 */
> > > - wa_mcr_write_clr(wal, GEN8_MISCCPCTL, GEN12_DOP_CLOCK_GATE_RENDER_ENABLE);
> > > + wa_write_clr(wal, GEN7_MISCCPCTL, GEN12_DOP_CLOCK_GATE_RENDER_ENABLE);
> > >
> > > /* Wa_18018781329 */
> > > wa_mcr_write_or(wal, RENDER_MOD_CTRL, FORCE_MISS_FTLB);
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> > > index 3d2249bda368..69133420c78b 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> > > @@ -39,9 +39,8 @@ static void guc_prepare_xfer(struct intel_gt *gt)
> > >
> > > if (GRAPHICS_VER(uncore->i915) == 9) {
> > > /* DOP Clock Gating Enable for GuC clocks */
> > > - intel_gt_mcr_multicast_write(gt, GEN8_MISCCPCTL,
> > > - GEN8_DOP_CLOCK_GATE_GUC_ENABLE |
> > > - intel_gt_mcr_read_any(gt, GEN8_MISCCPCTL));
> > > + intel_uncore_rmw(uncore, GEN7_MISCCPCTL, 0,
> > > + GEN8_DOP_CLOCK_GATE_GUC_ENABLE);
> > >
> > > /* allows for 5us (in 10ns units) before GT can go to RC6 */
> > > intel_uncore_write(uncore, GUC_ARAT_C6DIS, 0x1FF);
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index e0364c4141b8..798607959458 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -4300,8 +4300,8 @@ static void gen8_set_l3sqc_credits(struct drm_i915_private *dev_priv,
> > > u32 val;
> > >
> > > /* WaTempDisableDOPClkGating:bdw */
> > > - misccpctl = intel_gt_mcr_multicast_rmw(to_gt(dev_priv), GEN8_MISCCPCTL,
> > > - GEN8_DOP_CLOCK_GATE_ENABLE, 0);
> > > + misccpctl = intel_uncore_rmw(&dev_priv->uncore, GEN7_MISCCPCTL,
> > > + GEN7_DOP_CLOCK_GATE_ENABLE, 0);
> > >
> > > val = intel_gt_mcr_read_any(to_gt(dev_priv), GEN8_L3SQCREG1);
> > > val &= ~L3_PRIO_CREDITS_MASK;
> > > @@ -4315,7 +4315,7 @@ static void gen8_set_l3sqc_credits(struct drm_i915_private *dev_priv,
> > > */
> > > intel_gt_mcr_read_any(to_gt(dev_priv), GEN8_L3SQCREG1);
> > > udelay(1);
> > > - intel_gt_mcr_multicast_write(to_gt(dev_priv), GEN8_MISCCPCTL, misccpctl);
> > > + intel_uncore_write(&dev_priv->uncore, GEN7_MISCCPCTL, misccpctl);
> > > }
> > >
> > > static void icl_init_clock_gating(struct drm_i915_private *dev_priv)
> > > @@ -4453,8 +4453,8 @@ static void skl_init_clock_gating(struct drm_i915_private *dev_priv)
> > > gen9_init_clock_gating(dev_priv);
> > >
> > > /* WaDisableDopClockGating:skl */
> > > - intel_gt_mcr_multicast_rmw(to_gt(dev_priv), GEN8_MISCCPCTL,
> > > - GEN8_DOP_CLOCK_GATE_ENABLE, 0);
> > > + intel_uncore_rmw(&dev_priv->uncore, GEN7_MISCCPCTL,
> > > + GEN7_DOP_CLOCK_GATE_ENABLE, 0);
> > >
> > > /* WAC6entrylatency:skl */
> > > intel_uncore_rmw(&dev_priv->uncore, FBC_LLC_READ_CTRL, 0, FBC_LLC_FULLY_OPEN);
> > > --
> > > 2.39.0
> > >
> >
> > --
> > Matt Roper
> > Graphics Software Engineer
> > Linux GPU Platform Enablement
> > Intel Corporation
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
