All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/i915: Keep the ctx workarounds tightly packed
@ 2018-06-15  8:00 Chris Wilson
  2018-06-15  8:00 ` [PATCH 2/2] drm/i915: Break workaround register emission into batches of 15 Chris Wilson
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: Chris Wilson @ 2018-06-15  8:00 UTC (permalink / raw)
  To: intel-gfx

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))
 
 #define WA_SET_BIT_MASKED(addr, mask) \
 	WA_REG(addr, (mask), _MASKED_BIT_ENABLE(mask))
@@ -540,7 +569,7 @@ int intel_ctx_workarounds_emit(struct i915_request *rq)
 
 	*cs++ = MI_LOAD_REGISTER_IMM(w->count);
 	for (i = 0; i < w->count; i++) {
-		*cs++ = i915_mmio_reg_offset(w->reg[i].addr);
+		*cs++ = w->reg[i].addr;
 		*cs++ = w->reg[i].value;
 	}
 	*cs++ = MI_NOOP;
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] drm/i915: Break workaround register emission into batches of 15
  2018-06-15  8:00 [PATCH 1/2] drm/i915: Keep the ctx workarounds tightly packed Chris Wilson
@ 2018-06-15  8:00 ` Chris Wilson
  2018-06-15  8:22 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/2] drm/i915: Keep the ctx workarounds tightly packed Patchwork
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Chris Wilson @ 2018-06-15  8:00 UTC (permalink / raw)
  To: intel-gfx; +Cc: Kenneth Graunke

We are restricted to the number of registers we can rewrite into a
single command by the packet length. If we have more registers than can
be fitted into a single packet, we therefore need to split the writes
into multiple packets.

Reported-by: Kenneth Graunke <kenneth@whitecape.org>
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>
Cc: Kenneth Graunke <kenneth@whitecape.org>
---
 drivers/gpu/drm/i915/intel_workarounds.c | 31 ++++++++++++++++--------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
index f8bb32e974f6..d82e08af6a60 100644
--- a/drivers/gpu/drm/i915/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/intel_workarounds.c
@@ -553,6 +553,8 @@ int intel_ctx_workarounds_init(struct drm_i915_private *dev_priv)
 int intel_ctx_workarounds_emit(struct i915_request *rq)
 {
 	struct i915_workarounds *w = &rq->i915->workarounds;
+	struct i915_wa_reg *reg = w->reg;
+	unsigned int remain;
 	u32 *cs;
 	int ret, i;
 
@@ -563,18 +565,27 @@ int intel_ctx_workarounds_emit(struct i915_request *rq)
 	if (ret)
 		return ret;
 
-	cs = intel_ring_begin(rq, (w->count * 2 + 2));
-	if (IS_ERR(cs))
-		return PTR_ERR(cs);
+	remain = w->count;
+	do {
+		unsigned int count;
 
-	*cs++ = MI_LOAD_REGISTER_IMM(w->count);
-	for (i = 0; i < w->count; i++) {
-		*cs++ = w->reg[i].addr;
-		*cs++ = w->reg[i].value;
-	}
-	*cs++ = MI_NOOP;
+		count = min(remain, 15u);
+		remain -= count;
+
+		cs = intel_ring_begin(rq, 2 * count + 2);
+		if (IS_ERR(cs))
+			return PTR_ERR(cs);
+
+		*cs++ = MI_LOAD_REGISTER_IMM(count);
+		for (i = 0; i < count; i++) {
+			*cs++ = reg->addr;
+			*cs++ = reg->value;
+			reg++;
+		}
+		*cs++ = MI_NOOP;
 
-	intel_ring_advance(rq, cs);
+		intel_ring_advance(rq, cs);
+	} while (remain);
 
 	ret = rq->engine->emit_flush(rq, EMIT_BARRIER);
 	if (ret)
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* ✗ Fi.CI.SPARSE: warning for series starting with [1/2] drm/i915: Keep the ctx workarounds tightly packed
  2018-06-15  8:00 [PATCH 1/2] drm/i915: Keep the ctx workarounds tightly packed Chris Wilson
  2018-06-15  8:00 ` [PATCH 2/2] drm/i915: Break workaround register emission into batches of 15 Chris Wilson
@ 2018-06-15  8:22 ` Patchwork
  2018-06-15  8:36 ` ✓ Fi.CI.BAT: success " Patchwork
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2018-06-15  8:22 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915: Keep the ctx workarounds tightly packed
URL   : https://patchwork.freedesktop.org/series/44809/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915: Keep the ctx workarounds tightly packed
Okay!

Commit: drm/i915: Break workaround register emission into batches of 15
-
+drivers/gpu/drm/i915/intel_workarounds.c:572:25: warning: expression using sizeof(void)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread

* ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Keep the ctx workarounds tightly packed
  2018-06-15  8:00 [PATCH 1/2] drm/i915: Keep the ctx workarounds tightly packed Chris Wilson
  2018-06-15  8:00 ` [PATCH 2/2] drm/i915: Break workaround register emission into batches of 15 Chris Wilson
  2018-06-15  8:22 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/2] drm/i915: Keep the ctx workarounds tightly packed Patchwork
@ 2018-06-15  8:36 ` Patchwork
  2018-06-15 11:29 ` [PATCH 1/2] " Mika Kuoppala
  2018-06-15 12:39 ` ✓ Fi.CI.IGT: success for series starting with [1/2] " Patchwork
  4 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2018-06-15  8:36 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915: Keep the ctx workarounds tightly packed
URL   : https://patchwork.freedesktop.org/series/44809/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4323 -> Patchwork_9318 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/44809/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9318 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_gttfill@basic:
      fi-byt-n2820:       PASS -> FAIL (fdo#106744)

    igt@kms_flip@basic-flip-vs-dpms:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106000, fdo#106097)

    igt@kms_pipe_crc_basic@read-crc-pipe-c-frame-sequence:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106238)

    igt@vgem_basic@second-client:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106745)

    
    ==== Possible fixes ====

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-glk-j4005:       FAIL (fdo#100368) -> PASS

    igt@kms_flip@basic-plain-flip:
      fi-glk-j4005:       DMESG-WARN (fdo#106097) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
  fdo#106238 https://bugs.freedesktop.org/show_bug.cgi?id=106238
  fdo#106744 https://bugs.freedesktop.org/show_bug.cgi?id=106744
  fdo#106745 https://bugs.freedesktop.org/show_bug.cgi?id=106745


== Participating hosts (43 -> 38) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4323 -> Patchwork_9318

  CI_DRM_4323: 25d3805133071406ffae77c994f464dbbb3bb34e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4519: 3381a56be31defb3b5c23a4fbc19ac26a000c35b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9318: 5917a776ca11e999912245c16146f7f54012f91c @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

5917a776ca11 drm/i915: Break workaround register emission into batches of 15
1735e8c05885 drm/i915: Keep the ctx workarounds tightly packed

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9318/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] drm/i915: Keep the ctx workarounds tightly packed
  2018-06-15  8:00 [PATCH 1/2] drm/i915: Keep the ctx workarounds tightly packed Chris Wilson
                   ` (2 preceding siblings ...)
  2018-06-15  8:36 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-06-15 11:29 ` Mika Kuoppala
  2018-06-15 11:34   ` Chris Wilson
  2018-06-15 12:39 ` ✓ Fi.CI.IGT: success for series starting with [1/2] " Patchwork
  4 siblings, 1 reply; 7+ messages in thread
From: Mika Kuoppala @ 2018-06-15 11:29 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx

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);

It seems that gem_workarounds is already smart enough to not
parse/trust the driver provided read value.

Perhaps the only value in here was that we saw what
were context and non context saved ones. But we
have other tools for that.

>  
>  	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;

Keeping the workarounds in a sorted order by addr,
could be mentioned in the header. But not insisting
on that as this is rather clear from the code.

>  
> -	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))

If you do this, then the whole chain of initing workarounds
can be converted to omitting return value.
-Mika

>  
>  #define WA_SET_BIT_MASKED(addr, mask) \
>  	WA_REG(addr, (mask), _MASKED_BIT_ENABLE(mask))
> @@ -540,7 +569,7 @@ int intel_ctx_workarounds_emit(struct i915_request *rq)
>  
>  	*cs++ = MI_LOAD_REGISTER_IMM(w->count);
>  	for (i = 0; i < w->count; i++) {
> -		*cs++ = i915_mmio_reg_offset(w->reg[i].addr);
> +		*cs++ = w->reg[i].addr;
>  		*cs++ = w->reg[i].value;
>  	}
>  	*cs++ = MI_NOOP;
> -- 
> 2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] drm/i915: Keep the ctx workarounds tightly packed
  2018-06-15 11:29 ` [PATCH 1/2] " Mika Kuoppala
@ 2018-06-15 11:34   ` Chris Wilson
  0 siblings, 0 replies; 7+ messages in thread
From: Chris Wilson @ 2018-06-15 11:34 UTC (permalink / raw)
  To: Mika Kuoppala, intel-gfx

Quoting Mika Kuoppala (2018-06-15 12:29:23)
> 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);
> 
> It seems that gem_workarounds is already smart enough to not
> parse/trust the driver provided read value.
> 
> Perhaps the only value in here was that we saw what
> were context and non context saved ones. But we
> have other tools for that.

We couldn't even rely on it doing that, since what it read would be
random (we may have had a context, the power context may or may not have
the same values, we may never had loaded the values)...

Fortunately, we stopped using them :)

> >       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;
> 
> Keeping the workarounds in a sorted order by addr,
> could be mentioned in the header. But not insisting
> on that as this is rather clear from the code.
> 
> >  
> > -     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))
> 
> If you do this, then the whole chain of initing workarounds
> can be converted to omitting return value.

I was thinking the same for a followup. As well as not storing the
workarounds but computing them on the fly -- for the normal case, we
only need them once for the initial context image.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread

* ✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915: Keep the ctx workarounds tightly packed
  2018-06-15  8:00 [PATCH 1/2] drm/i915: Keep the ctx workarounds tightly packed Chris Wilson
                   ` (3 preceding siblings ...)
  2018-06-15 11:29 ` [PATCH 1/2] " Mika Kuoppala
@ 2018-06-15 12:39 ` Patchwork
  4 siblings, 0 replies; 7+ messages in thread
From: Patchwork @ 2018-06-15 12:39 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915: Keep the ctx workarounds tightly packed
URL   : https://patchwork.freedesktop.org/series/44809/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4323_full -> Patchwork_9318_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9318_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9318_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9318_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-vebox:
      shard-kbl:          PASS -> SKIP +1

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          SKIP -> PASS

    
== Known issues ==

  Here are the changes found in Patchwork_9318_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_eio@in-flight-internal-immediate:
      shard-apl:          PASS -> FAIL (fdo#105957)

    igt@gem_exec_suspend@basic-s3:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@kms_cursor_legacy@cursor-vs-flip-toggle:
      shard-hsw:          PASS -> FAIL (fdo#103355)

    igt@kms_flip@2x-plain-flip-fb-recreate-interruptible:
      shard-glk:          PASS -> FAIL (fdo#100368) +1

    igt@kms_flip@2x-plain-flip-ts-check:
      shard-hsw:          PASS -> FAIL (fdo#100368)

    igt@kms_flip_tiling@flip-to-y-tiled:
      shard-glk:          PASS -> FAIL (fdo#103822, fdo#104724) +1

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-shrfb-plflip-blt:
      shard-glk:          PASS -> FAIL (fdo#103167, fdo#104724)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_gtt:
      shard-glk:          INCOMPLETE (fdo#103359, k.org#198133) -> PASS

    igt@kms_atomic_transition@1x-modeset-transitions-nonblocking-fencing:
      shard-glk:          FAIL (fdo#105703) -> PASS

    igt@kms_setmode@basic:
      shard-kbl:          FAIL (fdo#99912) -> PASS

    igt@perf_pmu@other-init-2:
      shard-snb:          INCOMPLETE (fdo#105411) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103355 https://bugs.freedesktop.org/show_bug.cgi?id=103355
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105703 https://bugs.freedesktop.org/show_bug.cgi?id=105703
  fdo#105957 https://bugs.freedesktop.org/show_bug.cgi?id=105957
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4323 -> Patchwork_9318

  CI_DRM_4323: 25d3805133071406ffae77c994f464dbbb3bb34e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4519: 3381a56be31defb3b5c23a4fbc19ac26a000c35b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9318: 5917a776ca11e999912245c16146f7f54012f91c @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9318/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-06-15 12:39 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-15  8:00 [PATCH 1/2] drm/i915: Keep the ctx workarounds tightly packed Chris Wilson
2018-06-15  8:00 ` [PATCH 2/2] drm/i915: Break workaround register emission into batches of 15 Chris Wilson
2018-06-15  8:22 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/2] drm/i915: Keep the ctx workarounds tightly packed Patchwork
2018-06-15  8:36 ` ✓ Fi.CI.BAT: success " Patchwork
2018-06-15 11:29 ` [PATCH 1/2] " Mika Kuoppala
2018-06-15 11:34   ` Chris Wilson
2018-06-15 12:39 ` ✓ Fi.CI.IGT: success for series starting with [1/2] " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.