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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jan 13, 2016 at 10:06:26AM +0000, Arun Siluvery wrote:
> 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 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
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@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         |  3 +++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 18 ++++++++++++++++++
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 104bd18..660afe1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1653,11 +1653,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];
>  };
>  
>  struct i915_virtual_gpu {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0a98889..6668bb0 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1635,6 +1635,9 @@ enum skl_disp_power_wells {
>  #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	_MMIO(0x4700)
>  
>  #if 0
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 4060acf..354da81 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -787,6 +787,23 @@ 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,
> +					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_index[ring->id];
> +
> +	if (WARN_ON(index >= RING_MAX_NONPRIV_SLOTS))
> +		return -EINVAL;
> +
> +	WA_WRITE(_MMIO(RING_FORCE_TO_NONPRIV(ring->mmio_base) + index * 4),
> +		 reg_addr.reg);

No _MMIO junk outside of i915_reg.h please. Just parametrize the reg define
properly.

> +	wa->hw_whitelist_index[ring->id]++;
> +
> +	return 0;
> +}
> +
>  static int gen8_init_workarounds(struct intel_engine_cs *ring)
>  {
>  	struct drm_device *dev = ring->dev;
> @@ -1115,6 +1132,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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx