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

[Arun Siluvery]

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.

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

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

regards
Arun


> -Chris
>