Best Regards
Shuicheng
> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>
> Sent: Thursday, December 21, 2023 7:46 AM
> To: Lin, Shuicheng <shuicheng.lin@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Nerlige Ramappa, Umesh
> <umesh.nerlige.ramappa@xxxxxxxxx>
> Subject: Re: [PATCH] drm/i915/guc: Change wa and EU_PERF_CNTL registers
> to MCR type
>
> On Wed, Dec 20, 2023 at 12:39:51PM +0000, Shuicheng Lin wrote:
> > Some of the wa registers are MCR register, and EU_PERF_CNTL registers
> > are MCR register.
> > MCR register needs extra process for read/write.
> > As normal MMIO register also could work with the MCR register process,
> > change all wa registers to MCR type for code simplicity.
> >
> > Signed-off-by: Shuicheng Lin <shuicheng.lin@xxxxxxxxx>
> > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c | 16 ++++++++--------
> > 1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > index 63724e17829a..61ff4c7e31a6 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > @@ -378,7 +378,7 @@ static int guc_mmio_regset_init(struct temp_regset
> *regset,
> > ret |= GUC_MMIO_REG_ADD(gt, regset, GEN12_RCU_MODE,
> true);
> >
> > for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
> > - ret |= GUC_MMIO_REG_ADD(gt, regset, wa->reg, wa-
> >masked_reg);
> > + ret |= GUC_MCR_REG_ADD(gt, regset, wa->mcr_reg, wa-
> >masked_reg);
>
> I'd add some kind of a comment here, explaining that it's safe to always use
> the MCR form here, even though some of the workarounds are touching non-
> MCR registers. With a clarifying code comment added,
>
> Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>
>
> Matt
Hi Matt, thanks for the review. I have updated patch to add the code comment.
Please let me know if you have any questions.
Thanks.
>
> >
> > /* Be extra paranoid and include all whitelist registers. */
> > for (i = 0; i < RING_MAX_NONPRIV_SLOTS; i++) @@ -394,13 +394,13
> @@
> > static int guc_mmio_regset_init(struct temp_regset *regset,
> > ret |= GUC_MMIO_REG_ADD(gt, regset,
> GEN9_LNCFCMOCS(i), false);
> >
> > if (GRAPHICS_VER(engine->i915) >= 12) {
> > - ret |= GUC_MMIO_REG_ADD(gt, regset, EU_PERF_CNTL0,
> false);
> > - ret |= GUC_MMIO_REG_ADD(gt, regset, EU_PERF_CNTL1,
> false);
> > - ret |= GUC_MMIO_REG_ADD(gt, regset, EU_PERF_CNTL2,
> false);
> > - ret |= GUC_MMIO_REG_ADD(gt, regset, EU_PERF_CNTL3,
> false);
> > - ret |= GUC_MMIO_REG_ADD(gt, regset, EU_PERF_CNTL4,
> false);
> > - ret |= GUC_MMIO_REG_ADD(gt, regset, EU_PERF_CNTL5,
> false);
> > - ret |= GUC_MMIO_REG_ADD(gt, regset, EU_PERF_CNTL6,
> false);
> > + ret |= GUC_MCR_REG_ADD(gt, regset,
> MCR_REG(i915_mmio_reg_offset(EU_PERF_CNTL0)), false);
> > + ret |= GUC_MCR_REG_ADD(gt, regset,
> MCR_REG(i915_mmio_reg_offset(EU_PERF_CNTL1)), false);
> > + ret |= GUC_MCR_REG_ADD(gt, regset,
> MCR_REG(i915_mmio_reg_offset(EU_PERF_CNTL2)), false);
> > + ret |= GUC_MCR_REG_ADD(gt, regset,
> MCR_REG(i915_mmio_reg_offset(EU_PERF_CNTL3)), false);
> > + ret |= GUC_MCR_REG_ADD(gt, regset,
> MCR_REG(i915_mmio_reg_offset(EU_PERF_CNTL4)), false);
> > + ret |= GUC_MCR_REG_ADD(gt, regset,
> MCR_REG(i915_mmio_reg_offset(EU_PERF_CNTL5)), false);
> > + ret |= GUC_MCR_REG_ADD(gt, regset,
> > +MCR_REG(i915_mmio_reg_offset(EU_PERF_CNTL6)), false);
> > }
> >
> > return ret ? -1 : 0;
> > --
> > 2.25.1
> >
>
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation
