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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 03/07/2019 20:43, John Harrison wrote:
> On 7/3/2019 01:32, Chris Wilson wrote:
>> Quoting John.C.Harrison@Intel.com (2019-07-03 03:06:04)
>>> 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.
>>>
>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>> CC: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 43 +++++++++++++------
>>>   1 file changed, 31 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c 
>>> b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>>> index f8151d5946c8..5cd2b17105ba 100644
>>> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>>> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>>> @@ -482,12 +482,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 +588,37 @@ 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;
>> rsvd = 0;
>>
>> reg_write() will then dtrt.
> It seemed too suspiciously broken to have the test claim a read-only 
> register was successfully written to. This way makes it clear that the 
> test expects read-only to always return the first value read.

I suggest we go with this version if it is not too-disagreeable. Chris?

John can only hope it still applies.

Regards,

Tvrtko

>> Does this not replace the skip placed in check_whitelisted_registers()?
> The two versions of that test looks like they need to be able to set 
> values. So they can't be run on read-only registers.
> 
>> We still need a way to verify that the register exists, as even writing
>> from a secure batch fails (not tried ring though). Do we load a spinner,
>> tweak via mmio?
> 
> I don't think there is a reliable, generic mechanism to test that you 
> can actually read from a read only register. You need to know what 
> content it should provide. Even the current test (that it always returns 
> the same value) would break if the register changes dynamically (e.g. 
> it's a hardware counter).
> 
> John.
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx