> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx>
> Sent: Wednesday, December 20, 2023 1:49 AM
> To: Lin, Shuicheng <shuicheng.lin@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH] drm/i915/guc: Add MCR type check for wa registers
>
> On Mon, Dec 18, 2023 at 11:46:44AM +0000, Shuicheng Lin wrote:
> > Some of the wa registers are MCR registers, which have different
> > read/write process with normal MMIO registers.
> > Add function intel_gt_is_mcr_reg to check whether it is mcr register
> > or not.
> >
> > Signed-off-by: Shuicheng Lin <shuicheng.lin@xxxxxxxxx>
> > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 27
> ++++++++++++++++++++++
> > drivers/gpu/drm/i915/gt/intel_gt_mcr.h | 2 ++
> > drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c | 6 ++++-
> > 3 files changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > index e253750a51c5..b3ee9d152dbe 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > @@ -81,6 +81,11 @@ static const struct intel_mmio_range
> dg2_lncf_steering_table[] = {
> > {},
> > };
> >
> > +static const struct intel_mmio_range dg2_dss_steering_table[] = {
> > + { 0x00E400, 0x00E7FF },
> > + {},
> > +};
>
> This is incomplete. There are a _lot_ more DSS MCR ranges than this.
Yes. I just added the range I need.
>
> > +
> > /*
> > * We have several types of MCR registers on PVC where steering to (0,0)
> > * will always provide us with a non-terminated value. We'll stick
> > them @@ -190,6 +195,7 @@ void intel_gt_mcr_init(struct intel_gt *gt)
> > } else if (IS_DG2(i915)) {
> > gt->steering_table[MSLICE] = xehpsdv_mslice_steering_table;
> > gt->steering_table[LNCF] = dg2_lncf_steering_table;
> > + gt->steering_table[DSS] = dg2_dss_steering_table;
>
> Why are you making this change only on DG2? There are DSS steering ranges
> that we've been implicitly steering on many platforms. Switching to explicit
> steering is something that should probably happen on all platforms or no
> platforms.
>
Agree.
> > /*
> > * No need to hook up the GAM table since it has a dedicated
> > * steering control register on DG2 and can use implicit @@ -
> 715,6
> > +721,27 @@ void intel_gt_mcr_get_nonterminated_steering(struct intel_gt
> *gt,
> > *instance = gt->default_steering.instanceid; }
> >
> > +/*
> > + * intel_gt_is_mcr_reg - check whether it is a mcr register or not
> > + * @gt: GT structure
> > + * @reg: the register to check
> > + *
> > + * Returns true if @reg belong to a register range of any steering
> > +type,
> > + * otherwise, return false.
> > + */
> > +bool intel_gt_is_mcr_reg(struct intel_gt *gt, i915_reg_t reg) {
> > + int type;
> > + i915_mcr_reg_t mcr_reg = {.reg = reg.reg};
> > +
> > + for (type = 0; type < NUM_STEERING_TYPES; type++) {
> > + if (reg_needs_read_steering(gt, mcr_reg, type)) {
> > + return true;
> > + }
> > + }
> > + return false;
> > +}
>
> We don't need this function; see below.
>
> > +
> > /**
> > * intel_gt_mcr_read_any_fw - reads one instance of an MCR register
> > * @gt: GT structure
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
> > b/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
> > index 01ac565a56a4..1ac5e6e2814e 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.h
> > @@ -30,6 +30,8 @@ void intel_gt_mcr_multicast_write_fw(struct intel_gt
> > *gt,
> > u32 intel_gt_mcr_multicast_rmw(struct intel_gt *gt, i915_mcr_reg_t reg,
> > u32 clear, u32 set);
> >
> > +bool intel_gt_is_mcr_reg(struct intel_gt *gt, i915_reg_t reg);
> > +
> > void intel_gt_mcr_get_nonterminated_steering(struct intel_gt *gt,
> > i915_mcr_reg_t reg,
> > u8 *group, u8 *instance);
> > 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..6c3910c9dce5 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
> > @@ -377,8 +377,12 @@ static int guc_mmio_regset_init(struct
> temp_regset *regset,
> > CCS_MASK(engine->gt))
> > ret |= GUC_MMIO_REG_ADD(gt, regset, GEN12_RCU_MODE,
> true);
> >
> > + /* Check whether there is MCR register or not */
> > for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
> > - ret |= GUC_MMIO_REG_ADD(gt, regset, wa->reg, wa-
> >masked_reg);
> > + if (intel_gt_is_mcr_reg(gt, wa->reg))
>
> The proper condition here would be wa->is_mcr. Of course that assumes that
> you actually define all of the workarounds appropriately (using the MCR
> variants when appropriate) and also the registers properly (using
> MCR_REG() instead of _MMIO).
Yes. It is better.
>
> Converting all of the implicit steering over to explicit steering is a lot of invasive
> changes to the code, and I'm not sure it's really worth it at this point. A much
> simpler and equally effective fix for the GuC not steering DSS (and GSLICE)
> ranges properly would be to just add the steering flags inside
> guc_mmio_reg_add() so that it gets added to all registers (both MCR and non-
> MCR). That's basically what we used to do (and we even still have a comment
> to that effect inside guc_mcr_reg_add()). Then guc_mcr_reg_add() can
> become a one-line function to just typecast the register.
>
>
> Matt
I checked all the registers added by guc_mmio_reg_add(), and confirmed only
some of the WA registers and the EU_PERF_CNTL registers are in the MCR range.
So I will update patch to change the WA registers and EU_PERF_CNTL registers to
MCR type, and keep other registers with MMIO type.
Thanks for your detail explanation.
>
> > + ret |= GUC_MCR_REG_ADD(gt, regset, wa->mcr_reg,
> wa->masked_reg);
> > + else
> > + ret |= GUC_MMIO_REG_ADD(gt, regset, wa->reg, wa-
> >masked_reg);
> >
> > /* Be extra paranoid and include all whitelist registers. */
> > for (i = 0; i < RING_MAX_NONPRIV_SLOTS; i++)
> > --
> > 2.25.1
> >
>
> --
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation
