Re: [PATCH v10] drm/i915/icl: Gen11 forcewake support

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 01/02/2018 00:52, Michel Thierry wrote:
> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
>
> The main difference with previous GENs is that starting from Gen11
> each VCS and VECS engine has its own power well, which only exist
> if the related engine exists in the HW.
> The fallback forcewake request workaround is only needed on gen9
> according to the HSDES WA entry (1604254524), so we can go back to using
> the simpler fw_domains_get/put functions.
>
> BSpec: 18331
>
> v2: fix fwtable, use array to test shadow tables, create new
>      accessors to avoid check on every access (Tvrtko)
> v3 (from Paulo): Rebase.
> v4:
>    - Range 09400-097FF should be FORCEWAKE_ALL (Daniele)
>    - Use the BIT macro for forcewake domains (Daniele)
>    - Add a comment about the range ordering (Oscar)
>    - Updated commit message (Oscar)
> v5: Rebased
> v6: Use I915_MAX_VCS/VECS (Michal)
> v7: translate FORCEWAKE_ALL to available domains
> v8: rebase, add clarification on fallback ack in commit message.
> v9: fix rebase issue, change check in fw_domains_init from IS_GEN11
>      to GEN >= 11
> v10: Generate is_genX_shadowed with a macro (Daniele)
>       Include gen11_fw_ranges in the selftest (Michel)
>
> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> Acked-by: Michel Thierry <michel.thierry@xxxxxxxxx>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Signed-off-by: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx>
> ---
>
>   drivers/gpu/drm/i915/i915_reg.h               |   4 +
>   drivers/gpu/drm/i915/intel_uncore.c           | 155 ++++++++++++++++++++++++--
>   drivers/gpu/drm/i915/intel_uncore.h           |  27 ++++-
>   drivers/gpu/drm/i915/selftests/intel_uncore.c |  31 ++++--
>   4 files changed, 193 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index d29e8a0e2ca3..eaca12292ffe 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8015,9 +8015,13 @@ enum {
>   #define   VLV_GTLC_PW_RENDER_STATUS_MASK	(1 << 7)
>   #define  FORCEWAKE_MT				_MMIO(0xa188) /* multi-threaded */
>   #define  FORCEWAKE_MEDIA_GEN9			_MMIO(0xa270)
> +#define  FORCEWAKE_MEDIA_VDBOX_GEN11(n)		_MMIO(0xa540 + (n) * 4)
> +#define  FORCEWAKE_MEDIA_VEBOX_GEN11(n)		_MMIO(0xa560 + (n) * 4)
>   #define  FORCEWAKE_RENDER_GEN9			_MMIO(0xa278)
>   #define  FORCEWAKE_BLITTER_GEN9			_MMIO(0xa188)
>   #define  FORCEWAKE_ACK_MEDIA_GEN9		_MMIO(0x0D88)
> +#define  FORCEWAKE_ACK_MEDIA_VDBOX_GEN11(n)	_MMIO(0x0D50 + (n) * 4)
> +#define  FORCEWAKE_ACK_MEDIA_VEBOX_GEN11(n)	_MMIO(0x0D70 + (n) * 4)
>   #define  FORCEWAKE_ACK_RENDER_GEN9		_MMIO(0x0D84)
>   #define  FORCEWAKE_ACK_BLITTER_GEN9		_MMIO(0x130044)
>   #define   FORCEWAKE_KERNEL			BIT(0)
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 164dbb8cfa36..c1953043604b 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -37,6 +37,12 @@ static const char * const forcewake_domain_names[] = {
>   	"render",
>   	"blitter",
>   	"media",
> +	"vdbox0",
> +	"vdbox1",
> +	"vdbox2",
> +	"vdbox3",
> +	"vebox0",
> +	"vebox1",
>   };
>   
>   const char *
> @@ -773,6 +779,8 @@ void assert_forcewakes_active(struct drm_i915_private *dev_priv,
>   
>   /* We give fast paths for the really cool registers */
>   #define NEEDS_FORCE_WAKE(reg) ((reg) < 0x40000)
> +#define GEN11_NEEDS_FORCE_WAKE(reg) \
> +	((reg) < 0x40000 || ((reg) >= 0x1c0000 && (reg) < 0x1dc000))

Nitpick - I'd perhaps at least have a blank line between the two 
defines, or even moved the GEN11 lower in file, just before the first 
mention of GEN11 specific code starts appearing.

>   
>   #define __gen6_reg_read_fw_domains(offset) \
>   ({ \
> @@ -826,6 +834,14 @@ find_fw_domain(struct drm_i915_private *dev_priv, u32 offset)
>   	if (!entry)
>   		return 0;
>   
> +	/*
> +	 * The list of FW domains depends on the SKU in gen11+ so we
> +	 * can't determine it statically. We use FORCEWAKE_ALL and
> +	 * translate it here to the list of available domains.
> +	 */
> +	if (entry->domains == FORCEWAKE_ALL)
> +		return dev_priv->uncore.fw_domains;
> +
>   	WARN(entry->domains & ~dev_priv->uncore.fw_domains,
>   	     "Uninitialized forcewake domain(s) 0x%x accessed at 0x%x\n",
>   	     entry->domains & ~dev_priv->uncore.fw_domains, offset);
> @@ -860,6 +876,14 @@ static const struct intel_forcewake_range __vlv_fw_ranges[] = {
>   	__fwd; \
>   })
>   
> +#define __gen11_fwtable_reg_read_fw_domains(offset) \
> +({ \
> +	enum forcewake_domains __fwd = 0; \
> +	if (GEN11_NEEDS_FORCE_WAKE((offset))) \
> +		__fwd = find_fw_domain(dev_priv, offset); \
> +	__fwd; \
> +})
> +
>   /* *Must* be sorted by offset! See intel_shadow_table_check(). */
>   static const i915_reg_t gen8_shadowed_regs[] = {
>   	RING_TAIL(RENDER_RING_BASE),	/* 0x2000 (base) */
> @@ -871,6 +895,20 @@ static const i915_reg_t gen8_shadowed_regs[] = {
>   	/* TODO: Other registers are not yet used */
>   };
>   
> +static const i915_reg_t gen11_shadowed_regs[] = {
> +	RING_TAIL(RENDER_RING_BASE),		/* 0x2000 (base) */
> +	GEN6_RPNSWREQ,				/* 0xA008 */
> +	GEN6_RC_VIDEO_FREQ,			/* 0xA00C */
> +	RING_TAIL(BLT_RING_BASE),		/* 0x22000 (base) */
> +	RING_TAIL(GEN11_BSD_RING_BASE),		/* 0x1C0000 (base) */
> +	RING_TAIL(GEN11_BSD2_RING_BASE),	/* 0x1C4000 (base) */
> +	RING_TAIL(GEN11_VEBOX_RING_BASE),	/* 0x1C8000 (base) */
> +	RING_TAIL(GEN11_BSD3_RING_BASE),	/* 0x1D0000 (base) */
> +	RING_TAIL(GEN11_BSD4_RING_BASE),	/* 0x1D4000 (base) */
> +	RING_TAIL(GEN11_VEBOX2_RING_BASE),	/* 0x1D8000 (base) */
> +	/* TODO: Other registers are not yet used */
> +};
> +
>   static int mmio_reg_cmp(u32 key, const i915_reg_t *reg)
>   {
>   	u32 offset = i915_mmio_reg_offset(*reg);
> @@ -883,14 +921,17 @@ static int mmio_reg_cmp(u32 key, const i915_reg_t *reg)
>   		return 0;
>   }
>   
> -static bool is_gen8_shadowed(u32 offset)
> -{
> -	const i915_reg_t *regs = gen8_shadowed_regs;
> -
> -	return BSEARCH(offset, regs, ARRAY_SIZE(gen8_shadowed_regs),
> -		       mmio_reg_cmp);
> +#define __is_genX_shadowed(x) \
> +static bool is_gen##x##_shadowed(u32 offset) \
> +{ \
> +	const i915_reg_t *regs = gen##x##_shadowed_regs; \
> +	return BSEARCH(offset, regs, ARRAY_SIZE(gen##x##_shadowed_regs), \
> +		       mmio_reg_cmp); \
>   }
>   
> +__is_genX_shadowed(8)
> +__is_genX_shadowed(11)
> +
>   #define __gen8_reg_write_fw_domains(offset) \
>   ({ \
>   	enum forcewake_domains __fwd; \
> @@ -929,6 +970,14 @@ static const struct intel_forcewake_range __chv_fw_ranges[] = {
>   	__fwd; \
>   })
>   
> +#define __gen11_fwtable_reg_write_fw_domains(offset) \
> +({ \
> +	enum forcewake_domains __fwd = 0; \
> +	if (GEN11_NEEDS_FORCE_WAKE((offset)) && !is_gen11_shadowed(offset)) \
> +		__fwd = find_fw_domain(dev_priv, offset); \
> +	__fwd; \
> +})
> +
>   /* *Must* be sorted by offset ranges! See intel_fw_table_check(). */
>   static const struct intel_forcewake_range __gen9_fw_ranges[] = {
>   	GEN_FW_RANGE(0x0, 0xaff, FORCEWAKE_BLITTER),
> @@ -965,6 +1014,40 @@ static const struct intel_forcewake_range __gen9_fw_ranges[] = {
>   	GEN_FW_RANGE(0x30000, 0x3ffff, FORCEWAKE_MEDIA),
>   };
>   
> +/* *Must* be sorted by offset ranges! See intel_fw_table_check(). */
> +static const struct intel_forcewake_range __gen11_fw_ranges[] = {
> +	GEN_FW_RANGE(0x0, 0xaff, FORCEWAKE_BLITTER),
> +	GEN_FW_RANGE(0xb00, 0x1fff, 0), /* uncore range */
> +	GEN_FW_RANGE(0x2000, 0x26ff, FORCEWAKE_RENDER),
> +	GEN_FW_RANGE(0x2700, 0x2fff, FORCEWAKE_BLITTER),
> +	GEN_FW_RANGE(0x3000, 0x3fff, FORCEWAKE_RENDER),
> +	GEN_FW_RANGE(0x4000, 0x51ff, FORCEWAKE_BLITTER),
> +	GEN_FW_RANGE(0x5200, 0x7fff, FORCEWAKE_RENDER),
> +	GEN_FW_RANGE(0x8000, 0x813f, FORCEWAKE_BLITTER),
> +	GEN_FW_RANGE(0x8140, 0x815f, FORCEWAKE_RENDER),
> +	GEN_FW_RANGE(0x8160, 0x82ff, FORCEWAKE_BLITTER),
> +	GEN_FW_RANGE(0x8300, 0x84ff, FORCEWAKE_RENDER),
> +	GEN_FW_RANGE(0x8500, 0x8bff, FORCEWAKE_BLITTER),
> +	GEN_FW_RANGE(0x8c00, 0x8cff, FORCEWAKE_RENDER),
> +	GEN_FW_RANGE(0x8d00, 0x93ff, FORCEWAKE_BLITTER),
> +	GEN_FW_RANGE(0x9400, 0x97ff, FORCEWAKE_ALL),
> +	GEN_FW_RANGE(0x9800, 0xafff, FORCEWAKE_BLITTER),
> +	GEN_FW_RANGE(0xb000, 0xb47f, FORCEWAKE_RENDER),
> +	GEN_FW_RANGE(0xb480, 0xdfff, FORCEWAKE_BLITTER),
> +	GEN_FW_RANGE(0xe000, 0xe8ff, FORCEWAKE_RENDER),
> +	GEN_FW_RANGE(0xe900, 0x243ff, FORCEWAKE_BLITTER),
> +	GEN_FW_RANGE(0x24400, 0x247ff, FORCEWAKE_RENDER),
> +	GEN_FW_RANGE(0x24800, 0x3ffff, FORCEWAKE_BLITTER),
> +	GEN_FW_RANGE(0x40000, 0x1bffff, 0),
> +	GEN_FW_RANGE(0x1c0000, 0x1c3fff, FORCEWAKE_MEDIA_VDBOX0),
> +	GEN_FW_RANGE(0x1c4000, 0x1c7fff, FORCEWAKE_MEDIA_VDBOX1),
> +	GEN_FW_RANGE(0x1c8000, 0x1cbfff, FORCEWAKE_MEDIA_VEBOX0),
> +	GEN_FW_RANGE(0x1cc000, 0x1cffff, FORCEWAKE_BLITTER),
> +	GEN_FW_RANGE(0x1d0000, 0x1d3fff, FORCEWAKE_MEDIA_VDBOX2),
> +	GEN_FW_RANGE(0x1d4000, 0x1d7fff, FORCEWAKE_MEDIA_VDBOX3),
> +	GEN_FW_RANGE(0x1d8000, 0x1dbfff, FORCEWAKE_MEDIA_VEBOX1)
> +};
> +
>   static void
>   ilk_dummy_write(struct drm_i915_private *dev_priv)
>   {
> @@ -1095,7 +1178,12 @@ func##_read##x(struct drm_i915_private *dev_priv, i915_reg_t reg, bool trace) {
>   }
>   #define __gen6_read(x) __gen_read(gen6, x)
>   #define __fwtable_read(x) __gen_read(fwtable, x)
> +#define __gen11_fwtable_read(x) __gen_read(gen11_fwtable, x)
>   
> +__gen11_fwtable_read(8)
> +__gen11_fwtable_read(16)
> +__gen11_fwtable_read(32)
> +__gen11_fwtable_read(64)
>   __fwtable_read(8)
>   __fwtable_read(16)
>   __fwtable_read(32)
> @@ -1105,6 +1193,7 @@ __gen6_read(16)
>   __gen6_read(32)
>   __gen6_read(64)
>   
> +#undef __gen11_fwtable_read
>   #undef __fwtable_read
>   #undef __gen6_read
>   #undef GEN6_READ_FOOTER
> @@ -1181,7 +1270,11 @@ func##_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, boo
>   }
>   #define __gen8_write(x) __gen_write(gen8, x)
>   #define __fwtable_write(x) __gen_write(fwtable, x)
> +#define __gen11_fwtable_write(x) __gen_write(gen11_fwtable, x)
>   
> +__gen11_fwtable_write(8)
> +__gen11_fwtable_write(16)
> +__gen11_fwtable_write(32)
>   __fwtable_write(8)
>   __fwtable_write(16)
>   __fwtable_write(32)
> @@ -1192,6 +1285,7 @@ __gen6_write(8)
>   __gen6_write(16)
>   __gen6_write(32)
>   
> +#undef __gen11_fwtable_write
>   #undef __fwtable_write
>   #undef __gen8_write
>   #undef __gen6_write
> @@ -1240,6 +1334,13 @@ static void fw_domain_init(struct drm_i915_private *dev_priv,
>   	BUILD_BUG_ON(FORCEWAKE_RENDER != (1 << FW_DOMAIN_ID_RENDER));
>   	BUILD_BUG_ON(FORCEWAKE_BLITTER != (1 << FW_DOMAIN_ID_BLITTER));
>   	BUILD_BUG_ON(FORCEWAKE_MEDIA != (1 << FW_DOMAIN_ID_MEDIA));
> +	BUILD_BUG_ON(FORCEWAKE_MEDIA_VDBOX0 != (1 << FW_DOMAIN_ID_MEDIA_VDBOX0));
> +	BUILD_BUG_ON(FORCEWAKE_MEDIA_VDBOX1 != (1 << FW_DOMAIN_ID_MEDIA_VDBOX1));
> +	BUILD_BUG_ON(FORCEWAKE_MEDIA_VDBOX2 != (1 << FW_DOMAIN_ID_MEDIA_VDBOX2));
> +	BUILD_BUG_ON(FORCEWAKE_MEDIA_VDBOX3 != (1 << FW_DOMAIN_ID_MEDIA_VDBOX3));
> +	BUILD_BUG_ON(FORCEWAKE_MEDIA_VEBOX0 != (1 << FW_DOMAIN_ID_MEDIA_VEBOX0));
> +	BUILD_BUG_ON(FORCEWAKE_MEDIA_VEBOX1 != (1 << FW_DOMAIN_ID_MEDIA_VEBOX1));
> +
>   
>   	d->mask = BIT(domain_id);
>   
> @@ -1267,7 +1368,34 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv)
>   		dev_priv->uncore.fw_clear = _MASKED_BIT_DISABLE(FORCEWAKE_KERNEL);
>   	}
>   
> -	if (INTEL_GEN(dev_priv) >= 9) {
> +	if (INTEL_GEN(dev_priv) >= 11) {
> +		int i;
> +
> +		dev_priv->uncore.funcs.force_wake_get = fw_domains_get;
> +		dev_priv->uncore.funcs.force_wake_put = fw_domains_put;
> +		fw_domain_init(dev_priv, FW_DOMAIN_ID_RENDER,
> +			       FORCEWAKE_RENDER_GEN9,
> +			       FORCEWAKE_ACK_RENDER_GEN9);
> +		fw_domain_init(dev_priv, FW_DOMAIN_ID_BLITTER,
> +			       FORCEWAKE_BLITTER_GEN9,
> +			       FORCEWAKE_ACK_BLITTER_GEN9);
> +		for (i = 0; i < I915_MAX_VCS; i++) {
> +			if (!HAS_ENGINE(dev_priv, _VCS(i)))
> +				continue;
> +
> +			fw_domain_init(dev_priv, FW_DOMAIN_ID_MEDIA_VDBOX0 + i,
> +				       FORCEWAKE_MEDIA_VDBOX_GEN11(i),
> +				       FORCEWAKE_ACK_MEDIA_VDBOX_GEN11(i));
> +		}
> +		for (i = 0; i < I915_MAX_VECS; i++) {
> +			if (!HAS_ENGINE(dev_priv, _VECS(i)))
> +				continue;
> +
> +			fw_domain_init(dev_priv, FW_DOMAIN_ID_MEDIA_VEBOX0 + i,
> +				       FORCEWAKE_MEDIA_VEBOX_GEN11(i),
> +				       FORCEWAKE_ACK_MEDIA_VEBOX_GEN11(i));
> +		}
> +	} else if (IS_GEN9(dev_priv) || IS_GEN10(dev_priv)) {
>   		dev_priv->uncore.funcs.force_wake_get =
>   			fw_domains_get_with_fallback;
>   		dev_priv->uncore.funcs.force_wake_put = fw_domains_put;
> @@ -1422,10 +1549,14 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
>   			ASSIGN_WRITE_MMIO_VFUNCS(dev_priv, gen8);
>   			ASSIGN_READ_MMIO_VFUNCS(dev_priv, gen6);
>   		}
> -	} else {
> +	} else if (IS_GEN(dev_priv, 9, 10)) {
>   		ASSIGN_FW_DOMAINS_TABLE(__gen9_fw_ranges);
>   		ASSIGN_WRITE_MMIO_VFUNCS(dev_priv, fwtable);
>   		ASSIGN_READ_MMIO_VFUNCS(dev_priv, fwtable);
> +	} else {
> +		ASSIGN_FW_DOMAINS_TABLE(__gen11_fw_ranges);
> +		ASSIGN_WRITE_MMIO_VFUNCS(dev_priv, gen11_fwtable);
> +		ASSIGN_READ_MMIO_VFUNCS(dev_priv, gen11_fwtable);
>   	}
>   
>   	iosf_mbi_register_pmic_bus_access_notifier(
> @@ -1985,7 +2116,9 @@ intel_uncore_forcewake_for_read(struct drm_i915_private *dev_priv,
>   	u32 offset = i915_mmio_reg_offset(reg);
>   	enum forcewake_domains fw_domains;
>   
> -	if (HAS_FWTABLE(dev_priv)) {
> +	if (INTEL_GEN(dev_priv) >= 11) {
> +		fw_domains = __gen11_fwtable_reg_read_fw_domains(offset);
> +	} else if (HAS_FWTABLE(dev_priv)) {
>   		fw_domains = __fwtable_reg_read_fw_domains(offset);
>   	} else if (INTEL_GEN(dev_priv) >= 6) {
>   		fw_domains = __gen6_reg_read_fw_domains(offset);
> @@ -2006,7 +2139,9 @@ intel_uncore_forcewake_for_write(struct drm_i915_private *dev_priv,
>   	u32 offset = i915_mmio_reg_offset(reg);
>   	enum forcewake_domains fw_domains;
>   
> -	if (HAS_FWTABLE(dev_priv) && !IS_VALLEYVIEW(dev_priv)) {
> +	if (INTEL_GEN(dev_priv) >= 11) {
> +		fw_domains = __gen11_fwtable_reg_write_fw_domains(offset);
> +	} else if (HAS_FWTABLE(dev_priv) && !IS_VALLEYVIEW(dev_priv)) {
>   		fw_domains = __fwtable_reg_write_fw_domains(offset);
>   	} else if (IS_GEN8(dev_priv)) {
>   		fw_domains = __gen8_reg_write_fw_domains(offset);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index bed019ef000f..9e8330c5808e 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -37,17 +37,36 @@ enum forcewake_domain_id {
>   	FW_DOMAIN_ID_RENDER = 0,
>   	FW_DOMAIN_ID_BLITTER,
>   	FW_DOMAIN_ID_MEDIA,
> +	FW_DOMAIN_ID_MEDIA_VDBOX0,
> +	FW_DOMAIN_ID_MEDIA_VDBOX1,
> +	FW_DOMAIN_ID_MEDIA_VDBOX2,
> +	FW_DOMAIN_ID_MEDIA_VDBOX3,
> +	FW_DOMAIN_ID_MEDIA_VEBOX0,
> +	FW_DOMAIN_ID_MEDIA_VEBOX1,
>   
>   	FW_DOMAIN_ID_COUNT
>   };
>   
>   enum forcewake_domains {
> -	FORCEWAKE_RENDER = BIT(FW_DOMAIN_ID_RENDER),
> -	FORCEWAKE_BLITTER = BIT(FW_DOMAIN_ID_BLITTER),
> -	FORCEWAKE_MEDIA	= BIT(FW_DOMAIN_ID_MEDIA),
> +	FORCEWAKE_RENDER	= BIT(FW_DOMAIN_ID_RENDER),
> +	FORCEWAKE_BLITTER	= BIT(FW_DOMAIN_ID_BLITTER),
> +	FORCEWAKE_MEDIA		= BIT(FW_DOMAIN_ID_MEDIA),
> +	FORCEWAKE_MEDIA_VDBOX0	= BIT(FW_DOMAIN_ID_MEDIA_VDBOX0),
> +	FORCEWAKE_MEDIA_VDBOX1	= BIT(FW_DOMAIN_ID_MEDIA_VDBOX1),
> +	FORCEWAKE_MEDIA_VDBOX2	= BIT(FW_DOMAIN_ID_MEDIA_VDBOX2),
> +	FORCEWAKE_MEDIA_VDBOX3	= BIT(FW_DOMAIN_ID_MEDIA_VDBOX3),
> +	FORCEWAKE_MEDIA_VEBOX0	= BIT(FW_DOMAIN_ID_MEDIA_VEBOX0),
> +	FORCEWAKE_MEDIA_VEBOX1	= BIT(FW_DOMAIN_ID_MEDIA_VEBOX1),
> +
>   	FORCEWAKE_ALL = (FORCEWAKE_RENDER |
>   			 FORCEWAKE_BLITTER |
> -			 FORCEWAKE_MEDIA)
> +			 FORCEWAKE_MEDIA |
> +			 FORCEWAKE_MEDIA_VDBOX0 |
> +			 FORCEWAKE_MEDIA_VDBOX1 |
> +			 FORCEWAKE_MEDIA_VDBOX2 |
> +			 FORCEWAKE_MEDIA_VDBOX3 |
> +			 FORCEWAKE_MEDIA_VEBOX0 |
> +			 FORCEWAKE_MEDIA_VEBOX1)

If I am not confused, this this could be simplified as:

  FORCEWAKE_ALL = BIT(FW_DOMAIN_ID_COUNT) - 1;

>   };
>   
>   struct intel_uncore_funcs {
> diff --git a/drivers/gpu/drm/i915/selftests/intel_uncore.c b/drivers/gpu/drm/i915/selftests/intel_uncore.c
> index 2f6367643171..f76f2597df5c 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_uncore.c
> @@ -61,20 +61,30 @@ static int intel_fw_table_check(const struct intel_forcewake_range *ranges,
>   
>   static int intel_shadow_table_check(void)
>   {
> -	const i915_reg_t *reg = gen8_shadowed_regs;
> -	unsigned int i;
> +	struct {
> +		const i915_reg_t *regs;
> +		unsigned int size;
> +	} reg_lists[] = {
> +		{ gen8_shadowed_regs, ARRAY_SIZE(gen8_shadowed_regs) },
> +		{ gen11_shadowed_regs, ARRAY_SIZE(gen11_shadowed_regs) },
> +	};
> +	const i915_reg_t *reg;
> +	unsigned int i, j;
>   	s32 prev;
>   
> -	for (i = 0, prev = -1; i < ARRAY_SIZE(gen8_shadowed_regs); i++, reg++) {
> -		u32 offset = i915_mmio_reg_offset(*reg);
> +	for (j = 0; j < ARRAY_SIZE(reg_lists); ++j) {
> +		reg = reg_lists[j].regs;
> +		for (i = 0, prev = -1; i < reg_lists[j].size; i++, reg++) {
> +			u32 offset = i915_mmio_reg_offset(*reg);
>   
> -		if (prev >= (s32)offset) {
> -			pr_err("%s: entry[%d]:(%x) is before previous (%x)\n",
> -			       __func__, i, offset, prev);
> -			return -EINVAL;
> -		}
> +			if (prev >= (s32)offset) {
> +				pr_err("%s: entry[%d]:(%x) is before previous (%x)\n",
> +				       __func__, i, offset, prev);
> +				return -EINVAL;
> +			}
>   
> -		prev = offset;
> +			prev = offset;
> +		}
>   	}
>   
>   	return 0;
> @@ -90,6 +100,7 @@ int intel_uncore_mock_selftests(void)
>   		{ __vlv_fw_ranges, ARRAY_SIZE(__vlv_fw_ranges), false },
>   		{ __chv_fw_ranges, ARRAY_SIZE(__chv_fw_ranges), false },
>   		{ __gen9_fw_ranges, ARRAY_SIZE(__gen9_fw_ranges), true },
> +		{ __gen11_fw_ranges, ARRAY_SIZE(__gen11_fw_ranges), true },
>   	};
>   	int err, i;
>   

I haven't checked the ranges, but the code looks good. With or without 
the nitpicks:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

Regards,

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