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

[John Harrison <John.C.Harrison@Intel.com>]

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)
>>>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx