diff mbox

[06/17] drm/i915/ringbuffer: Serialize load of PD_DIR

Message ID 20180610194325.13467-7-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson June 10, 2018, 7:43 p.m. UTC 
After triggering the mm switch with a load of PD_DIR, which may be
deferred unto the MI_SET_CONTEXT on rcs, serialise the next commands
with that load by posting a read of PD_DIR (or else those subsequent
commands may access the stale page tables).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
---
 drivers/gpu/drm/i915/intel_engine_cs.c  |  5 +--
 drivers/gpu/drm/i915/intel_ringbuffer.c | 48 ++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_ringbuffer.h |  5 ++-
 3 files changed, 43 insertions(+), 15 deletions(-)

Comments

Chris Wilson June 11, 2018, 10:12 a.m. UTC | #1 
Quoting Chris Wilson (2018-06-10 20:43:14)
> After triggering the mm switch with a load of PD_DIR, which may be
> deferred unto the MI_SET_CONTEXT on rcs, serialise the next commands
> with that load by posting a read of PD_DIR (or else those subsequent
> commands may access the stale page tables).

My stress test failed over night. Much improved MTBF, but not eliminated
yet. Next step is to follow the STORE_REG_MEM with an
engine->emit_flush(EMIT_INVALIDATE).

I think we are indeed on the right track, and that these are much more
than random delays.
-Chris
Joonas Lahtinen June 11, 2018, 10:43 a.m. UTC | #2 
Quoting Chris Wilson (2018-06-10 22:43:14)
> After triggering the mm switch with a load of PD_DIR, which may be
> deferred unto the MI_SET_CONTEXT on rcs, serialise the next commands
> with that load by posting a read of PD_DIR (or else those subsequent
> commands may access the stale page tables).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Matthew Auld <matthew.william.auld@gmail.com>

Again, commit message checks out with code. Do you have your own
bug-triaging flowchart that you come up with these W/As? :)

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
Chris Wilson June 11, 2018, 10:47 a.m. UTC | #3 
Quoting Joonas Lahtinen (2018-06-11 11:43:51)
> Quoting Chris Wilson (2018-06-10 22:43:14)
> > After triggering the mm switch with a load of PD_DIR, which may be
> > deferred unto the MI_SET_CONTEXT on rcs, serialise the next commands
> > with that load by posting a read of PD_DIR (or else those subsequent
> > commands may access the stale page tables).
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Cc: Matthew Auld <matthew.william.auld@gmail.com>
> 
> Again, commit message checks out with code. Do you have your own
> bug-triaging flowchart that you come up with these W/As? :)

It's called running the tests that take too for CI :(

The lack of automation scares me, because I know how lazy and forgetful
I am.
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index d1cf8b4926ab..d278fed8cb31 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -499,7 +499,8 @@  void intel_engine_setup_common(struct intel_engine_cs *engine)
 	intel_engine_init_cmd_parser(engine);
 }
 
-int intel_engine_create_scratch(struct intel_engine_cs *engine, int size)
+int intel_engine_create_scratch(struct intel_engine_cs *engine,
+				unsigned int size)
 {
 	struct drm_i915_gem_object *obj;
 	struct i915_vma *vma;
@@ -533,7 +534,7 @@  int intel_engine_create_scratch(struct intel_engine_cs *engine, int size)
 	return ret;
 }
 
-static void intel_engine_cleanup_scratch(struct intel_engine_cs *engine)
+void intel_engine_cleanup_scratch(struct intel_engine_cs *engine)
 {
 	i915_vma_unpin_and_release(&engine->scratch);
 }
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 7970ecb199e2..66183eb3c102 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1362,8 +1362,9 @@  intel_ring_context_pin(struct intel_engine_cs *engine,
 
 static int intel_init_ring_buffer(struct intel_engine_cs *engine)
 {
-	struct intel_ring *ring;
 	struct i915_timeline *timeline;
+	struct intel_ring *ring;
+	unsigned int size;
 	int err;
 
 	intel_engine_setup_common(engine);
@@ -1389,12 +1390,21 @@  static int intel_init_ring_buffer(struct intel_engine_cs *engine)
 	GEM_BUG_ON(engine->buffer);
 	engine->buffer = ring;
 
-	err = intel_engine_init_common(engine);
+	size = PAGE_SIZE;
+	if (HAS_BROKEN_CS_TLB(engine->i915))
+		size = I830_WA_SIZE;
+	err = intel_engine_create_scratch(engine, size);
 	if (err)
 		goto err_unpin;
 
+	err = intel_engine_init_common(engine);
+	if (err)
+		goto err_scratch;
+
 	return 0;
 
+err_scratch:
+	intel_engine_cleanup_scratch(engine);
 err_unpin:
 	intel_ring_unpin(ring);
 err_ring:
@@ -1456,6 +1466,24 @@  static int load_pd_dir(struct i915_request *rq,
 	return 0;
 }
 
+static int flush_pd_dir(struct i915_request *rq)
+{
+	const struct intel_engine_cs * const engine = rq->engine;
+	u32 *cs;
+
+	cs = intel_ring_begin(rq, 4);
+	if (IS_ERR(cs))
+		return PTR_ERR(cs);
+
+	*cs++ = MI_STORE_REGISTER_MEM | MI_SRM_LRM_GLOBAL_GTT;
+	*cs++ = i915_mmio_reg_offset(RING_PP_DIR_BASE(engine));
+	*cs++ = i915_ggtt_offset(engine->scratch);
+	*cs++ = MI_NOOP;
+
+	intel_ring_advance(rq, cs);
+	return 0;
+}
+
 static inline int mi_set_context(struct i915_request *rq, u32 flags)
 {
 	struct drm_i915_private *i915 = rq->i915;
@@ -1634,6 +1662,12 @@  static int switch_context(struct i915_request *rq)
 			goto err_mm;
 	}
 
+	if (ppgtt) {
+		ret = flush_pd_dir(rq);
+		if (ret)
+			goto err_mm;
+	}
+
 	if (ctx->remap_slice) {
 		for (i = 0; i < MAX_L3_SLICES; i++) {
 			if (!(ctx->remap_slice & BIT(i)))
@@ -2154,16 +2188,6 @@  int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
 	if (ret)
 		return ret;
 
-	if (INTEL_GEN(dev_priv) >= 6) {
-		ret = intel_engine_create_scratch(engine, PAGE_SIZE);
-		if (ret)
-			return ret;
-	} else if (HAS_BROKEN_CS_TLB(dev_priv)) {
-		ret = intel_engine_create_scratch(engine, I830_WA_SIZE);
-		if (ret)
-			return ret;
-	}
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 1d8140ac2016..eba271e74c25 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -873,9 +873,12 @@  void intel_engine_init_global_seqno(struct intel_engine_cs *engine, u32 seqno);
 
 void intel_engine_setup_common(struct intel_engine_cs *engine);
 int intel_engine_init_common(struct intel_engine_cs *engine);
-int intel_engine_create_scratch(struct intel_engine_cs *engine, int size);
 void intel_engine_cleanup_common(struct intel_engine_cs *engine);
 
+int intel_engine_create_scratch(struct intel_engine_cs *engine,
+				unsigned int size);
+void intel_engine_cleanup_scratch(struct intel_engine_cs *engine);
+
 int intel_init_render_ring_buffer(struct intel_engine_cs *engine);
 int intel_init_bsd_ring_buffer(struct intel_engine_cs *engine);
 int intel_init_blt_ring_buffer(struct intel_engine_cs *engine);