[PATCH v2 1/4] drm/i915: Make LRC (un)pinning work on context and engine

[PATCH v2 1/4] drm/i915: Make LRC (un)pinning work on context and engine

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Previously intel_lr_context_(un)pin were operating on requests
which is in conflict with their names.

If we make them take a context and an engine, it makes the names
make more sense and it also makes future fixes possible.

v2: Rebase for default_context/kernel_context change.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Nick Hoath <nicholas.hoath@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c  |  2 +-
 drivers/gpu/drm/i915/intel_lrc.c | 49 ++++++++++++++++++++--------------------
 drivers/gpu/drm/i915/intel_lrc.h |  3 ++-
 3 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 371bbb28c471..28a28ebb3f16 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2680,7 +2680,7 @@ void i915_gem_request_free(struct kref *req_ref)
 
 	if (ctx) {
 		if (i915.enable_execlists && ctx != req->i915->kernel_context)
-			intel_lr_context_unpin(req);
+			intel_lr_context_unpin(ctx, req->ring);
 
 		i915_gem_context_unreference(ctx);
 	}
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 73d4347429df..28128acf23a3 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -225,7 +225,8 @@ enum {
 #define GEN8_CTX_ID_SHIFT 32
 #define CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT  0x17
 
-static int intel_lr_context_pin(struct drm_i915_gem_request *rq);
+static int intel_lr_context_pin(struct intel_context *ctx,
+				struct intel_engine_cs *engine);
 static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
 		struct drm_i915_gem_object *default_ctx_obj);
 
@@ -599,7 +600,7 @@ static int execlists_context_queue(struct drm_i915_gem_request *request)
 	int num_elements = 0;
 
 	if (request->ctx != request->i915->kernel_context)
-		intel_lr_context_pin(request);
+		intel_lr_context_pin(request->ctx, ring);
 
 	i915_gem_request_reference(request);
 
@@ -704,7 +705,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
 	}
 
 	if (request->ctx != request->i915->kernel_context)
-		ret = intel_lr_context_pin(request);
+		ret = intel_lr_context_pin(request->ctx, request->ring);
 
 	return ret;
 }
@@ -1015,7 +1016,8 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring)
 				ctx->engine[ring->id].state;
 
 		if (ctx_obj && (ctx != req->i915->kernel_context))
-			intel_lr_context_unpin(req);
+			intel_lr_context_unpin(ctx, ring);
+
 		list_del(&req->execlist_link);
 		i915_gem_request_unreference(req);
 	}
@@ -1059,8 +1061,8 @@ int logical_ring_flush_all_caches(struct drm_i915_gem_request *req)
 	return 0;
 }
 
-static int intel_lr_context_do_pin(struct intel_engine_cs *ring,
-				   struct intel_context *ctx)
+static int intel_lr_context_do_pin(struct intel_context *ctx,
+				   struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1103,41 +1105,40 @@ unpin_ctx_obj:
 	return ret;
 }
 
-static int intel_lr_context_pin(struct drm_i915_gem_request *rq)
+static int intel_lr_context_pin(struct intel_context *ctx,
+				struct intel_engine_cs *engine)
 {
 	int ret = 0;
-	struct intel_engine_cs *ring = rq->ring;
 
-	if (rq->ctx->engine[ring->id].pin_count++ == 0) {
-		ret = intel_lr_context_do_pin(ring, rq->ctx);
+	if (ctx->engine[engine->id].pin_count++ == 0) {
+		ret = intel_lr_context_do_pin(ctx, engine);
 		if (ret)
 			goto reset_pin_count;
 	}
 	return ret;
 
 reset_pin_count:
-	rq->ctx->engine[ring->id].pin_count = 0;
+	ctx->engine[engine->id].pin_count = 0;
 	return ret;
 }
 
-void intel_lr_context_unpin(struct drm_i915_gem_request *rq)
+void intel_lr_context_unpin(struct intel_context *ctx,
+			    struct intel_engine_cs *engine)
 {
-	struct intel_engine_cs *ring = rq->ring;
-	struct drm_i915_gem_object *ctx_obj = rq->ctx->engine[ring->id].state;
-	struct intel_ringbuffer *ringbuf = rq->ringbuf;
+	struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
 
-	WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
+	WARN_ON(!mutex_is_locked(&engine->dev->struct_mutex));
 
-	if (!ctx_obj)
+	if (WARN_ON_ONCE(!ctx_obj))
 		return;
 
-	if (--rq->ctx->engine[ring->id].pin_count == 0) {
-		kunmap(kmap_to_page(rq->ctx->engine[ring->id].lrc_reg_state));
-		intel_unpin_ringbuffer_obj(ringbuf);
+	if (--ctx->engine[engine->id].pin_count == 0) {
+		kunmap(kmap_to_page(ctx->engine[engine->id].lrc_reg_state));
+		intel_unpin_ringbuffer_obj(ctx->engine[engine->id].ringbuf);
 		i915_gem_object_ggtt_unpin(ctx_obj);
-		rq->ctx->engine[ring->id].lrc_vma = NULL;
-		rq->ctx->engine[ring->id].lrc_desc = 0;
-		rq->ctx->engine[ring->id].lrc_reg_state = NULL;
+		ctx->engine[engine->id].lrc_vma = NULL;
+		ctx->engine[engine->id].lrc_desc = 0;
+		ctx->engine[engine->id].lrc_reg_state = NULL;
 	}
 }
 
@@ -2062,7 +2063,7 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *ring)
 		goto error;
 
 	/* As this is the default context, always pin it */
-	ret = intel_lr_context_do_pin(ring, dctx);
+	ret = intel_lr_context_do_pin(dctx, ring);
 	if (ret) {
 		DRM_ERROR(
 			"Failed to pin and map ringbuffer %s: %d\n",
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 49af638f6213..e6cda3e225d0 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -101,7 +101,8 @@ void intel_lr_context_free(struct intel_context *ctx);
 uint32_t intel_lr_context_size(struct intel_engine_cs *ring);
 int intel_lr_context_deferred_alloc(struct intel_context *ctx,
 				    struct intel_engine_cs *ring);
-void intel_lr_context_unpin(struct drm_i915_gem_request *req);
+void intel_lr_context_unpin(struct intel_context *ctx,
+			    struct intel_engine_cs *engine);
 void intel_lr_context_reset(struct drm_device *dev,
 			struct intel_context *ctx);
 uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
-- 
1.9.1

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

[PATCH v2 2/4] drm/i915: Make LRC pinning own a reference to the context

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Will simplify the following fix and sounds logical.

v2: Add some whitespace to separate logic better. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Nick Hoath <nicholas.hoath@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 28128acf23a3..8eb6e364fefc 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1114,6 +1114,8 @@ static int intel_lr_context_pin(struct intel_context *ctx,
 		ret = intel_lr_context_do_pin(ctx, engine);
 		if (ret)
 			goto reset_pin_count;
+
+		i915_gem_context_reference(ctx);
 	}
 	return ret;
 
@@ -1139,6 +1141,8 @@ void intel_lr_context_unpin(struct intel_context *ctx,
 		ctx->engine[engine->id].lrc_vma = NULL;
 		ctx->engine[engine->id].lrc_desc = 0;
 		ctx->engine[engine->id].lrc_reg_state = NULL;
+
+		i915_gem_context_unreference(ctx);
 	}
 }
 
-- 
1.9.1

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

[PATCH 3/4] drm/i915: Extract context unpinning to its own function

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Will enable cleaner implementation of a following fix and
easier code unification in the future.

Idea and code by Chris Wilson.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 6a4f64b03db6..1a67e07b9e6a 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -321,6 +321,14 @@ err_destroy:
 	return ERR_PTR(ret);
 }
 
+static void i915_gem_context_unpin(struct intel_context *ctx,
+				   struct intel_engine_cs *engine)
+{
+	if (engine->id == RCS && ctx->legacy_hw_ctx.rcs_state)
+		i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
+	i915_gem_context_unreference(ctx);
+}
+
 void i915_gem_context_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -338,13 +346,9 @@ void i915_gem_context_reset(struct drm_device *dev)
 
 	for (i = 0; i < I915_NUM_RINGS; i++) {
 		struct intel_engine_cs *ring = &dev_priv->ring[i];
-		struct intel_context *lctx = ring->last_context;
-
-		if (lctx) {
-			if (lctx->legacy_hw_ctx.rcs_state && i == RCS)
-				i915_gem_object_ggtt_unpin(lctx->legacy_hw_ctx.rcs_state);
 
-			i915_gem_context_unreference(lctx);
+		if (ring->last_context) {
+			i915_gem_context_unpin(ring->last_context, ring);
 			ring->last_context = NULL;
 		}
 	}
@@ -417,13 +421,6 @@ void i915_gem_context_fini(struct drm_device *dev)
 		 * to offset the do_switch part, so that i915_gem_context_unreference()
 		 * can then free the base object correctly. */
 		WARN_ON(!dev_priv->ring[RCS].last_context);
-		if (dev_priv->ring[RCS].last_context == dctx) {
-			/* Fake switch to NULL context */
-			WARN_ON(dctx->legacy_hw_ctx.rcs_state->active);
-			i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
-			i915_gem_context_unreference(dctx);
-			dev_priv->ring[RCS].last_context = NULL;
-		}
 
 		i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
 	}
@@ -432,7 +429,7 @@ void i915_gem_context_fini(struct drm_device *dev)
 		struct intel_engine_cs *ring = &dev_priv->ring[i];
 
 		if (ring->last_context) {
-			i915_gem_context_unreference(ring->last_context);
+			i915_gem_context_unpin(ring->last_context, ring);
 			ring->last_context = NULL;
 		}
 	}
-- 
1.9.1

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

[PATCH v2] drm/i915: Extract context unpinning to its own function

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Will enable cleaner implementation of a following fix and
easier code unification in the future.

Idea and code by Chris Wilson.

v2: Do not return before last_contexts on engines are unpinned.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v1)
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 6a4f64b03db6..bba17b9f1be5 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -321,6 +321,14 @@ err_destroy:
 	return ERR_PTR(ret);
 }
 
+static void i915_gem_context_unpin(struct intel_context *ctx,
+				   struct intel_engine_cs *engine)
+{
+	if (engine->id == RCS && ctx->legacy_hw_ctx.rcs_state)
+		i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
+	i915_gem_context_unreference(ctx);
+}
+
 void i915_gem_context_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -329,22 +337,15 @@ void i915_gem_context_reset(struct drm_device *dev)
 	if (i915.enable_execlists) {
 		struct intel_context *ctx;
 
-		list_for_each_entry(ctx, &dev_priv->context_list, link) {
+		list_for_each_entry(ctx, &dev_priv->context_list, link)
 			intel_lr_context_reset(dev, ctx);
-		}
-
-		return;
 	}
 
 	for (i = 0; i < I915_NUM_RINGS; i++) {
 		struct intel_engine_cs *ring = &dev_priv->ring[i];
-		struct intel_context *lctx = ring->last_context;
-
-		if (lctx) {
-			if (lctx->legacy_hw_ctx.rcs_state && i == RCS)
-				i915_gem_object_ggtt_unpin(lctx->legacy_hw_ctx.rcs_state);
 
-			i915_gem_context_unreference(lctx);
+		if (ring->last_context) {
+			i915_gem_context_unpin(ring->last_context, ring);
 			ring->last_context = NULL;
 		}
 	}
@@ -417,13 +418,6 @@ void i915_gem_context_fini(struct drm_device *dev)
 		 * to offset the do_switch part, so that i915_gem_context_unreference()
 		 * can then free the base object correctly. */
 		WARN_ON(!dev_priv->ring[RCS].last_context);
-		if (dev_priv->ring[RCS].last_context == dctx) {
-			/* Fake switch to NULL context */
-			WARN_ON(dctx->legacy_hw_ctx.rcs_state->active);
-			i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
-			i915_gem_context_unreference(dctx);
-			dev_priv->ring[RCS].last_context = NULL;
-		}
 
 		i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
 	}
@@ -432,7 +426,7 @@ void i915_gem_context_fini(struct drm_device *dev)
 		struct intel_engine_cs *ring = &dev_priv->ring[i];
 
 		if (ring->last_context) {
-			i915_gem_context_unreference(ring->last_context);
+			i915_gem_context_unpin(ring->last_context, ring);
 			ring->last_context = NULL;
 		}
 	}
-- 
1.9.1

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

[PATCH v4 4/4] drm/i915: Fix premature LRC unpin in GuC mode

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

In GuC mode LRC pinning lifetime depends exclusively on the
request liftime. Since that is terminated by the seqno update
that opens up a race condition between GPU finishing writing
out the context image and the driver unpinning the LRC.

To extend the LRC lifetime we will employ a similar approach
to what legacy ringbuffer submission does.

We will start tracking the last submitted context per engine
and keep it pinned until it is replaced by another one.

Note that the driver unload path is a bit fragile and could
benefit greatly from efforts to unify the legacy and exec
list submission code paths.

At the moment i915_gem_context_fini has special casing for the
two which are potentialy not needed, and also depends on
i915_gem_cleanup_ringbuffer running before itself.

v2:
 * Move pinning into engine->emit_request and actually fix
   the reference/unreference logic. (Chris Wilson)

 * ring->dev can be NULL on driver unload so use a different
   route towards it.

v3:
 * Rebase.
 * Handle the reset path. (Chris Wilson)
 * Exclude default context from the pinning - it is impossible
   to get it right before default context special casing in
   general is eliminated.

v4:
 * Rebased & moved context tracking to
   intel_logical_ring_advance_and_submit.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Issue: VIZ-4277
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Nick Hoath <nicholas.hoath@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 10 +++++++---
 drivers/gpu/drm/i915/intel_lrc.c        | 16 ++++++++++++++--
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 1a67e07b9e6a..7e6f8c7b6d01 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -324,9 +324,13 @@ err_destroy:
 static void i915_gem_context_unpin(struct intel_context *ctx,
 				   struct intel_engine_cs *engine)
 {
-	if (engine->id == RCS && ctx->legacy_hw_ctx.rcs_state)
-		i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
-	i915_gem_context_unreference(ctx);
+	if (i915.enable_execlists) {
+		intel_lr_context_unpin(ctx, engine);
+	} else {
+		if (engine->id == RCS && ctx->legacy_hw_ctx.rcs_state)
+			i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
+		i915_gem_context_unreference(ctx);
+	}
 }
 
 void i915_gem_context_reset(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 8eb6e364fefc..0e215ea3f8ab 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -766,6 +766,7 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
 {
 	struct intel_ringbuffer *ringbuf = request->ringbuf;
 	struct drm_i915_private *dev_priv = request->i915;
+	struct intel_engine_cs *engine = request->ring;
 
 	intel_logical_ring_advance(ringbuf);
 	request->tail = ringbuf->tail;
@@ -780,9 +781,20 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
 	intel_logical_ring_emit(ringbuf, MI_NOOP);
 	intel_logical_ring_advance(ringbuf);
 
-	if (intel_ring_stopped(request->ring))
+	if (intel_ring_stopped(engine))
 		return 0;
 
+	if (engine->last_context != request->ctx) {
+		if (engine->last_context)
+			intel_lr_context_unpin(engine->last_context, engine);
+		if (request->ctx != request->i915->kernel_context) {
+			intel_lr_context_pin(request->ctx, engine);
+			engine->last_context = request->ctx;
+		} else {
+			engine->last_context = NULL;
+		}
+	}
+
 	if (dev_priv->guc.execbuf_client)
 		i915_guc_submit(dev_priv->guc.execbuf_client, request);
 	else
@@ -1129,7 +1141,7 @@ void intel_lr_context_unpin(struct intel_context *ctx,
 {
 	struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
 
-	WARN_ON(!mutex_is_locked(&engine->dev->struct_mutex));
+	WARN_ON(!mutex_is_locked(&ctx->i915->dev->struct_mutex));
 
 	if (WARN_ON_ONCE(!ctx_obj))
 		return;
-- 
1.9.1

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

Re: [PATCH v4 4/4] drm/i915: Fix premature LRC unpin in GuC mode

[Chris Wilson]

On Thu, Jan 21, 2016 at 12:14:10PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> In GuC mode LRC pinning lifetime depends exclusively on the
> request liftime. Since that is terminated by the seqno update
> that opens up a race condition between GPU finishing writing
> out the context image and the driver unpinning the LRC.
> 
> To extend the LRC lifetime we will employ a similar approach
> to what legacy ringbuffer submission does.
> 
> We will start tracking the last submitted context per engine
> and keep it pinned until it is replaced by another one.
> 
> Note that the driver unload path is a bit fragile and could
> benefit greatly from efforts to unify the legacy and exec
> list submission code paths.
> 
> At the moment i915_gem_context_fini has special casing for the
> two which are potentialy not needed, and also depends on
> i915_gem_cleanup_ringbuffer running before itself.
> 
> v2:
>  * Move pinning into engine->emit_request and actually fix
>    the reference/unreference logic. (Chris Wilson)
> 
>  * ring->dev can be NULL on driver unload so use a different
>    route towards it.
> 
> v3:
>  * Rebase.
>  * Handle the reset path. (Chris Wilson)
>  * Exclude default context from the pinning - it is impossible
>    to get it right before default context special casing in
>    general is eliminated.
> 
> v4:
>  * Rebased & moved context tracking to
>    intel_logical_ring_advance_and_submit.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Issue: VIZ-4277
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Nick Hoath <nicholas.hoath@intel.com>

Whilst it saddens me to see yet another (impossible) special case added
that will just have to be deleted again, the series is
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

I wonder if it is possible to poison the context objects before and
after, then do a deferred check for stray writes, and use that mode for
igt/gem_ctx_* (with some tests targetting active->idle vs
context-close). Would still be susceptible to timing as we need to
hit the interval between the seqno being complete and the delayed context
save, but that seems like the most reliable way to detect the error?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/4] drm/i915: Fix premature LRC unpin in GuC mode

[Tvrtko Ursulin]

On 21/01/16 12:32, Chris Wilson wrote:
> On Thu, Jan 21, 2016 at 12:14:10PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> In GuC mode LRC pinning lifetime depends exclusively on the
>> request liftime. Since that is terminated by the seqno update
>> that opens up a race condition between GPU finishing writing
>> out the context image and the driver unpinning the LRC.
>>
>> To extend the LRC lifetime we will employ a similar approach
>> to what legacy ringbuffer submission does.
>>
>> We will start tracking the last submitted context per engine
>> and keep it pinned until it is replaced by another one.
>>
>> Note that the driver unload path is a bit fragile and could
>> benefit greatly from efforts to unify the legacy and exec
>> list submission code paths.
>>
>> At the moment i915_gem_context_fini has special casing for the
>> two which are potentialy not needed, and also depends on
>> i915_gem_cleanup_ringbuffer running before itself.
>>
>> v2:
>>   * Move pinning into engine->emit_request and actually fix
>>     the reference/unreference logic. (Chris Wilson)
>>
>>   * ring->dev can be NULL on driver unload so use a different
>>     route towards it.
>>
>> v3:
>>   * Rebase.
>>   * Handle the reset path. (Chris Wilson)
>>   * Exclude default context from the pinning - it is impossible
>>     to get it right before default context special casing in
>>     general is eliminated.
>>
>> v4:
>>   * Rebased & moved context tracking to
>>     intel_logical_ring_advance_and_submit.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Issue: VIZ-4277
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Nick Hoath <nicholas.hoath@intel.com>
>
> Whilst it saddens me to see yet another (impossible) special case added
> that will just have to be deleted again, the series is
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks and sorry, hopefully it will get cleanup up soon. There seems to 
be a growing number of people who want it done.

And I still need to get back to your VMA rewrite and breadcrumbs would 
be nice as well.

> I wonder if it is possible to poison the context objects before and
> after, then do a deferred check for stray writes, and use that mode for
> igt/gem_ctx_* (with some tests targetting active->idle vs
> context-close). Would still be susceptible to timing as we need to
> hit the interval between the seqno being complete and the delayed context
> save, but that seems like the most reliable way to detect the error?

First it needs to be tested with GuC to check that it actually fixes the 
issue. And pass CI of course.

But I can't really figure where would you put this poisoning? You could 
put something in in exec list mode after context complete and check it 
before it is used next time, but I did not think we can hit this in exec 
list mode, only in GuC. You think it is possible?

And in GuC mode I have no idea at which point you would put "poisoning" in?

Regards,

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

Re: [PATCH v4 4/4] drm/i915: Fix premature LRC unpin in GuC mode

[Chris Wilson]

On Thu, Jan 21, 2016 at 01:51:30PM +0000, Tvrtko Ursulin wrote:
> But I can't really figure where would you put this poisoning? You
> could put something in in exec list mode after context complete and
> check it before it is used next time,

I was thinking just in context-free. Move the pages to a poisoned list
and check at the end of the test. The issue is that the GPU may write to
the pages as we release them, so instead of releasing them we just
poison them (or CRC them).

> but I did not think we can hit
> this in exec list mode, only in GuC. You think it is possible?

With the current code, no since the last context unreference is done
from i915_gem_request_free(), and the request reference is only dropped
after we see the context-switch/active->idle state change (i.e. the
context save should always be flushed by the time we unpin and free the
context). However, that ordering imposes the struct_mutex upon
request-free which leads to fairly severe issues that can be eleviated
by moving the context-unreference into the request-retire - which opens
up the context-close race to normal execlists, as the context can be
unreferenced before the next context-switch-interrupt, now fixed in this
patch. And reducing the context-pin lifetime even further should help
mitigate context thrashing and certain userspace stress tests (OpenGL
microbenchmarks).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [v2,1/4] drm/i915: Make LRC (un)pinning work on context and engine

[Patchwork]

== Summary ==

Built on 8fe9e785ae04fa7c37f7935cff12d62e38054b60 drm-intel-nightly: 2016y-01m-21d-11h-02m-42s UTC integration manifest

Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (ilk-hp8440p)

bdw-nuci7        total:140  pass:131  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:143  pass:137  dwarn:0   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:143  pass:119  dwarn:0   dfail:0   fail:0   skip:24 
byt-nuc          total:143  pass:128  dwarn:0   dfail:0   fail:0   skip:15 
hsw-brixbox      total:143  pass:136  dwarn:0   dfail:0   fail:0   skip:7  
ilk-hp8440p      total:143  pass:103  dwarn:2   dfail:0   fail:0   skip:38 
ivb-t430s        total:143  pass:137  dwarn:0   dfail:0   fail:0   skip:6  
skl-i5k-2        total:143  pass:134  dwarn:1   dfail:0   fail:0   skip:8  
skl-i7k-2        total:143  pass:134  dwarn:1   dfail:0   fail:0   skip:8  
snb-dellxps      total:143  pass:129  dwarn:0   dfail:0   fail:0   skip:14 
snb-x220t        total:143  pass:129  dwarn:0   dfail:0   fail:1   skip:13 

HANGED hsw-gt2 in igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a

Results at /archive/results/CI_IGT_test/Patchwork_1237/

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

[PATCH v2 1/4] drm/i915: Make LRC (un)pinning work on context and engine

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Previously intel_lr_context_(un)pin were operating on requests
which is in conflict with their names.

If we make them take a context and an engine, it makes the names
make more sense and it also makes future fixes possible.

v2: Rebase for default_context/kernel_context change.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Nick Hoath <nicholas.hoath@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c  |  2 +-
 drivers/gpu/drm/i915/intel_lrc.c | 49 ++++++++++++++++++++--------------------
 drivers/gpu/drm/i915/intel_lrc.h |  3 ++-
 3 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 371bbb28c471..28a28ebb3f16 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2680,7 +2680,7 @@ void i915_gem_request_free(struct kref *req_ref)
 
 	if (ctx) {
 		if (i915.enable_execlists && ctx != req->i915->kernel_context)
-			intel_lr_context_unpin(req);
+			intel_lr_context_unpin(ctx, req->ring);
 
 		i915_gem_context_unreference(ctx);
 	}
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index da97bc5666b5..6c95aa19cc90 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -225,7 +225,8 @@ enum {
 #define GEN8_CTX_ID_SHIFT 32
 #define CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT  0x17
 
-static int intel_lr_context_pin(struct drm_i915_gem_request *rq);
+static int intel_lr_context_pin(struct intel_context *ctx,
+				struct intel_engine_cs *engine);
 static void lrc_setup_hardware_status_page(struct intel_engine_cs *ring,
 		struct drm_i915_gem_object *default_ctx_obj);
 
@@ -598,7 +599,7 @@ static int execlists_context_queue(struct drm_i915_gem_request *request)
 	int num_elements = 0;
 
 	if (request->ctx != request->i915->kernel_context)
-		intel_lr_context_pin(request);
+		intel_lr_context_pin(request->ctx, ring);
 
 	i915_gem_request_reference(request);
 
@@ -703,7 +704,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
 	}
 
 	if (request->ctx != request->i915->kernel_context)
-		ret = intel_lr_context_pin(request);
+		ret = intel_lr_context_pin(request->ctx, request->ring);
 
 	return ret;
 }
@@ -1014,7 +1015,8 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring)
 				ctx->engine[ring->id].state;
 
 		if (ctx_obj && (ctx != req->i915->kernel_context))
-			intel_lr_context_unpin(req);
+			intel_lr_context_unpin(ctx, ring);
+
 		list_del(&req->execlist_link);
 		i915_gem_request_unreference(req);
 	}
@@ -1058,8 +1060,8 @@ int logical_ring_flush_all_caches(struct drm_i915_gem_request *req)
 	return 0;
 }
 
-static int intel_lr_context_do_pin(struct intel_engine_cs *ring,
-				   struct intel_context *ctx)
+static int intel_lr_context_do_pin(struct intel_context *ctx,
+				   struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1105,41 +1107,40 @@ unpin_ctx_obj:
 	return ret;
 }
 
-static int intel_lr_context_pin(struct drm_i915_gem_request *rq)
+static int intel_lr_context_pin(struct intel_context *ctx,
+				struct intel_engine_cs *engine)
 {
 	int ret = 0;
-	struct intel_engine_cs *ring = rq->ring;
 
-	if (rq->ctx->engine[ring->id].pin_count++ == 0) {
-		ret = intel_lr_context_do_pin(ring, rq->ctx);
+	if (ctx->engine[engine->id].pin_count++ == 0) {
+		ret = intel_lr_context_do_pin(ctx, engine);
 		if (ret)
 			goto reset_pin_count;
 	}
 	return ret;
 
 reset_pin_count:
-	rq->ctx->engine[ring->id].pin_count = 0;
+	ctx->engine[engine->id].pin_count = 0;
 	return ret;
 }
 
-void intel_lr_context_unpin(struct drm_i915_gem_request *rq)
+void intel_lr_context_unpin(struct intel_context *ctx,
+			    struct intel_engine_cs *engine)
 {
-	struct intel_engine_cs *ring = rq->ring;
-	struct drm_i915_gem_object *ctx_obj = rq->ctx->engine[ring->id].state;
-	struct intel_ringbuffer *ringbuf = rq->ringbuf;
+	struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
 
-	WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex));
+	WARN_ON(!mutex_is_locked(&engine->dev->struct_mutex));
 
-	if (!ctx_obj)
+	if (WARN_ON_ONCE(!ctx_obj))
 		return;
 
-	if (--rq->ctx->engine[ring->id].pin_count == 0) {
-		kunmap(kmap_to_page(rq->ctx->engine[ring->id].lrc_reg_state));
-		intel_unpin_ringbuffer_obj(ringbuf);
+	if (--ctx->engine[engine->id].pin_count == 0) {
+		kunmap(kmap_to_page(ctx->engine[engine->id].lrc_reg_state));
+		intel_unpin_ringbuffer_obj(ctx->engine[engine->id].ringbuf);
 		i915_gem_object_ggtt_unpin(ctx_obj);
-		rq->ctx->engine[ring->id].lrc_vma = NULL;
-		rq->ctx->engine[ring->id].lrc_desc = 0;
-		rq->ctx->engine[ring->id].lrc_reg_state = NULL;
+		ctx->engine[engine->id].lrc_vma = NULL;
+		ctx->engine[engine->id].lrc_desc = 0;
+		ctx->engine[engine->id].lrc_reg_state = NULL;
 	}
 }
 
@@ -2064,7 +2065,7 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *ring)
 		goto error;
 
 	/* As this is the default context, always pin it */
-	ret = intel_lr_context_do_pin(ring, dctx);
+	ret = intel_lr_context_do_pin(dctx, ring);
 	if (ret) {
 		DRM_ERROR(
 			"Failed to pin and map ringbuffer %s: %d\n",
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 49af638f6213..e6cda3e225d0 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -101,7 +101,8 @@ void intel_lr_context_free(struct intel_context *ctx);
 uint32_t intel_lr_context_size(struct intel_engine_cs *ring);
 int intel_lr_context_deferred_alloc(struct intel_context *ctx,
 				    struct intel_engine_cs *ring);
-void intel_lr_context_unpin(struct drm_i915_gem_request *req);
+void intel_lr_context_unpin(struct intel_context *ctx,
+			    struct intel_engine_cs *engine);
 void intel_lr_context_reset(struct drm_device *dev,
 			struct intel_context *ctx);
 uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
-- 
1.9.1

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

[PATCH v4 4/4] drm/i915: Fix premature LRC unpin in GuC mode

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

In GuC mode LRC pinning lifetime depends exclusively on the
request liftime. Since that is terminated by the seqno update
that opens up a race condition between GPU finishing writing
out the context image and the driver unpinning the LRC.

To extend the LRC lifetime we will employ a similar approach
to what legacy ringbuffer submission does.

We will start tracking the last submitted context per engine
and keep it pinned until it is replaced by another one.

Note that the driver unload path is a bit fragile and could
benefit greatly from efforts to unify the legacy and exec
list submission code paths.

At the moment i915_gem_context_fini has special casing for the
two which are potentialy not needed, and also depends on
i915_gem_cleanup_ringbuffer running before itself.

v2:
 * Move pinning into engine->emit_request and actually fix
   the reference/unreference logic. (Chris Wilson)

 * ring->dev can be NULL on driver unload so use a different
   route towards it.

v3:
 * Rebase.
 * Handle the reset path. (Chris Wilson)
 * Exclude default context from the pinning - it is impossible
   to get it right before default context special casing in
   general is eliminated.

v4:
 * Rebased & moved context tracking to
   intel_logical_ring_advance_and_submit.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Issue: VIZ-4277
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Nick Hoath <nicholas.hoath@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 10 +++++++---
 drivers/gpu/drm/i915/intel_lrc.c        | 16 ++++++++++++++--
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index bba17b9f1be5..83a097c94911 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -324,9 +324,13 @@ err_destroy:
 static void i915_gem_context_unpin(struct intel_context *ctx,
 				   struct intel_engine_cs *engine)
 {
-	if (engine->id == RCS && ctx->legacy_hw_ctx.rcs_state)
-		i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
-	i915_gem_context_unreference(ctx);
+	if (i915.enable_execlists) {
+		intel_lr_context_unpin(ctx, engine);
+	} else {
+		if (engine->id == RCS && ctx->legacy_hw_ctx.rcs_state)
+			i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
+		i915_gem_context_unreference(ctx);
+	}
 }
 
 void i915_gem_context_reset(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 61e64d78adbd..3a03646e343d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -765,6 +765,7 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
 {
 	struct intel_ringbuffer *ringbuf = request->ringbuf;
 	struct drm_i915_private *dev_priv = request->i915;
+	struct intel_engine_cs *engine = request->ring;
 
 	intel_logical_ring_advance(ringbuf);
 	request->tail = ringbuf->tail;
@@ -779,9 +780,20 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
 	intel_logical_ring_emit(ringbuf, MI_NOOP);
 	intel_logical_ring_advance(ringbuf);
 
-	if (intel_ring_stopped(request->ring))
+	if (intel_ring_stopped(engine))
 		return 0;
 
+	if (engine->last_context != request->ctx) {
+		if (engine->last_context)
+			intel_lr_context_unpin(engine->last_context, engine);
+		if (request->ctx != request->i915->kernel_context) {
+			intel_lr_context_pin(request->ctx, engine);
+			engine->last_context = request->ctx;
+		} else {
+			engine->last_context = NULL;
+		}
+	}
+
 	if (dev_priv->guc.execbuf_client)
 		i915_guc_submit(dev_priv->guc.execbuf_client, request);
 	else
@@ -1131,7 +1143,7 @@ void intel_lr_context_unpin(struct intel_context *ctx,
 {
 	struct drm_i915_gem_object *ctx_obj = ctx->engine[engine->id].state;
 
-	WARN_ON(!mutex_is_locked(&engine->dev->struct_mutex));
+	WARN_ON(!mutex_is_locked(&ctx->i915->dev->struct_mutex));
 
 	if (WARN_ON_ONCE(!ctx_obj))
 		return;
-- 
1.9.1

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