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

[Gustavo Sousa]

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 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.

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
> >