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

[Chris Wilson]

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)?

> +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)).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre