Re: [PATCH 1/2] drm/i915/gen9: Add framework to whitelist specific GPU registers

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Arun Siluvery <arun.siluvery@linux.intel.com> writes:

> Some of the HW registers are privileged and cannot be written to from a
> non-privileged batch buffers coming from userspace unless they are on whitelist.
> Userspace need write access to them to implement mainly preemption related WA and
> also some general 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).
>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Nick Hoath <nicholas.hoath@intel.com>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  9 ++++++++-
>  drivers/gpu/drm/i915/i915_reg.h         |  2 ++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 17 +++++++++++++++++
>  3 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9c10270..44fbd1d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1674,11 +1674,18 @@ struct i915_wa_reg {
>  	u32 mask;
>  };
>  
> -#define I915_MAX_WA_REGS 16
> +/*
> + * RING_MAX_NONPRIV_SLOTS is per-engine but at this point we are only
> + * allowing it for RCS as we don't foresee any requirement of having
> + * a whitelist for other engines. When it is really required for
> + * other engines then the limit need to be increased.
> + */
> +#define I915_MAX_WA_REGS (16 + RING_MAX_NONPRIV_SLOTS)
>  
>  struct i915_workarounds {
>  	struct i915_wa_reg reg[I915_MAX_WA_REGS];
>  	u32 count;
> +	u32 hw_whitelist_index[I915_NUM_RINGS];
>  };
>  

I have plans to allow multiple workaround lists. So we could then
have dedicated one for whitelists too, simplifying this.

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

>  struct i915_virtual_gpu {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 05c8621..797c74e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1578,6 +1578,8 @@ enum skl_disp_power_wells {
>  #define   RING_WAIT_I8XX	(1<<0) /* gen2, PRBx_HEAD */
>  #define   RING_WAIT		(1<<11) /* gen3+, PRBx_CTL */
>  #define   RING_WAIT_SEMAPHORE	(1<<10) /* gen6+ */
> +#define RING_FORCE_TO_NONPRIV(base) ((base)+0x4D0)
> +#define   RING_MAX_NONPRIV_SLOTS  12
>  
>  #define GEN7_TLB_RD_ADDR	0x4700
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index c82c74c..64b2754 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -800,6 +800,22 @@ static int wa_add(struct drm_i915_private *dev_priv,
>  
>  #define WA_WRITE(addr, val) WA_REG(addr, 0xffffffff, val)
>  
> +static inline int wa_ring_whitelist_reg(struct intel_engine_cs *ring,
> +					     uint32_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_index[ring->id];
> +
> +	if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
> +		return -EINVAL;
> +
> +	WA_WRITE(RING_FORCE_TO_NONPRIV(ring->mmio_base) + index * 4, reg_addr);
> +	wa->hw_whitelist_index[ring->id]++;
> +
> +	return 0;
> +}
> +
>  static int gen8_init_workarounds(struct intel_engine_cs *ring)
>  {
>  	struct drm_device *dev = ring->dev;
> @@ -1094,6 +1110,7 @@ int init_workarounds_ring(struct intel_engine_cs *ring)
>  	WARN_ON(ring->id != RCS);
>  
>  	dev_priv->workarounds.count = 0;
> +	dev_priv->workarounds.hw_whitelist_index[ring->id] = 0;
>  
>  	if (IS_BROADWELL(dev))
>  		return bdw_init_workarounds(ring);
> -- 
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx