Re: [PATCH 4/4] drm/i915: Update workarounds selftest for read only regs

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

On 18/06/2019 14:43, John Harrison wrote:
> On 6/17/2019 23:42, Tvrtko Ursulin wrote:
>> On 18/06/2019 02:01, John.C.Harrison@Intel.com wrote:
>>> From: "Robert M. Fosha" <robert.m.fosha@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@intel.com>
>>> Signed-off-by: John Harrison <John.C.Harrison@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.

I don't agree that it is just belt and braces but in fact a way to check 
that the thing really is read-only like it says on the tin. Lesson here 
is that history keeps repeating panics which could have been avoided by 
running the pre-existing tests during development.

However since I don't think there is any risk to driver or system 
stability, even if the read-only property happened to be broken, which 
it probably isn't, you can have a grumpy:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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