On Tue, Feb 07, 2023 at 07:37:58PM -0300, Gustavo Sousa wrote:
> 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.
Ah, we already have that: mcr_reg_cast()
:-)
So my suggestion is:
i915_reg_t reg = wa->is_mcr ? mcr_reg_cast(wa->mcr_reg) : wa->reg;
Ans use reg as argument for both intel_uncore_forcewake_for_reg() and
i915_mmio_reg_offset().
>
> 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
> >
