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

John Harrison <John.C.Harrison@xxxxxxxxx> · Tue, 18 Jun 2019 13:08:39 -0700

Tvrtko, does this look plausible?

It seems to work for me in that it passes on ICL with the new read-only 
registers. I'm not sure if there is a valid way to detect whether the 
registers are actually readable though. How would the test know what is 
a valid value? If one assumes that one gets back zero from an invalid 
read, how does one know that the read-only register is not supposed to 
return zero at this point anyway?

Or is it worth just putting in a test for non-zero and if we do find a 
register that can validly return zero then we special case that one and 
ignore it?

John.


On 6/18/2019 12:54, John.C.Harrison@xxxxxxxxx wrote:
From: John Harrison <John.C.Harrison@xxxxxxxxx>

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@xxxxxxxxx>
CC: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
  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)
  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;
+		} 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)

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







