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

Tvrtko Ursulin
2018-02-01T10:25:42Z

On 01/02/2018 00:52, Michel Thierry wrote:
> From: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>
> 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@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Acked-by: Michel Thierry <michel.thierry@intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> ---
>
> 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@intel.com>

Regards,

Tvrtko