RE: [PATCH] drm/i915/guc: Add MCR type check for wa registers
> -Original Message-
> From: Roper, Matthew D
> Sent: Wednesday, December 20, 2023 1:49 AM
> To: Lin, Shuicheng
> 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 +, 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
> > Cc: Matt Roper
> > ---
> > 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 *re
Re: [PATCH] drm/i915/guc: Add MCR type check for wa registers
On Mon, Dec 18, 2023 at 11:46:44AM +, 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
> Cc: Matt Roper
> ---
> 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
[PATCH] drm/i915/guc: Add MCR type check for wa registers
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
Cc: Matt Roper
---
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 },
+ {},
+};
+
/*
* 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++)
--
2.25.1
