[Intel-gfx] [PATCH 4/4] drm/i915: Update workarounds selftest for read only regs

[John Harrison]

On 6/17/2019 23:42, Tvrtko Ursulin wrote:
> On 18/06/2019 02:01, John.C.Harrison at Intel.com wrote:
>> From: "Robert M. Fosha" <robert.m.fosha at intel.com>
>>
>> Updates the live_workarounds selftest to handle whitelisted
>> registers that are flagged as read only.
>>
>> Signed-off-by: Robert M. Fosha <robert.m.fosha at intel.com>
>> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
>> ---
>>   .../gpu/drm/i915/gt/selftest_workarounds.c    | 43 +++++++++++++++++--
>>   1 file changed, 39 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c 
>> b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> index c8d335d63f9c..eb6d3aa2c8cc 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
>> @@ -408,6 +408,29 @@ static bool wo_register(struct intel_engine_cs 
>> *engine, u32 reg)
>>       return false;
>>   }
>>   +static bool ro_register(u32 reg)
>> +{
>> +    if (reg & RING_FORCE_TO_NONPRIV_RD)
>> +        return true;
>> +
>> +    return false;
>> +}
>> +
>> +static int whitelist_writable_count(struct intel_engine_cs *engine)
>> +{
>> +    int count = engine->whitelist.count;
>> +    int i;
>> +
>> +    for (i = 0; i < engine->whitelist.count; i++) {
>> +        u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>> +
>> +        if (ro_register(reg))
>> +            count--;
>> +    }
>> +
>> +    return count;
>> +}
>> +
>>   static int check_dirty_whitelist(struct i915_gem_context *ctx,
>>                    struct intel_engine_cs *engine)
>>   {
>> @@ -463,6 +486,9 @@ static int check_dirty_whitelist(struct 
>> i915_gem_context *ctx,
>>           if (wo_register(engine, reg))
>>               continue;
>>   +        if (ro_register(reg))
>> +            continue;
>> +
>>           srm = MI_STORE_REGISTER_MEM;
>>           lrm = MI_LOAD_REGISTER_MEM;
>>           if (INTEL_GEN(ctx->i915) >= 8)
>> @@ -734,9 +760,13 @@ static int read_whitelisted_registers(struct 
>> i915_gem_context *ctx,
>>         for (i = 0; i < engine->whitelist.count; i++) {
>>           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);
>>             *cs++ = srm;
>> -        *cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>> +        *cs++ = reg;
>>           *cs++ = lower_32_bits(offset);
>>           *cs++ = upper_32_bits(offset);
>>       }
>> @@ -769,9 +799,14 @@ static int scrub_whitelisted_registers(struct 
>> i915_gem_context *ctx,
>>           goto err_batch;
>>       }
>>   -    *cs++ = MI_LOAD_REGISTER_IMM(engine->whitelist.count);
>> +    *cs++ = MI_LOAD_REGISTER_IMM(whitelist_writable_count(engine));
>>       for (i = 0; i < engine->whitelist.count; i++) {
>> -        *cs++ = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>> +        u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg);
>> +
>> +        if (ro_register(reg))
>> +            continue;
>> +
>
> Are we not able to test the read-only property at all?
>
I am sure it would be possible to make such work. But can that wait 
until the next round when we add support for ranges? And write only 
access too if any registers actually use that and there is a way to test 
that it really does do something?

I believe Robert was looking at getting something going but it wasn't 
immediately working and we urgently need to get the HUC whitelist 
updates merged to hit the next release window. So right now, it is 
sufficient to say that the user land media driver works with these 
changes and therefore the whitelisting must be working. The kernel 
selftest is just a belt and braces check on top of that and therefore 
can wait until later.

John.