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