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

Michel Thierry
2018-02-01T16:02:37Z

On 2/1/2018 2:25 AM, Tvrtko Ursulin wrote:
>
> 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.
>

I'd go for a new blank line, it makes it obvious something changed
between gens.

>>   #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;
>

Yes, that's the same (and looks much better).

>>   };
>>   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:
>

Both are fair improvements, I'll resend it with these changes.

Thanks!

> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Regards,
>
> Tvrtko