[Intel-gfx] [PATCH 1/2] drm/i915: Add test for invalid flag bits in whitelist entries
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Wed Jul 3 13:50:37 UTC 2019
On 03/07/2019 03:06, John.C.Harrison at Intel.com wrote:
> From: John Harrison <John.C.Harrison at Intel.com>
>
> As per review feedback by Tvrtko, added a check that no invalid bits
> are being set in the whitelist flags fields.
>
> Also updated the read/write access definitions to make it clearer that
> they are an enum field not a set of single bit flags.
>
> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
> CC: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
> drivers/gpu/drm/i915/gt/intel_workarounds.c | 29 +++++++++++++++----
> .../gpu/drm/i915/gt/selftest_workarounds.c | 14 ++++++---
> drivers/gpu/drm/i915/i915_reg.h | 12 ++++++--
> 3 files changed, 42 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index a908d829d6bd..9b401833aceb 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1011,6 +1011,20 @@ bool intel_gt_verify_workarounds(struct intel_gt *gt, const char *from)
> return wa_list_verify(gt->uncore, >->i915->gt_wa_list, from);
> }
>
> +static inline bool is_nonpriv_flags_valid(u32 flags)
> +{
> + /* Check only valid flag bits are set */
> + if (flags & ~RING_FORCE_TO_NONPRIV_MASK_VALID)
> + return false;
> +
> + /* NB: Only 3 out of 4 enum values are valid for access field */
> + if ((flags & RING_FORCE_TO_NONPRIV_ACCESS_MASK) ==
> + RING_FORCE_TO_NONPRIV_ACCESS_INVALID)
> + return false;
> +
> + return true;
> +}
> +
> static void
> whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)
> {
> @@ -1021,6 +1035,9 @@ whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)
> if (GEM_DEBUG_WARN_ON(wal->count >= RING_MAX_NONPRIV_SLOTS))
> return;
>
> + if (GEM_DEBUG_WARN_ON(!is_nonpriv_flags_valid(flags)))
> + return;
> +
> wa.reg.reg |= flags;
> _wa_add(wal, &wa);
> }
> @@ -1028,7 +1045,7 @@ whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)
> static void
> whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
> {
> - whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_RW);
> + whitelist_reg_ext(wal, reg, RING_FORCE_TO_NONPRIV_ACCESS_RW);
> }
>
> static void gen9_whitelist_build(struct i915_wa_list *w)
> @@ -1109,7 +1126,7 @@ static void cfl_whitelist_build(struct intel_engine_cs *engine)
> * - PS_DEPTH_COUNT_UDW
> */
> whitelist_reg_ext(w, PS_INVOCATION_COUNT,
> - RING_FORCE_TO_NONPRIV_RD |
> + RING_FORCE_TO_NONPRIV_ACCESS_RD |
> RING_FORCE_TO_NONPRIV_RANGE_4);
> }
>
> @@ -1149,20 +1166,20 @@ static void icl_whitelist_build(struct intel_engine_cs *engine)
> * - PS_DEPTH_COUNT_UDW
> */
> whitelist_reg_ext(w, PS_INVOCATION_COUNT,
> - RING_FORCE_TO_NONPRIV_RD |
> + RING_FORCE_TO_NONPRIV_ACCESS_RD |
> RING_FORCE_TO_NONPRIV_RANGE_4);
> break;
>
> case VIDEO_DECODE_CLASS:
> /* hucStatusRegOffset */
> whitelist_reg_ext(w, _MMIO(0x2000 + engine->mmio_base),
> - RING_FORCE_TO_NONPRIV_RD);
> + RING_FORCE_TO_NONPRIV_ACCESS_RD);
> /* hucUKernelHdrInfoRegOffset */
> whitelist_reg_ext(w, _MMIO(0x2014 + engine->mmio_base),
> - RING_FORCE_TO_NONPRIV_RD);
> + RING_FORCE_TO_NONPRIV_ACCESS_RD);
> /* hucStatus2RegOffset */
> whitelist_reg_ext(w, _MMIO(0x23B0 + engine->mmio_base),
> - RING_FORCE_TO_NONPRIV_RD);
> + RING_FORCE_TO_NONPRIV_ACCESS_RD);
> break;
>
> default:
> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index b933d831eeb1..f8151d5946c8 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -394,6 +394,10 @@ static bool wo_register(struct intel_engine_cs *engine, u32 reg)
> enum intel_platform platform = INTEL_INFO(engine->i915)->platform;
> int i;
>
> + if ((reg & RING_FORCE_TO_NONPRIV_ACCESS_MASK) ==
> + RING_FORCE_TO_NONPRIV_ACCESS_WR)
> + return true;
> +
> for (i = 0; i < ARRAY_SIZE(wo_registers); i++) {
> if (wo_registers[i].platform == platform &&
> wo_registers[i].reg == reg)
> @@ -405,7 +409,8 @@ static bool wo_register(struct intel_engine_cs *engine, u32 reg)
>
> static bool ro_register(u32 reg)
> {
> - if (reg & RING_FORCE_TO_NONPRIV_RD)
> + if ((reg & RING_FORCE_TO_NONPRIV_ACCESS_MASK) ==
> + RING_FORCE_TO_NONPRIV_ACCESS_RD)
> return true;
>
> return false;
> @@ -757,8 +762,8 @@ static int read_whitelisted_registers(struct i915_gem_context *ctx,
> u64 offset = results->node.start + sizeof(u32) * i;
> u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>
> - /* Clear RD only and WR only flags */
> - reg &= ~(RING_FORCE_TO_NONPRIV_RD | RING_FORCE_TO_NONPRIV_WR);
> + /* Clear access permission field */
> + reg &= ~RING_FORCE_TO_NONPRIV_ACCESS_MASK;
>
> *cs++ = srm;
> *cs++ = reg;
> @@ -928,7 +933,8 @@ check_whitelisted_registers(struct intel_engine_cs *engine,
> for (i = 0; i < engine->whitelist.count; i++) {
> const struct i915_wa *wa = &engine->whitelist.list[i];
>
> - if (i915_mmio_reg_offset(wa->reg) & RING_FORCE_TO_NONPRIV_RD)
> + if (i915_mmio_reg_offset(wa->reg) &
> + RING_FORCE_TO_NONPRIV_ACCESS_RD)
> continue;
>
> if (!fn(engine, a[i], b[i], wa->reg))
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c814cc1b3ae5..0bd4bf0b4629 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2521,13 +2521,19 @@ enum i915_power_well_id {
> #define RING_WAIT_SEMAPHORE (1 << 10) /* gen6+ */
>
> #define RING_FORCE_TO_NONPRIV(base, i) _MMIO(((base) + 0x4D0) + (i) * 4)
> -#define RING_FORCE_TO_NONPRIV_RW (0 << 28) /* CFL+ & Gen11+ */
> -#define RING_FORCE_TO_NONPRIV_RD (1 << 28)
> -#define RING_FORCE_TO_NONPRIV_WR (2 << 28)
> +#define RING_FORCE_TO_NONPRIV_ACCESS_RW (0 << 28) /* CFL+ & Gen11+ */
> +#define RING_FORCE_TO_NONPRIV_ACCESS_RD (1 << 28)
> +#define RING_FORCE_TO_NONPRIV_ACCESS_WR (2 << 28)
> +#define RING_FORCE_TO_NONPRIV_ACCESS_INVALID (3 << 28)
> +#define RING_FORCE_TO_NONPRIV_ACCESS_MASK (3 << 28)
> #define RING_FORCE_TO_NONPRIV_RANGE_1 (0 << 0) /* CFL+ & Gen11+ */
> #define RING_FORCE_TO_NONPRIV_RANGE_4 (1 << 0)
> #define RING_FORCE_TO_NONPRIV_RANGE_16 (2 << 0)
> #define RING_FORCE_TO_NONPRIV_RANGE_64 (3 << 0)
> +#define RING_FORCE_TO_NONPRIV_RANGE_MASK (3 << 0)
> +#define RING_FORCE_TO_NONPRIV_MASK_VALID \
> + (RING_FORCE_TO_NONPRIV_RANGE_MASK \
> + | RING_FORCE_TO_NONPRIV_ACCESS_MASK)
> #define RING_MAX_NONPRIV_SLOTS 12
>
> #define GEN7_TLB_RD_ADDR _MMIO(0x4700)
>
Looks okay. the only nitpick I have is that "is flags" reads funny ("are
flags"?), but maybe it is just because I am not a native speaker.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
Regards,
Tvrtko
More information about the Intel-gfx
mailing list