[Intel-gfx] [PATCH v3 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Chris Wilson]

On Wed, Jan 13, 2016 at 07:14:56PM +0000, Arun Siluvery wrote:
> On 13/01/2016 19:01, Chris Wilson wrote:
> >On Wed, Jan 13, 2016 at 03:38:15PM +0000, Arun Siluvery wrote:
> >>Some of the HW registers are privileged and cannot be written to from
> >>non-privileged batch buffers coming from userspace unless they are added to
> >>the HW whitelist. This whitelist is maintained by HW and it is different from
> >>SW whitelist. Userspace need write access to them to implement preemption
> >>related WA.
> >>
> >>The reason for using this approach is, the register bits that control
> >>preemption granularity at the HW level are not context save/restored; so even
> >>if we set these bits always in kernel they are going to change once the
> >>context is switched out.  We can consider making them non-privileged by
> >>default but these registers also contain other chicken bits which should not
> >>be allowed to be modified.
> >>
> >>In the later revisions controlling bits are save/restored at context level but
> >>in the existing revisions these are exported via other debug registers and
> >>should be on the whitelist. This patch adds changes to provide HW with a list
> >>of registers to be whitelisted. HW checks this list during execution and
> >>provides access accordingly.
> >>
> >>HW imposes a limit on the number of registers on whitelist and it is
> >>per-engine.  At this point we are only enabling whitelist for RCS and we don't
> >>foresee any requirement for other engines.
> >>
> >>The registers to be whitelisted are added using generic workaround list
> >>mechanism, even these are only enablers for userspace workarounds. But by
> >>sharing this mechanism we get some test assets without additional cost (Mika).
> >>
> >>v2: rebase
> >>
> >>v3: parameterize RING_FORCE_TO_NONPRIV() as _MMIO() should be limited to
> >>i915_reg.h (Ville), drop inline for wa_ring_whitelist_reg (Mika).
> >>
> >>v4: Clarify that this is HW whitelist and different from the one maintained in
> >>driver. This list is engine specific but it gets initialized along with other
> >>WA which is RCS specific thing, so make it clear that we are not doing any
> >>cross engine setup during initialization (Chris).
> >
> >Those name work much better for me, so thanks for clearing them up and
> >allaying my fears.
> >
> >Would it not also make sense to expose hw_whitelist_count[] in
> >i915_wa_registers (debugfs)?
> >
> It is already is part of i915_wa_registers, each HW whitelist entry
> is just another entry in i915_workarounds. Mika suggested to add
> this using workaround list mechanism so that we get this without
> additional cost.

I just mean that is much easier to sanity check kernel internals if we
print them out in debugfs as well. Yes, you can inspect the value of
each register and determine which are whitelist enabling - but wouldn't
it also be good to check that you have the same count as the kernel. For
mere mortals, we can just look at the count and be happy that
whitelisting is taking effect.
 
> >>+static int wa_ring_whitelist_reg(struct intel_engine_cs *ring,
> >>+				 i915_reg_t reg_addr)
> >>+{
> >>+	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> >>+	struct i915_workarounds *wa = &dev_priv->workarounds;
> >>+	const uint32_t index = wa->hw_whitelist_count[ring->id];
> >>+
> >>+	if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
> >>+		return -EINVAL;
> >>+
> >>+	WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base, index), reg_addr.reg);
> >
> >WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base, index),
> >          i915_mmio_reg_offset(reg_addr));
> >
> >And just call it reg. (reg_addr would imply that you applied the mmio
> >offset, i.e were about to call ioread32(reg_addr)).
> the thought of using i915_mmio_reg_offset() came to me after sending
> v3 but thought no one would notice :)
> Do you want me to change this and send again?

If you have a moment, do so with my
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre