Re: [PATCH 1/3] drm/i915: Keep the ctx workarounds tightly packed

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

Chris Wilson <chris@chris-wilson.co.uk> writes:

> For each platform, we have a few registers that rewritten with multiple
> values -- they are not part of a sequence, just different parts of a
> masked register set at different times (e.g. platform and gen
> workarounds). Consolidate these into a single register write to keep the
> table compact.
>
> While adjusting the construction of the wa table, make it non fatal so
> that the driver still loads but keeping the warning and extra details
> for inspection.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c      | 25 ++--------
>  drivers/gpu/drm/i915/i915_drv.h          |  2 +-
>  drivers/gpu/drm/i915/intel_workarounds.c | 63 +++++++++++++++++-------
>  3 files changed, 52 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index c600279d3db5..f78895ffab9b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3378,28 +3378,13 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused)
>  
>  static int i915_wa_registers(struct seq_file *m, void *unused)
>  {
> -	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> -	struct i915_workarounds *workarounds = &dev_priv->workarounds;
> +	struct i915_workarounds *wa = &node_to_i915(m->private)->workarounds;
>  	int i;
>  
> -	intel_runtime_pm_get(dev_priv);
> -
> -	seq_printf(m, "Workarounds applied: %d\n", workarounds->count);
> -	for (i = 0; i < workarounds->count; ++i) {
> -		i915_reg_t addr;
> -		u32 mask, value, read;
> -		bool ok;
> -
> -		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",
> -			   i915_mmio_reg_offset(addr), value, mask, read, ok ? "OK" : "FAIL");
> -	}
> -
> -	intel_runtime_pm_put(dev_priv);
> +	seq_printf(m, "Workarounds applied: %d\n", wa->count);
> +	for (i = 0; i < wa->count; ++i)
> +		seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X\n",
> +			   wa->reg[i].addr, wa->reg[i].value, wa->reg[i].mask);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2c12de678e32..91c389622217 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1308,7 +1308,7 @@ struct i915_frontbuffer_tracking {
>  };
>  
>  struct i915_wa_reg {
> -	i915_reg_t addr;
> +	u32 addr;
>  	u32 value;
>  	/* bitmask representing WA bits */
>  	u32 mask;
> diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
> index 24b929ce3341..f8bb32e974f6 100644
> --- a/drivers/gpu/drm/i915/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/intel_workarounds.c
> @@ -48,29 +48,58 @@
>   * - Public functions to init or apply the given workaround type.
>   */
>  
> -static int wa_add(struct drm_i915_private *dev_priv,
> -		  i915_reg_t addr,
> -		  const u32 mask, const u32 val)
> +static void wa_add(struct drm_i915_private *i915,
> +		   i915_reg_t reg, const u32 mask, const u32 val)
>  {
> -	const unsigned int idx = dev_priv->workarounds.count;
> +	struct i915_workarounds *wa = &i915->workarounds;
> +	unsigned int start = 0, end = wa->count;
> +	unsigned int addr = i915_mmio_reg_offset(reg);
> +	struct i915_wa_reg *r;
> +
> +	while (start < end) {
> +		unsigned int mid = start + (end - start) / 2;
> +
> +		if (wa->reg[mid].addr < addr) {
> +			start = mid + 1;
> +		} else if (wa->reg[mid].addr > addr) {
> +			end = mid;
> +		} else {
> +			r = &wa->reg[mid];
> +
> +			if ((mask & ~r->mask) == 0) {
> +				DRM_ERROR("Discarding overwritten w/a for reg %04x (mask: %08x, value: %08x)\n",
> +					  addr, r->mask, r->value);
> +
> +				r->value &= ~mask;
> +			}
> +
> +			r->value |= val;
> +			r->mask  |= mask;
> +			return;
> +		}
> +	}
>  
> -	if (WARN_ON(idx >= I915_MAX_WA_REGS))
> -		return -ENOSPC;
> +	if (WARN_ON_ONCE(wa->count >= I915_MAX_WA_REGS)) {
> +		DRM_ERROR("Dropping w/a for reg %04x (mask: %08x, value: %08x)\n",
> +			  addr, mask, val);
> +		return;
> +	}
>  
> -	dev_priv->workarounds.reg[idx].addr = addr;
> -	dev_priv->workarounds.reg[idx].value = val;
> -	dev_priv->workarounds.reg[idx].mask = mask;
> +	r = &wa->reg[wa->count++];
> +	r->addr  = addr;
> +	r->value = val;
> +	r->mask  = mask;
>  
> -	dev_priv->workarounds.count++;
> +	while (r-- > wa->reg) {
> +		GEM_BUG_ON(r[0].addr == r[1].addr);
> +		if (r[1].addr > r[0].addr)
> +			break;
>  
> -	return 0;
> +		swap(r[1], r[0]);
> +	}
>  }
>  
> -#define WA_REG(addr, mask, val) do { \
> -		const int r = wa_add(dev_priv, (addr), (mask), (val)); \
> -		if (r) \
> -			return r; \
> -	} while (0)
> +#define WA_REG(addr, mask, val) wa_add(dev_priv, (addr), (mask), (val))

On previous thread, I noted that this makes the wa init chain
not needing return values.

But cleaning that up can be a followup.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx