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.
> +
> /*
> * 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
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
