Re: [PATCH] drm/i915/selftests: Move uncore fw selftests to operate on intel_gt

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 22/10/2019 14:10, Chris Wilson wrote:
> Forcewake is the speciality of the GT, so it is natural to run the
> intel_uncore_forcewake tests over the GT. So pass intel_gt as the
> parameter to our selftests.

I had the same urge yesterday but then ended unsure how the display/gt 
uncore split may affect this on the logical level. So I'll leave the 
decision to Daniele, he should know better what plans are there.

On the particular patch only comment is that I would consider one or two 
i915 locals for better readability.

Regards,

Tvrtko

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   drivers/gpu/drm/i915/selftests/intel_uncore.c | 54 ++++++++++---------
>   1 file changed, 28 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c
> index 0ffb141eb988..0e4e6be0101d 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_uncore.c
> @@ -140,19 +140,19 @@ static int live_forcewake_ops(void *arg)
>   		}
>   	};
>   	const struct reg *r;
> -	struct drm_i915_private *i915 = arg;
> +	struct intel_gt *gt = arg;
>   	struct intel_uncore_forcewake_domain *domain;
> -	struct intel_uncore *uncore = &i915->uncore;
> +	struct intel_uncore *uncore = gt->uncore;
>   	struct intel_engine_cs *engine;
>   	enum intel_engine_id id;
>   	intel_wakeref_t wakeref;
>   	unsigned int tmp;
>   	int err = 0;
>   
> -	GEM_BUG_ON(i915->gt.awake);
> +	GEM_BUG_ON(gt->awake);
>   
>   	/* vlv/chv with their pcu behave differently wrt reads */
> -	if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
> +	if (IS_VALLEYVIEW(gt->i915) || IS_CHERRYVIEW(gt->i915)) {
>   		pr_debug("PCU fakes forcewake badly; skipping\n");
>   		return 0;
>   	}
> @@ -170,15 +170,15 @@ static int live_forcewake_ops(void *arg)
>   
>   	/* We have to pick carefully to get the exact behaviour we need */
>   	for (r = registers; r->name; r++)
> -		if (r->platforms & INTEL_INFO(i915)->gen_mask)
> +		if (r->platforms & INTEL_INFO(gt->i915)->gen_mask)
>   			break;
>   	if (!r->name) {
>   		pr_debug("Forcewaked register not known for %s; skipping\n",
> -			 intel_platform_name(INTEL_INFO(i915)->platform));
> +			 intel_platform_name(INTEL_INFO(gt->i915)->platform));
>   		return 0;
>   	}
>   
> -	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> +	wakeref = intel_runtime_pm_get(uncore->rpm);
>   
>   	for_each_fw_domain(domain, uncore, tmp) {
>   		smp_store_mb(domain->active, false);
> @@ -188,7 +188,7 @@ static int live_forcewake_ops(void *arg)
>   		intel_uncore_fw_release_timer(&domain->timer);
>   	}
>   
> -	for_each_engine(engine, i915, id) {
> +	for_each_engine(engine, gt, id) {
>   		i915_reg_t mmio = _MMIO(engine->mmio_base + r->offset);
>   		u32 __iomem *reg = uncore->regs + engine->mmio_base + r->offset;
>   		enum forcewake_domains fw_domains;
> @@ -249,22 +249,22 @@ static int live_forcewake_ops(void *arg)
>   	}
>   
>   out_rpm:
> -	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> +	intel_runtime_pm_put(uncore->rpm, wakeref);
>   	return err;
>   }
>   
>   static int live_forcewake_domains(void *arg)
>   {
>   #define FW_RANGE 0x40000
> -	struct drm_i915_private *dev_priv = arg;
> -	struct intel_uncore *uncore = &dev_priv->uncore;
> +	struct intel_gt *gt = arg;
> +	struct intel_uncore *uncore = gt->uncore;
>   	unsigned long *valid;
>   	u32 offset;
>   	int err;
>   
> -	if (!HAS_FPGA_DBG_UNCLAIMED(dev_priv) &&
> -	    !IS_VALLEYVIEW(dev_priv) &&
> -	    !IS_CHERRYVIEW(dev_priv))
> +	if (!HAS_FPGA_DBG_UNCLAIMED(gt->i915) &&
> +	    !IS_VALLEYVIEW(gt->i915) &&
> +	    !IS_CHERRYVIEW(gt->i915))
>   		return 0;
>   
>   	/*
> @@ -283,7 +283,7 @@ static int live_forcewake_domains(void *arg)
>   	for (offset = 0; offset < FW_RANGE; offset += 4) {
>   		i915_reg_t reg = { offset };
>   
> -		(void)I915_READ_FW(reg);
> +		intel_uncore_posting_read_fw(uncore, reg);
>   		if (!check_for_unclaimed_mmio(uncore))
>   			set_bit(offset, valid);
>   	}
> @@ -300,7 +300,7 @@ static int live_forcewake_domains(void *arg)
>   
>   		check_for_unclaimed_mmio(uncore);
>   
> -		(void)I915_READ(reg);
> +		intel_uncore_posting_read_fw(uncore, reg);
>   		if (check_for_unclaimed_mmio(uncore)) {
>   			pr_err("Unclaimed mmio read to register 0x%04x\n",
>   			       offset);
> @@ -312,21 +312,23 @@ static int live_forcewake_domains(void *arg)
>   	return err;
>   }
>   
> +static int live_fw_table(void *arg)
> +{
> +	struct intel_gt *gt = arg;
> +
> +	/* Confirm the table we load is still valid */
> +	return intel_fw_table_check(gt->uncore->fw_domains_table,
> +				    gt->uncore->fw_domains_table_entries,
> +				    INTEL_GEN(gt->i915) >= 9);
> +}
> +
>   int intel_uncore_live_selftests(struct drm_i915_private *i915)
>   {
>   	static const struct i915_subtest tests[] = {
> +		SUBTEST(live_fw_table),
>   		SUBTEST(live_forcewake_ops),
>   		SUBTEST(live_forcewake_domains),
>   	};
>   
> -	int err;
> -
> -	/* Confirm the table we load is still valid */
> -	err = intel_fw_table_check(i915->uncore.fw_domains_table,
> -				   i915->uncore.fw_domains_table_entries,
> -				   INTEL_GEN(i915) >= 9);
> -	if (err)
> -		return err;
> -
> -	return i915_subtests(tests, i915);
> +	return intel_gt_live_subtests(tests, &i915->gt);
>   }
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx