[Intel-gfx] [PATCH] drm/i915: Implement read-only support in whitelist selftest

Tvrtko Ursulin tvrtko.ursulin at linux.intel.com
Wed Jun 19 06:49:18 UTC 2019


On 18/06/2019 20:54, John.C.Harrison at Intel.com wrote:
> From: John Harrison <John.C.Harrison at Intel.com>
> 
> Newer hardware supports extra feature in the whitelist registers. This
> patch updates the selftest to test that entries marked as read only
> are actually read only.
> 
> 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   |  8 +--
>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 53 +++++++++++++------
>   drivers/gpu/drm/i915/i915_reg.h               |  9 ++--
>   3 files changed, 48 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 93caa7b6d7a9..4429ee08b20e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1028,7 +1028,7 @@ whitelist_reg_ext(struct i915_wa_list *wal, i915_reg_t reg, u32 flags)

Reminder to add a GEM_BUG_ON if invalid flags are passed in.

>   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)
> @@ -1134,13 +1134,13 @@ printk(KERN_INFO "%-32s:%4d> Boo! [engine = %s, instance = %d, base = 0x%X, reg
>   
>   		/* 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 eb6d3aa2c8cc..a0a88ec66861 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -399,6 +399,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)
> @@ -410,7 +414,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;
> @@ -482,12 +487,12 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   		u32 srm, lrm, rsvd;
>   		u32 expect;
>   		int idx;
> +		bool ro_reg;
>   
>   		if (wo_register(engine, reg))
>   			continue;
>   
> -		if (ro_register(reg))
> -			continue;
> +		ro_reg = ro_register(reg);
>   
>   		srm = MI_STORE_REGISTER_MEM;
>   		lrm = MI_LOAD_REGISTER_MEM;
> @@ -588,24 +593,36 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   		}
>   
>   		GEM_BUG_ON(values[ARRAY_SIZE(values) - 1] != 0xffffffff);
> -		rsvd = results[ARRAY_SIZE(values)]; /* detect write masking */
> -		if (!rsvd) {
> -			pr_err("%s: Unable to write to whitelisted register %x\n",
> -			       engine->name, reg);
> -			err = -EINVAL;
> -			goto out_unpin;
> +		if (ro_reg) {
> +			rsvd = 0xFFFFFFFF;

I guess it doesn't matter what value you set since it is only used on 
pr_info on ro_reg paths?

> +		} else {
> +			rsvd = results[ARRAY_SIZE(values)]; /* detect write masking */
> +			if (!rsvd) {
> +				pr_err("%s: Unable to write to whitelisted register %x\n",
> +				       engine->name, reg);
> +				err = -EINVAL;
> +				goto out_unpin;
> +			}
>   		}
>   
>   		expect = results[0];
>   		idx = 1;
>   		for (v = 0; v < ARRAY_SIZE(values); v++) {
> -			expect = reg_write(expect, values[v], rsvd);
> +			if (ro_reg)
> +				expect = results[0];
> +			else
> +				expect = reg_write(expect, values[v], rsvd);
> +
>   			if (results[idx] != expect)
>   				err++;
>   			idx++;
>   		}
>   		for (v = 0; v < ARRAY_SIZE(values); v++) {
> -			expect = reg_write(expect, ~values[v], rsvd);
> +			if (ro_reg)
> +				expect = results[0];
> +			else
> +				expect = reg_write(expect, ~values[v], rsvd);
> +
>   			if (results[idx] != expect)
>   				err++;
>   			idx++;
> @@ -622,7 +639,10 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   			for (v = 0; v < ARRAY_SIZE(values); v++) {
>   				u32 w = values[v];
>   
> -				expect = reg_write(expect, w, rsvd);
> +				if (ro_reg)
> +					expect = results[0];
> +				else
> +					expect = reg_write(expect, w, rsvd);
>   				pr_info("Wrote %08x, read %08x, expect %08x\n",
>   					w, results[idx], expect);
>   				idx++;
> @@ -630,7 +650,10 @@ static int check_dirty_whitelist(struct i915_gem_context *ctx,
>   			for (v = 0; v < ARRAY_SIZE(values); v++) {
>   				u32 w = ~values[v];
>   
> -				expect = reg_write(expect, w, rsvd);
> +				if (ro_reg)
> +					expect = results[0];
> +				else
> +					expect = reg_write(expect, w, rsvd);
>   				pr_info("Wrote %08x, read %08x, expect %08x\n",
>   					w, results[idx], expect);
>   				idx++;
> @@ -762,8 +785,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;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index cc295a4f6e92..ba22e3b8f77e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2513,13 +2513,16 @@ 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_MAX_NONPRIV_SLOTS  12
>   
>   #define GEN7_TLB_RD_ADDR	_MMIO(0x4700)
> 

This in fact looks fine to me.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko


More information about the Intel-gfx mailing list