[Intel-gfx] [PATCH v4 1/8] drm/i915/gen9: Add framework to whitelist specific GPU registers
Arun Siluvery
arun.siluvery at linux.intel.com
Tue Jan 19 02:16:52 PST 2016
On 19/01/2016 09:00, Daniel Vetter wrote:
> On Thu, Jan 14, 2016 at 03:27:35PM +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: improvements suggested by Chris Wilson.
>> 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.
>> Make HW whitelist count of each engine available in debugfs.
>>
>> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
>> Reviewed-by: Mika Kuoppala <mika.kuoppala at intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Signed-off-by: Arun Siluvery <arun.siluvery at linux.intel.com>
>
> If you resend just single patches to a series you must --in-reply-to the
> individual patch, not the cover letter. Otherwise patchwork won't pick it
> up, which means we don't have CI results for this.
Hi Daniel,
Yes I did use --in-reply-to but probably not the correct message-id,
will keep this in mind.
>
> Since it's been a while probably best to just resend the entire pile.
>
> Also we seem to be missing r-b tags for the actual w/a changes.
yes, actual w/a are yet to be reviewed, I can resend all of them once
they are reviewed or you want me to send it now?
regards
Arun
> -Daniel
>
>> ---
>> drivers/gpu/drm/i915/i915_debugfs.c | 15 ++++++++++-----
>> drivers/gpu/drm/i915/i915_drv.h | 9 ++++++++-
>> drivers/gpu/drm/i915/i915_reg.h | 3 +++
>> drivers/gpu/drm/i915/intel_ringbuffer.c | 17 +++++++++++++++++
>> 4 files changed, 38 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index e3377ab..7eb002c 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -3229,9 +3229,11 @@ static int i915_wa_registers(struct seq_file *m, void *unused)
>> {
>> int i;
>> int ret;
>> + struct intel_engine_cs *ring;
>> struct drm_info_node *node = (struct drm_info_node *) m->private;
>> struct drm_device *dev = node->minor->dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> + struct i915_workarounds *workarounds = &dev_priv->workarounds;
>>
>> ret = mutex_lock_interruptible(&dev->struct_mutex);
>> if (ret)
>> @@ -3239,15 +3241,18 @@ static int i915_wa_registers(struct seq_file *m, void *unused)
>>
>> intel_runtime_pm_get(dev_priv);
>>
>> - seq_printf(m, "Workarounds applied: %d\n", dev_priv->workarounds.count);
>> - for (i = 0; i < dev_priv->workarounds.count; ++i) {
>> + seq_printf(m, "Workarounds applied: %d\n", workarounds->count);
>> + for_each_ring(ring, dev_priv, i)
>> + seq_printf(m, "HW whitelist count for %s: %d\n",
>> + ring->name, workarounds->hw_whitelist_count[i]);
>> + for (i = 0; i < workarounds->count; ++i) {
>> i915_reg_t addr;
>> u32 mask, value, read;
>> bool ok;
>>
>> - addr = dev_priv->workarounds.reg[i].addr;
>> - mask = dev_priv->workarounds.reg[i].mask;
>> - value = dev_priv->workarounds.reg[i].value;
>> + addr = workarounds->reg[i].addr;
>> + mask = workarounds->reg[i].mask;
>> + value = workarounds->reg[i].value;
>> read = I915_READ(addr);
>> ok = (value & mask) == (read & mask);
>> seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X, read: 0x%08x, status: %s\n",
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 104bd18..83fccc0 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_count[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..7938814 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, i) _MMIO(((base)+0x4D0) + (i)*4)
>> +#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..56af736 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -787,6 +787,22 @@ static int wa_add(struct drm_i915_private *dev_priv,
>>
>> #define WA_WRITE(addr, val) WA_REG(addr, 0xffffffff, val)
>>
>> +static int wa_ring_whitelist_reg(struct intel_engine_cs *ring, i915_reg_t reg)
>> +{
>> + 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),
>> + i915_mmio_reg_offset(reg));
>> + wa->hw_whitelist_count[ring->id]++;
>> +
>> + return 0;
>> +}
>> +
>> static int gen8_init_workarounds(struct intel_engine_cs *ring)
>> {
>> struct drm_device *dev = ring->dev;
>> @@ -1115,6 +1131,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_count[RCS] = 0;
>>
>> if (IS_BROADWELL(dev))
>> return bdw_init_workarounds(ring);
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list