[Intel-gfx] [PATCH v2] drm/i915: Check whitelist registers across resets

[Oscar Mateo]

On 4/13/2018 9:54 AM, Chris Wilson wrote:
> Quoting Oscar Mateo (2018-04-13 17:46:42)
>>
>> On 4/12/2018 8:21 AM, Chris Wilson wrote:
>>> Add a selftest to ensure that we restore the whitelisted registers after
>>> rewrite the registers everytime they might be scrubbed, e.g. module
>>> load, reset and resume. For the other volatile workaround registers, we
>>> export their presence via debugfs and check in igt/gem_workarounds.
>>> However, we don't export the whitelist and rather than do so, let's test
>>> them directly in the kernel.
>> I guess my question is... why? what was the problem with exporting the
>> list of whitelist registers in debugfs?
> We don't... (There's no RING_NONPRIV checking currently)

There is no checking, but we were showing the full list in debugfs. Ok, 
I guess it wasn't that useful without the corresponding igt...

> I wasn't fond
> of the igt for it is checking kernel implementation rather than behaviour.
> The kernel gives it a checklist which it dutifully follows... Now that
> we have selftests, we don't need to write what I think should be unit
> tests in igt anymore.

Ah, so I take the plan is to also check the other WAs in selftests? 
Somehow I thought you wanted to treat whitelisting differently.

>>> The test we use is to read the registers back from the CS (this helps us
>>> be sure that the registers will be valid for MI_LRI etc). In order to
>>> generate the expected list, we split intel_whitelist_workarounds_emit
>>> into two phases, the first to build the list and the second to apply.
>>> Inside the test, we only build the list and then check that list against
>>> the hw.
>>>
>>> v2: Filter out pre-gen8 as they do not have RING_NONPRIV.
>>>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Oscar Mateo <oscar.mateo at intel.com>
>>> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>>> ---
>>> +static struct whitelist *whitelist_build(struct intel_engine_cs *engine,
>>> +                                      struct whitelist *w)
>>> +{
>>> +     struct drm_i915_private *i915 = engine->i915;
>>> +
>>> +     GEM_BUG_ON(engine->id != RCS);
>>> +
>>> +     w->count = 0;
>>> +     w->nopid = i915_mmio_reg_offset(RING_NOPID(engine->mmio_base));
>> :)
>>
>>> +
>>> +     if (INTEL_GEN(i915) < 8)
>>> +             return NULL;
>>> +     else if (IS_BROADWELL(i915))
>>> +             bdw_whitelist_build(engine, w);
>> Is it worth passing the engine around? Even of we end up with
>> whitelisted register in engines != RCS, we will need more changes than this.
> No idea, it was easy enough to pass around, so I did just in case it was
> useful in future (pulling out i915 or whatever).
> -Chris

I'd say let's cross that bridge when we get to it, but with or without it:

Reviewed-by: Oscar Mateo <oscar.mateo at intel.com>