[PATCH] drm/i915/guc: Change wa and EU_PERF_CNTL registers to MCR type

[Lin, Shuicheng]

Best Regards
Shuicheng

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper at intel.com>
> Sent: Thursday, December 21, 2023 7:46 AM
> To: Lin, Shuicheng <shuicheng.lin at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; Nerlige Ramappa, Umesh
> <umesh.nerlige.ramappa at intel.com>
> 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 at intel.com>
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
> > ---
> >  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 at intel.com>
> 
> 
> 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