[17/48] drm/i915: Track which ring a context ran on

Commit Message

Ben Widawsky Dec. 6, 2013, 10:11 p.m. UTC

From: Ben Widawsky <[e-mail]>

Previously we dropped the association of a context to a ring. It is
however very important to know which ring a context ran on (we could
have reused the other member, but I was nitpicky).

This is very important when we switch address spaces, which unlike
context objects, do change per ring.

As an example, if we have:

        RCS   BCS
ctx            A
ctx      A
ctx      B
ctx            B

Without tracking the last ring B ran on, we wouldn't know to switch the
address space on BCS in the last row.

As a result, we no longer need to track which ring a context "belongs"
to, as it never really made much sense anyway.

Signed-off-by: Ben Widawsky <[e-mail]>
---
 drivers/gpu/drm/i915/i915_drv.h         |  2 +-
 drivers/gpu/drm/i915/i915_gem_context.c | 12 +++++-------
 2 files changed, 6 insertions(+), 8 deletions(-)

Comments

Chris Wilson April 18, 2014, 9:51 a.m. UTC | #1

On Fri, Dec 06, 2013 at 02:11:02PM -0800, Ben Widawsky wrote:
> From: Ben Widawsky <[e-mail]>
> 
> Previously we dropped the association of a context to a ring. It is
> however very important to know which ring a context ran on (we could
> have reused the other member, but I was nitpicky).
> 
> This is very important when we switch address spaces, which unlike
> context objects, do change per ring.
> 
> As an example, if we have:
> 
>         RCS   BCS
> ctx            A
> ctx      A
> ctx      B
> ctx            B
> 
> Without tracking the last ring B ran on, we wouldn't know to switch the
> address space on BCS in the last row.
> 
> As a result, we no longer need to track which ring a context "belongs"
> to, as it never really made much sense anyway.

What about ring->last_context != to? That would force the update on BCS
from A to B.
-Chris

Daniel Vetter April 22, 2014, 2:25 p.m. UTC | #2

On Fri, Apr 18, 2014 at 10:51:46AM +0100, Chris Wilson wrote:
> On Fri, Dec 06, 2013 at 02:11:02PM -0800, Ben Widawsky wrote:
> > From: Ben Widawsky <[e-mail]>
> > 
> > Previously we dropped the association of a context to a ring. It is
> > however very important to know which ring a context ran on (we could
> > have reused the other member, but I was nitpicky).
> > 
> > This is very important when we switch address spaces, which unlike
> > context objects, do change per ring.
> > 
> > As an example, if we have:
> > 
> >         RCS   BCS
> > ctx            A
> > ctx      A
> > ctx      B
> > ctx            B
> > 
> > Without tracking the last ring B ran on, we wouldn't know to switch the
> > address space on BCS in the last row.
> > 
> > As a result, we no longer need to track which ring a context "belongs"
> > to, as it never really made much sense anyway.
> 
> What about ring->last_context != to? That would force the update on BCS
> from A to B.

Yeah ctx->last_ring isn't really what we want, I'll add another JIRA for
switching all the ctx->last_ring checks to instead check
ring->last_context != to. Since that's what matters

I think this doesn't have the possibility to have broken anything yet
since we don't allow the same context on multiple rings. Except the
default one, but mesa creates new contexts anyway afaik. Or am I wrong?

It's a definite blocker for execlists and full ppgtt though.
-Daniel

Chris Wilson April 22, 2014, 2:54 p.m. UTC | #3

On Tue, Apr 22, 2014 at 04:25:22PM +0200, Daniel Vetter wrote:
> I think this doesn't have the possibility to have broken anything yet
> since we don't allow the same context on multiple rings. Except the
> default one, but mesa creates new contexts anyway afaik. Or am I wrong?

It's a minor regression in that we reload the context before every
batch; a brief wtf when doing perf monitoring.
-Chris

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7cf782f..1d4651c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -695,7 +695,7 @@  struct i915_hw_context {
 	bool is_initialized;
 	uint8_t remap_slice;
 	struct drm_i915_file_private *file_priv;
-	struct intel_ring_buffer *ring;
+	struct intel_ring_buffer *last_ring;
 	struct drm_i915_gem_object *obj;
 	struct i915_ctx_hang_stats hang_stats;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index f5d6035..0c2ff5a 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -176,11 +176,6 @@  create_hw_context(struct drm_device *dev,
 			goto err_out;
 	}
 
-	/* The ring associated with the context object is handled by the normal
-	 * object tracking code. We give an initial ring value simple to pass an
-	 * assertion in the context switch code.
-	 */
-	ctx->ring = &dev_priv->ring[RCS];
 	list_add_tail(&ctx->link, &dev_priv->context_list);
 
 	/* Default context will never have a file_priv */
@@ -208,7 +203,8 @@  err_out:
 
 static inline bool is_default_context(struct i915_hw_context *ctx)
 {
-	return (ctx == ctx->ring->default_context);
+	/* Cheap trick to determine default contexts */
+	return ctx->file_priv ? false : true;
 }
 
 /**
@@ -338,6 +334,7 @@  void i915_gem_context_fini(struct drm_device *dev)
 			i915_gem_context_unreference(ring->last_context);
 
 		ring->default_context = NULL;
+		ring->last_context = NULL;
 	}
 
 	i915_gem_object_ggtt_unpin(dctx->obj);
@@ -465,7 +462,7 @@  static int do_switch(struct intel_ring_buffer *ring,
 		BUG_ON(!i915_gem_obj_is_pinned(from->obj));
 	}
 
-	if (from == to && !to->remap_slice)
+	if (from == to && from->last_ring == ring && !to->remap_slice)
 		return 0;
 
 	if (ring != &dev_priv->ring[RCS]) {
@@ -555,6 +552,7 @@  done:
 	i915_gem_context_reference(to);
 	ring->last_context = to;
 	to->is_initialized = true;
+	to->last_ring = ring;
 
 	return 0;
 }