[Intel-gfx] [PATCH 4/4] drm/i915/selftest: Use forcewake to sanity check engine wa lists

[Gustavo Sousa]

On Wed, Feb 01, 2023 at 02:28:31PM -0800, Matt Roper wrote:
> Although register information in the bspec includes a field that is
> supposed to reflect a register's reset characteristics (i.e., whether a
> register maintains its value through engine resets), it's been
> discovered that this information is incorrect for some register ranges
> (i.e., registers that are not affected by engine resets are tagged in a
> way that indicates they would be).
> 
> We can sanity check workaround registers placed on the RCS/CCS engine
> workaround lists (including those placed there via the
> general_render_compute_wa_init() function) by comparing against the
> forcewake table.  As far as we know, there's never a case where a
> register that lives outside the RENDER powerwell will be reset by an
> RCS/CCS engine reset.
> 
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
>  .../gpu/drm/i915/gt/selftest_workarounds.c    | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index 14a8b25b6204..1bc8febc5c1d 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -1362,12 +1362,64 @@ live_engine_reset_workarounds(void *arg)
>  	return ret;
>  }
>  
> +/*
> + * The bspec's documentation for register reset behavior can be unreliable for
> + * some MMIO ranges.  But in general we do not expect registers outside the
> + * RENDER forcewake domain to be reset by RCS/CCS engine resets.  If we find
> + * workaround registers on an RCS or CCS engine's list, it likely indicates

I think "workaround registers" is too general and makes the sentence a bit
confusing. I believe you mean those registers mentioned in the previous
sentence, right? Maybe s/workaround registers/said registers/?

> + * the register is misdocumented in the bspec and the workaround implementation
> + * should be moved to the GT workaround list instead.
> + */
> +static int
> +live_check_engine_workarounds_fw(void *arg)
> +{
> +	struct intel_gt *gt = arg;
> +	struct intel_engine_cs *engine;
> +	struct wa_lists *lists;
> +	enum intel_engine_id id;
> +	int ret = 0;
> +
> +	lists = kzalloc(sizeof(*lists), GFP_KERNEL);
> +	if (!lists)
> +		return -ENOMEM;
> +
> +	reference_lists_init(gt, lists);
> +
> +	for_each_engine(engine, gt, id) {
> +		struct i915_wa_list *wal = &lists->engine[id].wa_list;
> +		struct i915_wa *wa;
> +		int i;
> +
> +		if (engine->class != RENDER_CLASS &&
> +		    engine->class != COMPUTE_CLASS)
> +			continue;
> +
> +		for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> +			enum forcewake_domains fw;
> +
> +			fw = intel_uncore_forcewake_for_reg(gt->uncore, wa->reg,
> +							    FW_REG_READ | FW_REG_WRITE);
> +			if ((fw & FORCEWAKE_RENDER) == 0) {
> +				pr_err("%s: Register %#x not in RENDER forcewake domain!\n",
> +				       engine->name, i915_mmio_reg_offset(wa->reg));

I think it is safer to use the correct member (wa->reg vs wa->mcr_reg) according
to the value of wa->is_mcr. Coincidently the reg member for both types have the
same offset in the struct, but I do not think we should rely on that.

One issue is that, unlike i915_mmio_reg_offset(),
intel_uncore_forcewake_for_reg() is not implemented with generics and expects
i915_reg_t. A workaround here would be "converting" the wa->mcr_reg (when
wa->is_mcr holds) an i915_reg_t by copying the correct fields. While this is
trivial since both types have only one field, I think the proper way
(future-proof) of doing that is by having a dedicated function/macro to do the
transformation.

Thinking about an alternative approach, do you think we could say that
i915_mcr_reg_t will always have the same fields as i915_reg_t? In that case, we
could tweak the type definition (or at least formalize via code documentation)
to reflect that and then it would be okay to always use wa->reg here, as
i915_mcr_reg_t would be thought as a subclass of i915_reg_t.

> +				ret = -EINVAL;
> +			}
> +		}
> +	}
> +
> +	reference_lists_fini(gt, lists);
> +	kfree(lists);
> +
> +	return ret;
> +}
> +
>  int intel_workarounds_live_selftests(struct drm_i915_private *i915)
>  {
>  	static const struct i915_subtest tests[] = {
>  		SUBTEST(live_dirty_whitelist),
>  		SUBTEST(live_reset_whitelist),
>  		SUBTEST(live_isolated_whitelist),
> +		SUBTEST(live_check_engine_workarounds_fw),
>  		SUBTEST(live_gpu_reset_workarounds),
>  		SUBTEST(live_engine_reset_workarounds),
>  	};
> -- 
> 2.39.1
>