[Intel-gfx] [PATCH] drm/i915: Implement read-only support in whitelist selftest
John Harrison
John.C.Harrison at Intel.com
Wed Jul 3 02:20:14 UTC 2019
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 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