On Wed, Nov 30, 2022 at 09:21:07PM +0530, Balasubramani Vivekanandan wrote:
> On 28.11.2022 15:30, Matt Roper wrote:
> > PPAT setup involves a series of multicast writes. This can be optimized
> > slightly be acquiring forcewake and the steering lock just once for the
> > entire sequence.
> >
> > Suggested-by: Balasubramani Vivekanandan
> > <balasubramani.vivekanan...@intel.com>
> > Signed-off-by: Matt Roper <matthew.d.ro...@intel.com>
> > ---
> > drivers/gpu/drm/i915/gt/intel_gtt.c | 27 +++++++++++++++++++--------
> > 1 file changed, 19 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c
> > b/drivers/gpu/drm/i915/gt/intel_gtt.c
> > index 2ba3983984b9..288d9f118ee9 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gtt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
> > @@ -482,14 +482,25 @@ static void tgl_setup_private_ppat(struct
> > intel_uncore *uncore)
> >
> > static void xehp_setup_private_ppat(struct intel_gt *gt)
> > {
> > - intel_gt_mcr_multicast_write(gt, XEHP_PAT_INDEX(0), GEN8_PPAT_WB);
> > - intel_gt_mcr_multicast_write(gt, XEHP_PAT_INDEX(1), GEN8_PPAT_WC);
> > - intel_gt_mcr_multicast_write(gt, XEHP_PAT_INDEX(2), GEN8_PPAT_WT);
> > - intel_gt_mcr_multicast_write(gt, XEHP_PAT_INDEX(3), GEN8_PPAT_UC);
> > - intel_gt_mcr_multicast_write(gt, XEHP_PAT_INDEX(4), GEN8_PPAT_WB);
> > - intel_gt_mcr_multicast_write(gt, XEHP_PAT_INDEX(5), GEN8_PPAT_WB);
> > - intel_gt_mcr_multicast_write(gt, XEHP_PAT_INDEX(6), GEN8_PPAT_WB);
> > - intel_gt_mcr_multicast_write(gt, XEHP_PAT_INDEX(7), GEN8_PPAT_WB);
> > + enum forcewake_domains fw;
> > + unsigned long flags;
> > +
> > + fw = intel_uncore_forcewake_for_reg(gt->uncore,
> > _MMIO(XEHP_PAT_INDEX(0).reg),
> > + FW_REG_READ);
>
> I am not completely aware of forcewake implementation. I am wondering if
> the last parameter should be FW_REG_WRITE since it is register write
> which is happening later.
Yep, good catch. Using FW_REG_WRITE allows the driver to potentially
skip obtaining forcewake and waking the hardware if the registers being
written are "shadowed" so it's always good to use FW_REG_WRITE in places
where we're only writing and not reading.
In this case the specific registers in question don't appear to be part
of the shadow table for any affected platforms (e.g.,
dg2_shadowed_regs[] and such in intel_uncore.c), so FW_REG_WRITE will
still behave the same as FW_REG_READ here. But it's always possible
that future platforms could decide to shadow these registers, so it's
good to fix anyway; I just sent an updated copy of this patch making
that change.
Matt
>
> Regards,
> Bala
>
> > + intel_uncore_forcewake_get(gt->uncore, fw);
> > +
> > + intel_gt_mcr_lock(gt, &flags);
> > + intel_gt_mcr_multicast_write_fw(gt, XEHP_PAT_INDEX(0), GEN8_PPAT_WB);
> > + intel_gt_mcr_multicast_write_fw(gt, XEHP_PAT_INDEX(1), GEN8_PPAT_WC);
> > + intel_gt_mcr_multicast_write_fw(gt, XEHP_PAT_INDEX(2), GEN8_PPAT_WT);
> > + intel_gt_mcr_multicast_write_fw(gt, XEHP_PAT_INDEX(3), GEN8_PPAT_UC);
> > + intel_gt_mcr_multicast_write_fw(gt, XEHP_PAT_INDEX(4), GEN8_PPAT_WB);
> > + intel_gt_mcr_multicast_write_fw(gt, XEHP_PAT_INDEX(5), GEN8_PPAT_WB);
> > + intel_gt_mcr_multicast_write_fw(gt, XEHP_PAT_INDEX(6), GEN8_PPAT_WB);
> > + intel_gt_mcr_multicast_write_fw(gt, XEHP_PAT_INDEX(7), GEN8_PPAT_WB);
> > + intel_gt_mcr_unlock(gt, flags);
> > +
> > + intel_uncore_forcewake_put(gt->uncore, fw);
> > }
> >
> > static void icl_setup_private_ppat(struct intel_uncore *uncore)
> > --
> > 2.38.1
> >
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
