Re: [PATCH 1/2] drm/i915: Move uncore selfchecks to late selftest infrastructure

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

On 07/12/2016 18:52, Chris Wilson wrote:
> Now that the kselftest infrastructure exists, put it to use and add to
> it the existing consistency checks on the fw register lookup tables.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_late_selftests.h |   1 +
>  drivers/gpu/drm/i915/intel_uncore.c        | 116 +++++++++++++++++------------
>  2 files changed, 69 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_late_selftests.h b/drivers/gpu/drm/i915/i915_late_selftests.h
> index e6645d08d964..289a651db2fd 100644
> --- a/drivers/gpu/drm/i915/i915_late_selftests.h
> +++ b/drivers/gpu/drm/i915/i915_late_selftests.h
> @@ -8,4 +8,5 @@
>   *
>   * Tests are executed in reverse order by igt/drv_selftest
>   */
> +selftest(uncore, intel_uncore_late_selftests)
>  selftest(sanitycheck, i915_late_sanitycheck) /* keep last */
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index c1ca4df38dea..bd8436b8f3a4 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -628,33 +628,6 @@ find_fw_domain(struct drm_i915_private *dev_priv, u32 offset)
>  	return entry ? entry->domains : 0;
>  }
>
> -static void
> -intel_fw_table_check(struct drm_i915_private *dev_priv)
> -{
> -	const struct intel_forcewake_range *ranges;
> -	unsigned int num_ranges;
> -	s32 prev;
> -	unsigned int i;
> -
> -	if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG))
> -		return;
> -
> -	ranges = dev_priv->uncore.fw_domains_table;
> -	if (!ranges)
> -		return;
> -
> -	num_ranges = dev_priv->uncore.fw_domains_table_entries;
> -
> -	for (i = 0, prev = -1; i < num_ranges; i++, ranges++) {
> -		WARN_ON_ONCE(IS_GEN9(dev_priv) &&
> -			     (prev + 1) != (s32)ranges->start);
> -		WARN_ON_ONCE(prev >= (s32)ranges->start);
> -		prev = ranges->start;
> -		WARN_ON_ONCE(prev >= (s32)ranges->end);
> -		prev = ranges->end;
> -	}
> -}
> -
>  #define GEN_FW_RANGE(s, e, d) \
>  	{ .start = (s), .end = (e), .domains = (d) }
>
> @@ -693,23 +666,6 @@ static const i915_reg_t gen8_shadowed_regs[] = {
>  	/* TODO: Other registers are not yet used */
>  };
>
> -static void intel_shadow_table_check(void)
> -{
> -	const i915_reg_t *reg = gen8_shadowed_regs;
> -	s32 prev;
> -	u32 offset;
> -	unsigned int i;
> -
> -	if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG))
> -		return;
> -
> -	for (i = 0, prev = -1; i < ARRAY_SIZE(gen8_shadowed_regs); i++, reg++) {
> -		offset = i915_mmio_reg_offset(*reg);
> -		WARN_ON_ONCE(prev >= (s32)offset);
> -		prev = offset;
> -	}
> -}
> -
>  static int mmio_reg_cmp(u32 key, const i915_reg_t *reg)
>  {
>  	u32 offset = i915_mmio_reg_offset(*reg);
> @@ -1436,10 +1392,6 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
>  		break;
>  	}
>
> -	intel_fw_table_check(dev_priv);
> -	if (INTEL_GEN(dev_priv) >= 8)
> -		intel_shadow_table_check();
> -
>  	if (intel_vgpu_active(dev_priv)) {
>  		ASSIGN_WRITE_MMIO_VFUNCS(vgpu);
>  		ASSIGN_READ_MMIO_VFUNCS(vgpu);
> @@ -1962,3 +1914,71 @@ intel_uncore_forcewake_for_reg(struct drm_i915_private *dev_priv,
>
>  	return fw_domains;
>  }
> +
> +#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> +#include "i915_selftest.h"
> +
> +static int intel_fw_table_check(struct drm_i915_private *i915)
> +{
> +	const struct intel_forcewake_range *ranges;
> +	unsigned int num_ranges, i;
> +	s32 prev;
> +
> +	ranges = i915->uncore.fw_domains_table;
> +	if (!ranges)
> +		return 0;
> +
> +	num_ranges = i915->uncore.fw_domains_table_entries;
> +	for (i = 0, prev = -1; i < num_ranges; i++, ranges++) {
> +		/* Check that the tabke is watertight */

table

> +		if (WARN_ON(IS_GEN9(i915) && (prev + 1) != (s32)ranges->start))
> +			return -EINVAL;
> +
> +		/* Check that the table never goes backwards */
> +		if (WARN_ON(prev >= (s32)ranges->start))
> +			return -EINVAL;
> +
> +		/* Check that the entry is valid */
> +		if (WARN_ON(ranges->start >= (s32)ranges->end))

Comparison between signed and unsigned?

> +			return -EINVAL;
> +
> +		prev = ranges->end;
> +	}
> +
> +	return 0;
> +}
> +
> +static int intel_shadow_table_check(void)
> +{
> +	const i915_reg_t *reg = gen8_shadowed_regs;
> +	unsigned int i;
> +	s32 prev;
> +
> +	for (i = 0, prev = -1; i < ARRAY_SIZE(gen8_shadowed_regs); i++, reg++) {
> +		u32 offset = i915_mmio_reg_offset(*reg);
> +		if (WARN_ON(prev >= (s32)offset))
> +			return -EINVAL;
> +
> +		prev = offset;
> +	}
> +
> +	return 0;
> +}
> +
> +int intel_uncore_late_selftests(struct drm_i915_private *i915)
> +{
> +	int err;
> +
> +	err = intel_fw_table_check(i915);
> +	if (err)
> +		return err;
> +
> +	if (INTEL_GEN(i915) >= 8) {

Could just always check it, the fw table is identically unused on some 
platforms.

> +		err = intel_shadow_table_check();
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +#endif
>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx