All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] drm/i915: Keep the ctx workarounds tightly packed
@ 2018-06-15  8:59 Chris Wilson
  2018-06-15  8:59 ` [PATCH 2/3] drm/i915: Break workaround register emission into batches of 15 Chris Wilson
                   ` (6 more replies)
  0 siblings, 7 replies; 13+ messages in thread
From: Chris Wilson @ 2018-06-15  8:59 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] 13+ messages in thread

* [PATCH 2/3] drm/i915: Break workaround register emission into batches of 15
  2018-06-15  8:59 [PATCH 1/3] drm/i915: Keep the ctx workarounds tightly packed Chris Wilson
@ 2018-06-15  8:59 ` Chris Wilson
  2018-06-15  8:59 ` [PATCH 3/3] drm/i915: Enable provoking vertex fix on Gen9+ systems Chris Wilson
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Chris Wilson @ 2018-06-15  8:59 UTC (permalink / raw)
  To: intel-gfx; +Cc: Kenneth Graunke

We are restricted to the number of registers we can write with 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] 13+ messages in thread

* [PATCH 3/3] drm/i915: Enable provoking vertex fix on Gen9+ systems.
  2018-06-15  8:59 [PATCH 1/3] drm/i915: Keep the ctx workarounds tightly packed Chris Wilson
  2018-06-15  8:59 ` [PATCH 2/3] drm/i915: Break workaround register emission into batches of 15 Chris Wilson
@ 2018-06-15  8:59 ` Chris Wilson
  2018-06-15  9:23 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915: Keep the ctx workarounds tightly packed Patchwork
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Chris Wilson @ 2018-06-15  8:59 UTC (permalink / raw)
  To: intel-gfx; +Cc: Kenneth Graunke

From: Kenneth Graunke <kenneth@whitecape.org>

The SF and clipper units mishandle the provoking vertex in some cases,
which can cause misrendering with shaders that use flat shaded inputs.

There are chicken bits in 3D_CHICKEN3 (for SF) and FF_SLICE_CHICKEN
(for the clipper) that work around the issue.  These registers are
unfortunately not part of the logical context (even the power context).

This patch sets both bits, and bumps the number of allowed workaround
registers to avoid running out of space (thanks to Chris Wilson for
helping debug that issue).

I am not aware of any workaround names or numbers assigned for these
issues, they're simply recommended in the documentation for each of
the registers.

Bugzilla: https://bugs.freedesktop.org/103047
---
 drivers/gpu/drm/i915/i915_reg.h          | 5 +++++
 drivers/gpu/drm/i915/intel_workarounds.c | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 140f6a27d696..35d8c2be85be 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2432,12 +2432,17 @@ enum i915_power_well_id {
 #define _3D_CHICKEN	_MMIO(0x2084)
 #define  _3D_CHICKEN_HIZ_PLANE_DISABLE_MSAA_4X_SNB	(1 << 10)
 #define _3D_CHICKEN2	_MMIO(0x208c)
+
+#define FF_SLICE_CHICKEN	_MMIO(0x2088)
+#define  FF_SLICE_CHICKEN_CL_PROVOKING_VERTEX_FIX	(1 << 1)
+
 /* Disables pipelining of read flushes past the SF-WIZ interface.
  * Required on all Ironlake steppings according to the B-Spec, but the
  * particular danger of not doing so is not specified.
  */
 # define _3D_CHICKEN2_WM_READ_PIPELINED			(1 << 14)
 #define _3D_CHICKEN3	_MMIO(0x2090)
+#define  _3D_CHICKEN_SF_PROVOKING_VERTEX_FIX		(1 << 12)
 #define  _3D_CHICKEN_SF_DISABLE_OBJEND_CULL		(1 << 10)
 #define  _3D_CHICKEN3_AA_LINE_QUALITY_FIX_ENABLE	(1 << 5)
 #define  _3D_CHICKEN3_SF_DISABLE_FASTCLIP_CULL		(1 << 5)
diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
index d82e08af6a60..ca74b4b3f206 100644
--- a/drivers/gpu/drm/i915/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/intel_workarounds.c
@@ -303,6 +303,12 @@ static int gen9_ctx_workarounds_init(struct drm_i915_private *dev_priv)
 	if (IS_GEN9_LP(dev_priv))
 		WA_SET_BIT_MASKED(GEN9_WM_CHICKEN3, GEN9_FACTOR_IN_CLR_VAL_HIZ);
 
+	/* BSpec: 11391 */
+	WA_SET_BIT_MASKED(FF_SLICE_CHICKEN,
+			  FF_SLICE_CHICKEN_CL_PROVOKING_VERTEX_FIX);
+	/* BSpec: 11299 */
+	WA_SET_BIT_MASKED(_3D_CHICKEN3, _3D_CHICKEN_SF_PROVOKING_VERTEX_FIX);
+
 	return 0;
 }
 
-- 
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] 13+ messages in thread

* ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915: Keep the ctx workarounds tightly packed
  2018-06-15  8:59 [PATCH 1/3] drm/i915: Keep the ctx workarounds tightly packed Chris Wilson
  2018-06-15  8:59 ` [PATCH 2/3] drm/i915: Break workaround register emission into batches of 15 Chris Wilson
  2018-06-15  8:59 ` [PATCH 3/3] drm/i915: Enable provoking vertex fix on Gen9+ systems Chris Wilson
@ 2018-06-15  9:23 ` Patchwork
  2018-06-15  9:24 ` ✗ Fi.CI.SPARSE: " Patchwork
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-06-15  9:23 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

$ dim checkpatch origin/drm-tip
7aec116d9abf drm/i915: Keep the ctx workarounds tightly packed
6deece3deb40 drm/i915: Break workaround register emission into batches of 15
ae25fb82a171 drm/i915: Enable provoking vertex fix on Gen9+ systems.
-:61: ERROR:MISSING_SIGN_OFF: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 0 checks, 29 lines checked

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

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

* ✗ Fi.CI.SPARSE: warning for series starting with [1/3] drm/i915: Keep the ctx workarounds tightly packed
  2018-06-15  8:59 [PATCH 1/3] drm/i915: Keep the ctx workarounds tightly packed Chris Wilson
                   ` (2 preceding siblings ...)
  2018-06-15  9:23 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915: Keep the ctx workarounds tightly packed Patchwork
@ 2018-06-15  9:24 ` Patchwork
  2018-06-15  9:39 ` ✗ Fi.CI.BAT: failure " Patchwork
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-06-15  9:24 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/3] drm/i915: Keep the ctx workarounds tightly packed
URL   : https://patchwork.freedesktop.org/series/44811/
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)

Commit: drm/i915: Enable provoking vertex fix on Gen9+ systems.
Okay!

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

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

* ✗ Fi.CI.BAT: failure for series starting with [1/3] drm/i915: Keep the ctx workarounds tightly packed
  2018-06-15  8:59 [PATCH 1/3] drm/i915: Keep the ctx workarounds tightly packed Chris Wilson
                   ` (3 preceding siblings ...)
  2018-06-15  9:24 ` ✗ Fi.CI.SPARSE: " Patchwork
@ 2018-06-15  9:39 ` Patchwork
  2018-06-15 11:36 ` [PATCH 1/3] " Mika Kuoppala
  2018-06-15 16:01 ` Oscar Mateo Lozano
  6 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2018-06-15  9:39 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

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

== Summary - FAILURE ==

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

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

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@gem_workarounds@basic-read:
      fi-skl-6260u:       PASS -> FAIL
      fi-cfl-s3:          PASS -> FAIL
      fi-skl-6700k2:      PASS -> FAIL
      fi-skl-6770hq:      PASS -> FAIL
      fi-kbl-7560u:       PASS -> FAIL
      fi-skl-6600u:       PASS -> FAIL
      fi-kbl-guc:         PASS -> FAIL
      fi-kbl-7500u:       PASS -> FAIL
      fi-skl-6700hq:      PASS -> FAIL
      fi-bxt-j4205:       PASS -> FAIL
      fi-skl-gvtdvm:      PASS -> FAIL
      fi-cfl-guc:         PASS -> FAIL
      {fi-whl-u}:         PASS -> FAIL
      fi-cfl-8700k:       PASS -> FAIL
      fi-glk-j4005:       PASS -> FAIL
      fi-skl-guc:         PASS -> FAIL
      fi-kbl-7567u:       PASS -> FAIL
      fi-kbl-r:           PASS -> FAIL

    
    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        PASS -> SKIP

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ctx_create@basic-files:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#105719)

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

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

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

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


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

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


== Build changes ==

    * Linux: CI_DRM_4323 -> Patchwork_9320

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


== Linux commits ==

ae25fb82a171 drm/i915: Enable provoking vertex fix on Gen9+ systems.
6deece3deb40 drm/i915: Break workaround register emission into batches of 15
7aec116d9abf drm/i915: Keep the ctx workarounds tightly packed

== Logs ==

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

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

* Re: [PATCH 1/3] drm/i915: Keep the ctx workarounds tightly packed
  2018-06-15  8:59 [PATCH 1/3] drm/i915: Keep the ctx workarounds tightly packed Chris Wilson
                   ` (4 preceding siblings ...)
  2018-06-15  9:39 ` ✗ Fi.CI.BAT: failure " Patchwork
@ 2018-06-15 11:36 ` Mika Kuoppala
  2018-06-15 16:01 ` Oscar Mateo Lozano
  6 siblings, 0 replies; 13+ messages in thread
From: Mika Kuoppala @ 2018-06-15 11:36 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);
>  
>  	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

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

* Re: [PATCH 1/3] drm/i915: Keep the ctx workarounds tightly packed
  2018-06-15  8:59 [PATCH 1/3] drm/i915: Keep the ctx workarounds tightly packed Chris Wilson
                   ` (5 preceding siblings ...)
  2018-06-15 11:36 ` [PATCH 1/3] " Mika Kuoppala
@ 2018-06-15 16:01 ` Oscar Mateo Lozano
  2018-06-15 16:08   ` Chris Wilson
  2018-06-15 16:19   ` Ville Syrjälä
  6 siblings, 2 replies; 13+ messages in thread
From: Oscar Mateo Lozano @ 2018-06-15 16:01 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx



On 6/15/2018 1:59 AM, Chris Wilson wrote:
> 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.

A while ago I sent a patch 
(https://patchwork.freedesktop.org/patch/205035/) that uses simple MMIO 
writes to apply ctx workarounds. This is possible since we now have 
proper golden contexts, and avoids the need for these patches.
It also has the advantage that an improperly classified WA doesn't get 
lost (we still need the classification if we want to properly validate 
the WAs, but that's a different story).
Are we sure we prefer to do this instead?

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

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

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

* Re: [PATCH 1/3] drm/i915: Keep the ctx workarounds tightly packed
  2018-06-15 16:01 ` Oscar Mateo Lozano
@ 2018-06-15 16:08   ` Chris Wilson
  2018-06-15 16:19   ` Ville Syrjälä
  1 sibling, 0 replies; 13+ messages in thread
From: Chris Wilson @ 2018-06-15 16:08 UTC (permalink / raw)
  To: Oscar Mateo Lozano, intel-gfx

Quoting Oscar Mateo Lozano (2018-06-15 17:01:37)
> 
> 
> On 6/15/2018 1:59 AM, Chris Wilson wrote:
> > 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.
> 
> A while ago I sent a patch 
> (https://patchwork.freedesktop.org/patch/205035/) that uses simple MMIO 
> writes to apply ctx workarounds. This is possible since we now have 
> proper golden contexts, and avoids the need for these patches.
> It also has the advantage that an improperly classified WA doesn't get 
> lost (we still need the classification if we want to properly validate 
> the WAs, but that's a different story).
> Are we sure we prefer to do this instead?

Short attention span, I was caught up in trying to fix the overflow.

So I think I want to keep the checker here that we aren't using
conflicting workarounds, and keep the list ordered (because that helps
us when reading and checking them).

Care to respin? :)

Meanwhile, gem_workarounds is still complaining that the write here to
_3D_CHICKEN3 isn't sticking. It works locally, and I can't see anything
to explain why it wouldn't for CI.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 1/3] drm/i915: Keep the ctx workarounds tightly packed
  2018-06-15 16:01 ` Oscar Mateo Lozano
  2018-06-15 16:08   ` Chris Wilson
@ 2018-06-15 16:19   ` Ville Syrjälä
  2018-06-15 16:22     ` Chris Wilson
  1 sibling, 1 reply; 13+ messages in thread
From: Ville Syrjälä @ 2018-06-15 16:19 UTC (permalink / raw)
  To: Oscar Mateo Lozano; +Cc: intel-gfx

On Fri, Jun 15, 2018 at 09:01:37AM -0700, Oscar Mateo Lozano wrote:
> 
> 
> On 6/15/2018 1:59 AM, Chris Wilson wrote:
> > 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.
> 
> A while ago I sent a patch 
> (https://patchwork.freedesktop.org/patch/205035/) that uses simple MMIO 
> writes to apply ctx workarounds. This is possible since we now have 
> proper golden contexts, and avoids the need for these patches.
> It also has the advantage that an improperly classified WA doesn't get 
> lost (we still need the classification if we want to properly validate 
> the WAs, but that's a different story).
> Are we sure we prefer to do this instead?

Wouldn't that require PSMI+FSM dance to make sure execlist has
an active context when you write the regs? Can't see anything like that
in the code currently, nor is there anything in the referenced patch.

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

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

* Re: [PATCH 1/3] drm/i915: Keep the ctx workarounds tightly packed
  2018-06-15 16:19   ` Ville Syrjälä
@ 2018-06-15 16:22     ` Chris Wilson
  2018-06-15 16:37       ` Ville Syrjälä
  0 siblings, 1 reply; 13+ messages in thread
From: Chris Wilson @ 2018-06-15 16:22 UTC (permalink / raw)
  To: Ville Syrjälä, Oscar Mateo Lozano; +Cc: intel-gfx

Quoting Ville Syrjälä (2018-06-15 17:19:14)
> On Fri, Jun 15, 2018 at 09:01:37AM -0700, Oscar Mateo Lozano wrote:
> > 
> > 
> > On 6/15/2018 1:59 AM, Chris Wilson wrote:
> > > 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.
> > 
> > A while ago I sent a patch 
> > (https://patchwork.freedesktop.org/patch/205035/) that uses simple MMIO 
> > writes to apply ctx workarounds. This is possible since we now have 
> > proper golden contexts, and avoids the need for these patches.
> > It also has the advantage that an improperly classified WA doesn't get 
> > lost (we still need the classification if we want to properly validate 
> > the WAs, but that's a different story).
> > Are we sure we prefer to do this instead?
> 
> Wouldn't that require PSMI+FSM dance to make sure execlist has
> an active context when you write the regs? Can't see anything like that
> in the code currently, nor is there anything in the referenced patch.

We keep forcewake asserted across the bringup, from before we load the
default context until after we have saved the context image. These mmio
writes should be saved along with the image. That's the theory at least.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH 1/3] drm/i915: Keep the ctx workarounds tightly packed
  2018-06-15 16:22     ` Chris Wilson
@ 2018-06-15 16:37       ` Ville Syrjälä
  2018-06-15 16:43         ` Chris Wilson
  0 siblings, 1 reply; 13+ messages in thread
From: Ville Syrjälä @ 2018-06-15 16:37 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Fri, Jun 15, 2018 at 05:22:40PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjälä (2018-06-15 17:19:14)
> > On Fri, Jun 15, 2018 at 09:01:37AM -0700, Oscar Mateo Lozano wrote:
> > > 
> > > 
> > > On 6/15/2018 1:59 AM, Chris Wilson wrote:
> > > > 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.
> > > 
> > > A while ago I sent a patch 
> > > (https://patchwork.freedesktop.org/patch/205035/) that uses simple MMIO 
> > > writes to apply ctx workarounds. This is possible since we now have 
> > > proper golden contexts, and avoids the need for these patches.
> > > It also has the advantage that an improperly classified WA doesn't get 
> > > lost (we still need the classification if we want to properly validate 
> > > the WAs, but that's a different story).
> > > Are we sure we prefer to do this instead?
> > 
> > Wouldn't that require PSMI+FSM dance to make sure execlist has
> > an active context when you write the regs? Can't see anything like that
> > in the code currently, nor is there anything in the referenced patch.
> 
> We keep forcewake asserted across the bringup, from before we load the
> default context until after we have saved the context image. These mmio
> writes should be saved along with the image. That's the theory at least.

Force wake isn't quite enough from what I understand. Or maybe it was
that there is at least some delay between forcewake ack asserting and
the context actually having been loaded (just my recollection of what
Mika's experiments once showed).

The spec at least still lists the extra dance for MMIO access into
context saved registers.

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

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

* Re: [PATCH 1/3] drm/i915: Keep the ctx workarounds tightly packed
  2018-06-15 16:37       ` Ville Syrjälä
@ 2018-06-15 16:43         ` Chris Wilson
  0 siblings, 0 replies; 13+ messages in thread
From: Chris Wilson @ 2018-06-15 16:43 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

Quoting Ville Syrjälä (2018-06-15 17:37:44)
> On Fri, Jun 15, 2018 at 05:22:40PM +0100, Chris Wilson wrote:
> > Quoting Ville Syrjälä (2018-06-15 17:19:14)
> > > On Fri, Jun 15, 2018 at 09:01:37AM -0700, Oscar Mateo Lozano wrote:
> > > > 
> > > > 
> > > > On 6/15/2018 1:59 AM, Chris Wilson wrote:
> > > > > 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.
> > > > 
> > > > A while ago I sent a patch 
> > > > (https://patchwork.freedesktop.org/patch/205035/) that uses simple MMIO 
> > > > writes to apply ctx workarounds. This is possible since we now have 
> > > > proper golden contexts, and avoids the need for these patches.
> > > > It also has the advantage that an improperly classified WA doesn't get 
> > > > lost (we still need the classification if we want to properly validate 
> > > > the WAs, but that's a different story).
> > > > Are we sure we prefer to do this instead?
> > > 
> > > Wouldn't that require PSMI+FSM dance to make sure execlist has
> > > an active context when you write the regs? Can't see anything like that
> > > in the code currently, nor is there anything in the referenced patch.
> > 
> > We keep forcewake asserted across the bringup, from before we load the
> > default context until after we have saved the context image. These mmio
> > writes should be saved along with the image. That's the theory at least.
> 
> Force wake isn't quite enough from what I understand. Or maybe it was
> that there is at least some delay between forcewake ack asserting and
> the context actually having been loaded (just my recollection of what
> Mika's experiments once showed).
> 
> The spec at least still lists the extra dance for MMIO access into
> context saved registers.

We do at least have the advantage now of checking that all contexts start
with the expected w/a register state. Hmm, if we stuff those registers
with garbage inside gem_workarounds, that will help to confirm that we
do load them correctly. (If having them scrubbed by reset isn't enough!)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-15  8:59 [PATCH 1/3] drm/i915: Keep the ctx workarounds tightly packed Chris Wilson
2018-06-15  8:59 ` [PATCH 2/3] drm/i915: Break workaround register emission into batches of 15 Chris Wilson
2018-06-15  8:59 ` [PATCH 3/3] drm/i915: Enable provoking vertex fix on Gen9+ systems Chris Wilson
2018-06-15  9:23 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/3] drm/i915: Keep the ctx workarounds tightly packed Patchwork
2018-06-15  9:24 ` ✗ Fi.CI.SPARSE: " Patchwork
2018-06-15  9:39 ` ✗ Fi.CI.BAT: failure " Patchwork
2018-06-15 11:36 ` [PATCH 1/3] " Mika Kuoppala
2018-06-15 16:01 ` Oscar Mateo Lozano
2018-06-15 16:08   ` Chris Wilson
2018-06-15 16:19   ` Ville Syrjälä
2018-06-15 16:22     ` Chris Wilson
2018-06-15 16:37       ` Ville Syrjälä
2018-06-15 16:43         ` Chris Wilson

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.