| Message ID | 20231218114644.3799660-1-shuicheng.lin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/guc: Add MCR type check for wa registers | expand |
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@intel.com> > Cc: Matt Roper <matthew.d.roper@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. > + > /* > * 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. > /* > * 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). 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 > + 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 >
> -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@intel.com> > Sent: Wednesday, December 20, 2023 1:49 AM > To: Lin, Shuicheng <shuicheng.lin@intel.com> > Cc: intel-gfx@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@intel.com> > > Cc: Matt Roper <matthew.d.roper@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
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 }, + {}, +}; + /* * 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; /* * 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; +} + /** * 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)) + 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++)
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@intel.com> Cc: Matt Roper <matthew.d.roper@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(-)