[Intel-gfx] [PATCH] drm/i915: Implement read-only support in whitelist selftest
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Tue Jun 25 08:33:00 UTC 2019
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 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)
>> 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)
>>
More information about the Intel-gfx
mailing list