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

Tue, 02 Jul 2019 19:21:04 -0700 

Patches sent.

I haven't made any changes to dmesg output as I'm not sure what you mean.
Ah, do you mean the debug print in wa_init_finish()? Sure, I can add the engine name to that. 

John.


On 6/25/2019 01:33, Tvrtko Ursulin wrote:


Ping.

We agreed to follow up with a test ASAP after merging.
Here's another feature request for you: Add engine->name logging to wa_init_start in intel_engine_init_whitelist. Because with the change to add whitelist on other engines there are now multiple identical lines in dmesg. 

To sum up that's three todo items:

1. Resend the selftest for CI.
2. Add GEM_BUG_ON for reg->flags checking invalid flag usage.
3. Improve dmesg so we know which engine got how many whitelist entries.

Thanks,

Tvrtko

On 20/06/2019 16:43, Tvrtko Ursulin wrote:


Hi,
You will need to send this not as reply to this thread so it is picked up by CI and then can be merged.
But please also add a patch which adds that GEM_BUG_ON reg->flags check we talked about. 

Regards,

Tvrtko

On 18/06/2019 20:54, john.c.harri...@intel.com wrote:

From: John Harrison <john.c.harri...@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.harri...@intel.com>
CC: Tvrtko Ursulin <tvrtko.ursu...@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) 
  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to