| Message ID | 20190618195421.31027-1-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Implement read-only support in whitelist selftest | expand |
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@Intel.com wrote: > From: John Harrison <John.C.Harrison@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@Intel.com> > CC: Tvrtko Ursulin <tvrtko.ursulin@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)
On 18/06/2019 21:08, John Harrison wrote: > 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? I was thinking we just read the register and then verify it is unchanged after every existing write to it. Regards, Tvrtko
On 18/06/2019 20:54, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@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@Intel.com> > CC: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com> Regards, Tvrtko
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.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@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@Intel.com> > CC: Tvrtko Ursulin <tvrtko.ursulin@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) >
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.Harrison@Intel.com wrote: >> From: John Harrison <John.C.Harrison@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@Intel.com> >> CC: Tvrtko Ursulin <tvrtko.ursulin@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) >>
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.Harrison@Intel.com wrote: >>> From: John Harrison <John.C.Harrison@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@Intel.com> >>> CC: Tvrtko Ursulin <tvrtko.ursulin@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) >>>
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)