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

[Chris Wilson]

On Wed, Jan 13, 2016 at 01:41:47PM +0000, Arun Siluvery wrote:
> On 13/01/2016 13:03, Chris Wilson wrote:
> >On Wed, Jan 13, 2016 at 12:28:17PM +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.
> >
> >You need to be clear that this is the hw whitelist and not our sw
> >whitelist.
> >
> ok, I will update the commit msg to clarify this.
> 
> >>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).
> >>
> >>Reviewed-by: Mika Kuoppala <mika.kuoppala at intel.com>
> >>Cc: Mika Kuoppala <mika.kuoppala at intel.com>
> >>Signed-off-by: Arun Siluvery <arun.siluvery at 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 | 17 +++++++++++++++++
> >>  3 files changed, 28 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];
> >
> >This is a counter only to be used whilst constructing the reg list. It
> >also implies that this list of wa_reg is now engine specific. Hmm, looks
> >like it always has been, just been relying on RCS running first.
> >
> >I would fix that up first.
> We are initializing all WA only for RCS, similarly HW whitelist also
> which is engine specific.
> 
> HW whitelist is initialized along with WA so essentially we are
> doing it only for RCS. Afaik there is no requirement to have it for
> other engines, we can actually limit hw_whitelist_index to RCS. I
> only allowed for all rings because spec defined it for other engines
> and also we don't have to change this when required. It is not clear
> what needs fixing up.

You can at least remove the appearance of it doing performing cross-engine
setup. That mistake has already been observed by users where we have
used RCS to initialise the other rings, and this patch looks like it
could be doing exactly the same - or enabling others to make such a
mistake.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre