All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Only skip requests once a context is banned
@ 2017-01-03 11:59 Chris Wilson
  2017-01-03 13:49 ` Tvrtko Ursulin
  0 siblings, 1 reply; 4+ messages in thread
From: Chris Wilson @ 2017-01-03 11:59 UTC (permalink / raw)
  To: intel-gfx; +Cc: Mika Kuoppala

If we skip before banning, we have an inconsistent interface between
execbuf still queueing valid request but those requests already queued
being cancelled. If we only cancel the pending requests once we stop
accepting new requests, the interface is more consistent.

Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Fixes: 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete requests")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0bfc99fcac8c..3d0eb9391d3d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2739,7 +2739,7 @@ static void reset_request(struct drm_i915_gem_request *request)
 static void i915_gem_reset_engine(struct intel_engine_cs *engine)
 {
 	struct drm_i915_gem_request *request;
-	struct i915_gem_context *incomplete_ctx;
+	struct i915_gem_context *hung_ctx;
 	struct intel_timeline *timeline;
 	unsigned long flags;
 	bool ring_hung;
@@ -2751,6 +2751,8 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
 	if (!request)
 		return;
 
+	hung_ctx = request->ctx;
+
 	ring_hung = engine->hangcheck.stalled;
 	if (engine->hangcheck.seqno != intel_engine_get_seqno(engine)) {
 		DRM_DEBUG_DRIVER("%s pardoned, was guilty? %s\n",
@@ -2760,10 +2762,10 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
 	}
 
 	if (ring_hung) {
-		i915_gem_context_mark_guilty(request->ctx);
+		i915_gem_context_mark_guilty(hung_ctx);
 		request->fence.status = -EIO;
 	} else {
-		i915_gem_context_mark_innocent(request->ctx);
+		i915_gem_context_mark_innocent(hung_ctx);
 	}
 
 	if (!ring_hung)
@@ -2775,6 +2777,10 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
 	/* Setup the CS to resume from the breadcrumb of the hung request */
 	engine->reset_hw(engine, request);
 
+	/* If this context is now banned, skip all of its pending requests. */
+	if (!i915_gem_context_is_banned(hung_ctx))
+		return;
+
 	/* Users of the default context do not rely on logical state
 	 * preserved between batches. They have to emit full state on
 	 * every batch and so it is safe to execute queued requests following
@@ -2783,17 +2789,16 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
 	 * Other contexts preserve state, now corrupt. We want to skip all
 	 * queued requests that reference the corrupt context.
 	 */
-	incomplete_ctx = request->ctx;
-	if (i915_gem_context_is_default(incomplete_ctx))
+	if (i915_gem_context_is_default(hung_ctx))
 		return;
 
-	timeline = i915_gem_context_lookup_timeline(incomplete_ctx, engine);
+	timeline = i915_gem_context_lookup_timeline(hung_ctx, engine);
 
 	spin_lock_irqsave(&engine->timeline->lock, flags);
 	spin_lock(&timeline->lock);
 
 	list_for_each_entry_continue(request, &engine->timeline->requests, link)
-		if (request->ctx == incomplete_ctx)
+		if (request->ctx == hung_ctx)
 			reset_request(request);
 
 	list_for_each_entry(request, &timeline->requests, link)
-- 
2.11.0

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: Only skip requests once a context is banned
  2017-01-03 11:59 [PATCH] drm/i915: Only skip requests once a context is banned Chris Wilson
@ 2017-01-03 13:49 ` Tvrtko Ursulin
  0 siblings, 0 replies; 4+ messages in thread
From: Tvrtko Ursulin @ 2017-01-03 13:49 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: Mika Kuoppala


On 03/01/2017 11:59, Chris Wilson wrote:
> If we skip before banning, we have an inconsistent interface between
> execbuf still queueing valid request but those requests already queued
> being cancelled. If we only cancel the pending requests once we stop
> accepting new requests, the interface is more consistent.
>
> Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Fixes: 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete requests")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0bfc99fcac8c..3d0eb9391d3d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2739,7 +2739,7 @@ static void reset_request(struct drm_i915_gem_request *request)
>  static void i915_gem_reset_engine(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_gem_request *request;
> -	struct i915_gem_context *incomplete_ctx;
> +	struct i915_gem_context *hung_ctx;
>  	struct intel_timeline *timeline;
>  	unsigned long flags;
>  	bool ring_hung;
> @@ -2751,6 +2751,8 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
>  	if (!request)
>  		return;
>
> +	hung_ctx = request->ctx;
> +
>  	ring_hung = engine->hangcheck.stalled;
>  	if (engine->hangcheck.seqno != intel_engine_get_seqno(engine)) {
>  		DRM_DEBUG_DRIVER("%s pardoned, was guilty? %s\n",
> @@ -2760,10 +2762,10 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
>  	}
>
>  	if (ring_hung) {
> -		i915_gem_context_mark_guilty(request->ctx);
> +		i915_gem_context_mark_guilty(hung_ctx);
>  		request->fence.status = -EIO;
>  	} else {
> -		i915_gem_context_mark_innocent(request->ctx);
> +		i915_gem_context_mark_innocent(hung_ctx);
>  	}
>
>  	if (!ring_hung)
> @@ -2775,6 +2777,10 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
>  	/* Setup the CS to resume from the breadcrumb of the hung request */
>  	engine->reset_hw(engine, request);
>
> +	/* If this context is now banned, skip all of its pending requests. */
> +	if (!i915_gem_context_is_banned(hung_ctx))
> +		return;
> +
>  	/* Users of the default context do not rely on logical state
>  	 * preserved between batches. They have to emit full state on
>  	 * every batch and so it is safe to execute queued requests following
> @@ -2783,17 +2789,16 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
>  	 * Other contexts preserve state, now corrupt. We want to skip all
>  	 * queued requests that reference the corrupt context.
>  	 */
> -	incomplete_ctx = request->ctx;
> -	if (i915_gem_context_is_default(incomplete_ctx))
> +	if (i915_gem_context_is_default(hung_ctx))
>  		return;
>
> -	timeline = i915_gem_context_lookup_timeline(incomplete_ctx, engine);
> +	timeline = i915_gem_context_lookup_timeline(hung_ctx, engine);
>
>  	spin_lock_irqsave(&engine->timeline->lock, flags);
>  	spin_lock(&timeline->lock);
>
>  	list_for_each_entry_continue(request, &engine->timeline->requests, link)
> -		if (request->ctx == incomplete_ctx)
> +		if (request->ctx == hung_ctx)
>  			reset_request(request);
>
>  	list_for_each_entry(request, &timeline->requests, link)
>

LGTM, but hopefully Mika can also double-check.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

^ permalink raw reply	[flat|nested] 4+ messages in thread
* [PATCH] drm/i915: Only skip requests once a context is banned
@ 2017-01-05 17:00 ` Chris Wilson
  0 siblings, 0 replies; 4+ messages in thread
From: Chris Wilson @ 2017-01-05 17:00 UTC (permalink / raw)
  To: intel-gfx; +Cc: Chris Wilson, Tvrtko Ursulin, Mika Kuoppala, # v4 . 9+

If we skip before banning, we have an inconsistent interface between
execbuf still queueing valid request but those requests already queued
being cancelled. If we only cancel the pending requests once we stop
accepting new requests, the user interface is more consistent.

Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Fixes: 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete requests")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: <stable@vger.kernel.org> # v4.9+
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 70ddff3570bb..e60467271898 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2734,7 +2734,7 @@ void i915_gem_reset_prepare(struct drm_i915_private *dev_priv)
 static void i915_gem_reset_engine(struct intel_engine_cs *engine)
 {
 	struct drm_i915_gem_request *request;
-	struct i915_gem_context *incomplete_ctx;
+	struct i915_gem_context *hung_ctx;
 	struct intel_timeline *timeline;
 	unsigned long flags;
 	bool ring_hung;
@@ -2746,6 +2746,8 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
 	if (!request)
 		return;
 
+	hung_ctx = request->ctx;
+
 	ring_hung = engine->hangcheck.stalled;
 	if (engine->hangcheck.seqno != intel_engine_get_seqno(engine)) {
 		DRM_DEBUG_DRIVER("%s pardoned, was guilty? %s\n",
@@ -2755,9 +2757,9 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
 	}
 
 	if (ring_hung)
-		i915_gem_context_mark_guilty(request->ctx);
+		i915_gem_context_mark_guilty(hung_ctx);
 	else
-		i915_gem_context_mark_innocent(request->ctx);
+		i915_gem_context_mark_innocent(hung_ctx);
 
 	if (!ring_hung)
 		return;
@@ -2768,6 +2770,10 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
 	/* Setup the CS to resume from the breadcrumb of the hung request */
 	engine->reset_hw(engine, request);
 
+	/* If this context is now banned, skip all of its pending requests. */
+	if (!i915_gem_context_is_banned(hung_ctx))
+		return;
+
 	/* Users of the default context do not rely on logical state
 	 * preserved between batches. They have to emit full state on
 	 * every batch and so it is safe to execute queued requests following
@@ -2776,17 +2782,16 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
 	 * Other contexts preserve state, now corrupt. We want to skip all
 	 * queued requests that reference the corrupt context.
 	 */
-	incomplete_ctx = request->ctx;
-	if (i915_gem_context_is_default(incomplete_ctx))
+	if (i915_gem_context_is_default(hung_ctx))
 		return;
 
-	timeline = i915_gem_context_lookup_timeline(incomplete_ctx, engine);
+	timeline = i915_gem_context_lookup_timeline(hung_ctx, engine);
 
 	spin_lock_irqsave(&engine->timeline->lock, flags);
 	spin_lock(&timeline->lock);
 
 	list_for_each_entry_continue(request, &engine->timeline->requests, link)
-		if (request->ctx == incomplete_ctx)
+		if (request->ctx == hung_ctx)
 			reset_request(request);
 
 	list_for_each_entry(request, &timeline->requests, link)
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* [PATCH] drm/i915: Only skip requests once a context is banned
@ 2017-01-05 17:00 ` Chris Wilson
  0 siblings, 0 replies; 4+ messages in thread
From: Chris Wilson @ 2017-01-05 17:00 UTC (permalink / raw)
  To: intel-gfx; +Cc: Mika Kuoppala, # v4 . 9+

If we skip before banning, we have an inconsistent interface between
execbuf still queueing valid request but those requests already queued
being cancelled. If we only cancel the pending requests once we stop
accepting new requests, the user interface is more consistent.

Reported-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Fixes: 821ed7df6e2a ("drm/i915: Update reset path to fix incomplete requests")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: <stable@vger.kernel.org> # v4.9+
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 70ddff3570bb..e60467271898 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2734,7 +2734,7 @@ void i915_gem_reset_prepare(struct drm_i915_private *dev_priv)
 static void i915_gem_reset_engine(struct intel_engine_cs *engine)
 {
 	struct drm_i915_gem_request *request;
-	struct i915_gem_context *incomplete_ctx;
+	struct i915_gem_context *hung_ctx;
 	struct intel_timeline *timeline;
 	unsigned long flags;
 	bool ring_hung;
@@ -2746,6 +2746,8 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
 	if (!request)
 		return;
 
+	hung_ctx = request->ctx;
+
 	ring_hung = engine->hangcheck.stalled;
 	if (engine->hangcheck.seqno != intel_engine_get_seqno(engine)) {
 		DRM_DEBUG_DRIVER("%s pardoned, was guilty? %s\n",
@@ -2755,9 +2757,9 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
 	}
 
 	if (ring_hung)
-		i915_gem_context_mark_guilty(request->ctx);
+		i915_gem_context_mark_guilty(hung_ctx);
 	else
-		i915_gem_context_mark_innocent(request->ctx);
+		i915_gem_context_mark_innocent(hung_ctx);
 
 	if (!ring_hung)
 		return;
@@ -2768,6 +2770,10 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
 	/* Setup the CS to resume from the breadcrumb of the hung request */
 	engine->reset_hw(engine, request);
 
+	/* If this context is now banned, skip all of its pending requests. */
+	if (!i915_gem_context_is_banned(hung_ctx))
+		return;
+
 	/* Users of the default context do not rely on logical state
 	 * preserved between batches. They have to emit full state on
 	 * every batch and so it is safe to execute queued requests following
@@ -2776,17 +2782,16 @@ static void i915_gem_reset_engine(struct intel_engine_cs *engine)
 	 * Other contexts preserve state, now corrupt. We want to skip all
 	 * queued requests that reference the corrupt context.
 	 */
-	incomplete_ctx = request->ctx;
-	if (i915_gem_context_is_default(incomplete_ctx))
+	if (i915_gem_context_is_default(hung_ctx))
 		return;
 
-	timeline = i915_gem_context_lookup_timeline(incomplete_ctx, engine);
+	timeline = i915_gem_context_lookup_timeline(hung_ctx, engine);
 
 	spin_lock_irqsave(&engine->timeline->lock, flags);
 	spin_lock(&timeline->lock);
 
 	list_for_each_entry_continue(request, &engine->timeline->requests, link)
-		if (request->ctx == incomplete_ctx)
+		if (request->ctx == hung_ctx)
 			reset_request(request);
 
 	list_for_each_entry(request, &timeline->requests, link)
-- 
2.11.0

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-01-05 17:01 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-03 11:59 [PATCH] drm/i915: Only skip requests once a context is banned Chris Wilson
2017-01-03 13:49 ` Tvrtko Ursulin
2017-01-05 17:00 Chris Wilson
2017-01-05 17:00 ` 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.