All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: John.C.Harrison@Intel.com, Intel-GFX@Lists.FreeDesktop.Org
Subject: Re: [PATCH 1/2] drm/i915: Add test for invalid flag bits in whitelist entries
Date: Wed, 3 Jul 2019 14:50:37 +0100	[thread overview]
Message-ID: <cc0074e3-e0f4-be24-023a-e107aa7ae948@linux.intel.com> (raw)
In-Reply-To: <20190703020604.20369-2-John.C.Harrison@Intel.com>


On 03/07/2019 03:06, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@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@Intel.com>
> CC: Tvrtko Ursulin <tvrtko.ursulin@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, &gt->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@intel.com>

Regards,

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

  reply	other threads:[~2019-07-03 13:50 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-03  2:06 [PATCH 0/2] Improve whitelist selftest for read-only registers John.C.Harrison
2019-07-03  2:06 ` [PATCH 1/2] drm/i915: Add test for invalid flag bits in whitelist entries John.C.Harrison
2019-07-03 13:50   ` Tvrtko Ursulin [this message]
2019-07-03  2:06 ` [PATCH 2/2] drm/i915: Implement read-only support in whitelist selftest John.C.Harrison
2019-07-03  8:32   ` Chris Wilson
2019-07-03 19:43     ` John Harrison
2019-07-10  8:21       ` Tvrtko Ursulin
2019-07-04 10:10   ` Tvrtko Ursulin
2019-07-03  2:43 ` ✓ Fi.CI.BAT: success for Improve whitelist selftest for read-only registers Patchwork
2019-07-03  7:50 ` [PATCH 3/3] drm/i915: Add engine name to workaround debug print John.C.Harrison
2019-07-03 13:53   ` Tvrtko Ursulin
2019-07-03  9:36 ` ✓ Fi.CI.BAT: success for Improve whitelist selftest for read-only registers Patchwork
2019-07-03 23:48 ` ✓ Fi.CI.IGT: " Patchwork
2019-07-04  2:31 ` Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cc0074e3-e0f4-be24-023a-e107aa7ae948@linux.intel.com \
    --to=tvrtko.ursulin@linux.intel.com \
    --cc=Intel-GFX@Lists.FreeDesktop.Org \
    --cc=John.C.Harrison@Intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.