diff mbox series

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

Message ID 20230201222831.608281-4-matthew.d.roper@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/4] drm/i915/pvc: Annotate two more workaround/tuning registers as MCR | expand

Commit Message

Matt Roper Feb. 1, 2023, 10:28 p.m. UTC 
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@intel.com>
---
 .../gpu/drm/i915/gt/selftest_workarounds.c    | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)

Comments

Gustavo Sousa Feb. 7, 2023, 10:37 p.m. UTC | #1 
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@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
>
Gustavo Sousa Feb. 8, 2023, 12:51 p.m. UTC | #2 
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@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
> >
diff mbox series

Patch

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
+ * 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));
+				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),
 	};