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

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

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.

> 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