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@xxxxxxxxx>
> ---
> .../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
>
