[PATCH] drm/i915/guc: Add MCR type check for wa registers

Wed Dec 20 11:59:14 UTC 2023

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper at intel.com>
> Sent: Wednesday, December 20, 2023 1:49 AM
> To: Lin, Shuicheng <shuicheng.lin at intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> 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 at intel.com>
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > ---
> >  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