[PATCH 1/2] drm/i915: put context upon switching

[PATCH 1/2] drm/i915: put context upon switching

[Mika Kuoppala]

From: Chris Wilson <chris@chris-wilson.co.uk>

In order to be notified of when the context and all of its associated
objects is idle (for if the context maps to a ppgtt) we need a callback
from the retire handler. We can arrange this by using the kref_get/put
of the context for request tracking and by inserting a request to
demarque the switch away from the old context.

[Ben: fixed minor error to patch compile, AND s/last_context/from/]
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c |   37 ++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_ringbuffer.h |    2 +-
 2 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 9e8c685..817222a 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -353,13 +353,13 @@ mi_set_context(struct intel_ring_buffer *ring,
 static int do_switch(struct i915_hw_context *to)
 {
 	struct intel_ring_buffer *ring = to->ring;
-	struct drm_i915_gem_object *from_obj = ring->last_context_obj;
+	struct i915_hw_context *from = ring->last_context;
 	u32 hw_flags = 0;
 	int ret;
 
-	BUG_ON(from_obj != NULL && from_obj->pin_count == 0);
+	BUG_ON(from != NULL && from->obj != NULL && from->obj->pin_count == 0);
 
-	if (from_obj == to->obj)
+	if (from == to)
 		return 0;
 
 	ret = i915_gem_object_pin(to->obj, CONTEXT_ALIGN, false, false);
@@ -382,7 +382,7 @@ static int do_switch(struct i915_hw_context *to)
 
 	if (!to->is_initialized || is_default_context(to))
 		hw_flags |= MI_RESTORE_INHIBIT;
-	else if (WARN_ON_ONCE(from_obj == to->obj)) /* not yet expected */
+	else if (WARN_ON_ONCE(from == to)) /* not yet expected */
 		hw_flags |= MI_FORCE_RESTORE;
 
 	ret = mi_set_context(ring, to, hw_flags);
@@ -397,9 +397,9 @@ static int do_switch(struct i915_hw_context *to)
 	 * is a bit suboptimal because the retiring can occur simply after the
 	 * MI_SET_CONTEXT instead of when the next seqno has completed.
 	 */
-	if (from_obj != NULL) {
-		from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
-		i915_gem_object_move_to_active(from_obj, ring);
+	if (from != NULL) {
+		from->obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
+		i915_gem_object_move_to_active(from->obj, ring);
 		/* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
 		 * whole damn pipeline, we don't need to explicitly mark the
 		 * object dirty. The only exception is that the context must be
@@ -407,15 +407,26 @@ static int do_switch(struct i915_hw_context *to)
 		 * able to defer doing this until we know the object would be
 		 * swapped, but there is no way to do that yet.
 		 */
-		from_obj->dirty = 1;
-		BUG_ON(from_obj->ring != ring);
-		i915_gem_object_unpin(from_obj);
+		from->obj->dirty = 1;
+		BUG_ON(from->obj->ring != ring);
+
+		ret = i915_add_request(ring, NULL, NULL);
+		if (ret) {
+			/* Too late, we've already scheduled a context switch.
+			 * Try to undo the change so that the hw state is
+			 * consistent with out tracking. In case of emergency,
+			 * scream.
+			 */
+			WARN_ON(mi_set_context(ring, from, MI_RESTORE_INHIBIT));
+			return ret;
+		}
 
-		drm_gem_object_unreference(&from_obj->base);
+		i915_gem_object_unpin(from->obj);
+		i915_gem_context_unreference(from);
 	}
 
-	drm_gem_object_reference(&to->obj->base);
-	ring->last_context_obj = to->obj;
+	i915_gem_context_reference(to);
+	ring->last_context = to;
 	to->is_initialized = true;
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index d66208c..dac1614 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -135,7 +135,7 @@ struct  intel_ring_buffer {
 	 */
 	bool itlb_before_ctx_switch;
 	struct i915_hw_context *default_context;
-	struct drm_i915_gem_object *last_context_obj;
+	struct i915_hw_context *last_context;
 
 	void *private;
 };
-- 
1.7.9.5

[PATCH 2/2] drm/i915: add context into request struct

[Mika Kuoppala]

Storing context reference into request struct
allows us to inspect context and its associated
objects when requests are retired.

Both ppgtt and arb robustness work will need
this.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |    3 +++
 drivers/gpu/drm/i915/i915_gem.c |   24 ++++++++++++++++++------
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3ac71db..ca0b0ce 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1270,6 +1270,9 @@ struct drm_i915_gem_request {
 	/** Postion in the ringbuffer of the end of the request */
 	u32 tail;
 
+	/** Context related to this request */
+	struct i915_hw_context *ctx;
+
 	/** Time at which this request was emitted, in jiffies. */
 	unsigned long emitted_jiffies;
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6be940e..8a81d1a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2042,6 +2042,11 @@ i915_add_request(struct intel_ring_buffer *ring,
 	request->seqno = intel_ring_get_seqno(ring);
 	request->ring = ring;
 	request->tail = request_ring_position;
+	request->ctx = ring->last_context;
+
+	if (request->ctx)
+		i915_gem_context_reference(request->ctx);
+
 	request->emitted_jiffies = jiffies;
 	was_empty = list_empty(&ring->request_list);
 	list_add_tail(&request->list, &ring->request_list);
@@ -2094,6 +2099,17 @@ i915_gem_request_remove_from_client(struct drm_i915_gem_request *request)
 	spin_unlock(&file_priv->mm.lock);
 }
 
+static void i915_gem_free_request(struct drm_i915_gem_request *request)
+{
+	list_del(&request->list);
+	i915_gem_request_remove_from_client(request);
+
+	if (request->ctx)
+		i915_gem_context_unreference(request->ctx);
+
+	kfree(request);
+}
+
 static void i915_gem_reset_ring_lists(struct drm_i915_private *dev_priv,
 				      struct intel_ring_buffer *ring)
 {
@@ -2104,9 +2120,7 @@ static void i915_gem_reset_ring_lists(struct drm_i915_private *dev_priv,
 					   struct drm_i915_gem_request,
 					   list);
 
-		list_del(&request->list);
-		i915_gem_request_remove_from_client(request);
-		kfree(request);
+		i915_gem_free_request(request);
 	}
 
 	while (!list_empty(&ring->active_list)) {
@@ -2198,9 +2212,7 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
 		 */
 		ring->last_retired_head = request->tail;
 
-		list_del(&request->list);
-		i915_gem_request_remove_from_client(request);
-		kfree(request);
+		i915_gem_free_request(request);
 	}
 
 	/* Move any buffers on the active list that are no longer referenced
-- 
1.7.9.5

Re: [PATCH 2/2] drm/i915: add context into request struct

[Ben Widawsky]

On Thu, May 02, 2013 at 04:48:08PM +0300, Mika Kuoppala wrote:
> Storing context reference into request struct
> allows us to inspect context and its associated
> objects when requests are retired.
> 
> Both ppgtt and arb robustness work will need
> this.
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

Both 1&2 are:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

You should add your sob to 1, since you modified it (slightly), and
maybe run them both my Chris to make sure he approves as well.

Thanks a lot of reworking the series to this order :D

[snip]

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 2/2] drm/i915: add context into request struct

[Daniel Vetter]

On Fri, May 03, 2013 at 05:07:42PM -0700, Ben Widawsky wrote:
> On Thu, May 02, 2013 at 04:48:08PM +0300, Mika Kuoppala wrote:
> > Storing context reference into request struct
> > allows us to inspect context and its associated
> > objects when requests are retired.
> > 
> > Both ppgtt and arb robustness work will need
> > this.
> > 
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> 
> Both 1&2 are:
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> You should add your sob to 1, since you modified it (slightly), and
> maybe run them both my Chris to make sure he approves as well.
> 
> Thanks a lot of reworking the series to this order :D

Both patches merged to dinq, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915: add context into request struct

[Mika Kuoppala]

Ben Widawsky <ben@bwidawsk.net> writes:

> On Thu, May 02, 2013 at 04:48:08PM +0300, Mika Kuoppala wrote:
>> Storing context reference into request struct
>> allows us to inspect context and its associated
>> objects when requests are retired.
>> 
>> Both ppgtt and arb robustness work will need
>> this.
>> 
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>
> Both 1&2 are:
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
>
> You should add your sob to 1, since you modified it (slightly), and
> maybe run them both my Chris to make sure he approves as well.

I took 1/2 from:
http://cgit.freedesktop.org/~bwidawsk/drm-intel/commit/?h=ppgtt-ctx&id=5e266650d53d42ebbc8c22f2846c8ed87d747b21

and i don't remember intentionally modifying it. Is there some merge fallout I
missed? I couldn't spot any diff to original.

-Mika

Re: [PATCH 2/2] drm/i915: add context into request struct

[Ben Widawsky]

On Mon, May 06, 2013 at 12:28:18PM +0300, Mika Kuoppala wrote:
> Ben Widawsky <ben@bwidawsk.net> writes:
> 
> > On Thu, May 02, 2013 at 04:48:08PM +0300, Mika Kuoppala wrote:
> >> Storing context reference into request struct
> >> allows us to inspect context and its associated
> >> objects when requests are retired.
> >> 
> >> Both ppgtt and arb robustness work will need
> >> this.
> >> 
> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >
> > Both 1&2 are:
> > Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> >
> > You should add your sob to 1, since you modified it (slightly), and
> > maybe run them both my Chris to make sure he approves as well.
> 
> I took 1/2 from:
> http://cgit.freedesktop.org/~bwidawsk/drm-intel/commit/?h=ppgtt-ctx&id=5e266650d53d42ebbc8c22f2846c8ed87d747b21
> 
> and i don't remember intentionally modifying it. Is there some merge fallout I
> missed? I couldn't spot any diff to original.
> 
> -Mika

Ah, I see. The original patch from Chris had all 4 args to add request.
My patch had already fixed it :-D

-- 
Ben Widawsky, Intel Open Source Technology Center