public inbox for intel-gfx@lists.freedesktop.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox