Re: [PATCH v2] drm/i915: Check whitelist registers across resets

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Oscar Mateo (2018-04-13 18:04:16)
> 
> 
> 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...

Oh no we weren't. wa_ring_whitelist_reg() isn't storing the reg in the
wa list, just that we have used the RING_NONPRIV slot. At one point, I
think the intention was there but that seems to disappeared and now I
removed the notion entirely ;)

> > 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.

Right, the long term goal will be to move all the workaround testing
here. It just so happens that I wrote a buggy patch that CI was happy
with that alerted me to the lack of testing ;)

The only fly in the ointment is doing S3/S4 testing, as doing
suspend/resume from inside the kernel tricky (trickier than even getting
it right from userspace). So I think we may just have to be a little
more creative, and do something like call i915_drv_suspend() directly.
Hmm, now that's an idea. (i915_drv_suspend(); scribble over state;
i915_drv_resume()).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx