[3/3] drm/i915: Cleanup context switching through do_switch()
diff mbox

Message ID 1342352064-6749-3-git-send-email-chris@chris-wilson.co.uk
State New, archived
Headers show

Commit Message

Chris Wilson July 15, 2012, 11:34 a.m. UTC
When bug hunting, I found the interface to do_switch() overly
complicated and I believe festered the earlier bug. This aims to make
the code a little clearer.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c |   57 ++++++++++++++-----------------
 1 file changed, 26 insertions(+), 31 deletions(-)

Comments

Ben Widawsky July 16, 2012, 5:15 p.m. UTC | #1
On Sun, 15 Jul 2012 12:34:24 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> When bug hunting, I found the interface to do_switch() overly
> complicated and I believe festered the earlier bug. This aims to make
> the code a little clearer.

Would you be willing to split this up into 2 patches? One which
reorganizes create_default, and one which changes do_switch?

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c |   57 ++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 0cfc9d2..90a9c9e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -97,8 +97,7 @@
>  
>  static struct i915_hw_context *
>  i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
> -static int do_switch(struct drm_i915_gem_object *from_obj,
> -		     struct i915_hw_context *to, u32 seqno);
> +static int do_switch(struct i915_hw_context *to);
>  
>  static int get_context_size(struct drm_device *dev)
>  {
> @@ -220,19 +219,20 @@ static int create_default_context(struct drm_i915_private *dev_priv)
>  	 */
>  	dev_priv->ring[RCS].default_context = ctx;
>  	ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
> -	if (ret) {
> -		do_destroy(ctx);
> -		return ret;
> -	}
> +	if (ret)
> +		goto err_destroy;
>  
> -	ret = do_switch(NULL, ctx, 0);
> -	if (ret) {
> -		i915_gem_object_unpin(ctx->obj);
> -		do_destroy(ctx);
> -	} else {
> -		DRM_DEBUG_DRIVER("Default HW context loaded\n");
> -	}
> +	ret = do_switch(ctx);
> +	if (ret)
> +		goto err_unpin;
>  
> +	DRM_DEBUG_DRIVER("Default HW context loaded\n");
> +	return 0;
> +
> +err_unpin:
> +	i915_gem_object_unpin(ctx->obj);
> +err_destroy:
> +	do_destroy(ctx);
>  	return ret;
>  }
>  
> @@ -359,17 +359,18 @@ mi_set_context(struct intel_ring_buffer *ring,
>  	return ret;
>  }
>  
> -static int do_switch(struct drm_i915_gem_object *from_obj,
> -		     struct i915_hw_context *to,
> -		     u32 seqno)
> +static int do_switch(struct i915_hw_context *to)
>  {
> -	struct intel_ring_buffer *ring = NULL;
> +	struct intel_ring_buffer *ring = to->ring;
> +	struct drm_i915_gem_object *from_obj = ring->last_context_obj;
>  	u32 hw_flags = 0;
>  	int ret;
>  
> -	BUG_ON(to == NULL);
>  	BUG_ON(from_obj != NULL && from_obj->pin_count == 0);
>  
> +	if (from_obj == to->obj)
> +		return 0;
> +
>  	ret = i915_gem_object_pin(to->obj, CONTEXT_ALIGN, false);
>  	if (ret)
>  		return ret;
> @@ -389,7 +390,6 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
>  	else if (WARN_ON_ONCE(from_obj == to->obj)) /* not yet expected */
>  		hw_flags |= MI_FORCE_RESTORE;
>  
> -	ring = to->ring;
>  	ret = mi_set_context(ring, to, hw_flags);
>  	if (ret) {
>  		i915_gem_object_unpin(to->obj);
> @@ -403,6 +403,7 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
>  	 * MI_SET_CONTEXT instead of when the next seqno has completed.
>  	 */
>  	if (from_obj != NULL) {
> +		u32 seqno = i915_gem_next_request_seqno(ring);
>  		from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
>  		i915_gem_object_move_to_active(from_obj, ring, seqno);
>  		/* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
> @@ -413,7 +414,7 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
>  		 * swapped, but there is no way to do that yet.
>  		 */
>  		from_obj->dirty = 1;
> -		BUG_ON(from_obj->ring != to->ring);
> +		BUG_ON(from_obj->ring != ring);
>  		i915_gem_object_unpin(from_obj);
>  
>  		drm_gem_object_unreference(&from_obj->base);
> @@ -444,10 +445,7 @@ int i915_switch_context(struct intel_ring_buffer *ring,
>  			int to_id)
>  {
>  	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> -	struct drm_i915_file_private *file_priv = NULL;
>  	struct i915_hw_context *to;
> -	struct drm_i915_gem_object *from_obj = ring->last_context_obj;
> -	int ret;
>  
>  	if (dev_priv->hw_contexts_disabled)
>  		return 0;
> @@ -455,21 +453,18 @@ int i915_switch_context(struct intel_ring_buffer *ring,
>  	if (ring != &dev_priv->ring[RCS])
>  		return 0;
>  
> -	if (file)
> -		file_priv = file->driver_priv;
> -
>  	if (to_id == DEFAULT_CONTEXT_ID) {
>  		to = ring->default_context;
>  	} else {
> -		to = i915_gem_context_get(file_priv, to_id);
> +		if (file == NULL)
> +			return -EINVAL;
> +
> +		to = i915_gem_context_get(file->driver_priv, to_id);
>  		if (to == NULL)
>  			return -ENOENT;
>  	}
>  
> -	if (from_obj == to->obj)
> -		return 0;
> -
> -	return do_switch(from_obj, to, i915_gem_next_request_seqno(to->ring));
> +	return do_switch(to);
>  }
>  
>  int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
Daniel Vetter July 16, 2012, 6:27 p.m. UTC | #2
On Sun, Jul 15, 2012 at 12:34:24PM +0100, Chris Wilson wrote:
> When bug hunting, I found the interface to do_switch() overly
> complicated and I believe festered the earlier bug. This aims to make
> the code a little clearer.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>

I've picked up patches 2&3 with Ben's irc r-b on patch 3, thanks.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 0cfc9d2..90a9c9e 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -97,8 +97,7 @@ 
 
 static struct i915_hw_context *
 i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
-static int do_switch(struct drm_i915_gem_object *from_obj,
-		     struct i915_hw_context *to, u32 seqno);
+static int do_switch(struct i915_hw_context *to);
 
 static int get_context_size(struct drm_device *dev)
 {
@@ -220,19 +219,20 @@  static int create_default_context(struct drm_i915_private *dev_priv)
 	 */
 	dev_priv->ring[RCS].default_context = ctx;
 	ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
-	if (ret) {
-		do_destroy(ctx);
-		return ret;
-	}
+	if (ret)
+		goto err_destroy;
 
-	ret = do_switch(NULL, ctx, 0);
-	if (ret) {
-		i915_gem_object_unpin(ctx->obj);
-		do_destroy(ctx);
-	} else {
-		DRM_DEBUG_DRIVER("Default HW context loaded\n");
-	}
+	ret = do_switch(ctx);
+	if (ret)
+		goto err_unpin;
 
+	DRM_DEBUG_DRIVER("Default HW context loaded\n");
+	return 0;
+
+err_unpin:
+	i915_gem_object_unpin(ctx->obj);
+err_destroy:
+	do_destroy(ctx);
 	return ret;
 }
 
@@ -359,17 +359,18 @@  mi_set_context(struct intel_ring_buffer *ring,
 	return ret;
 }
 
-static int do_switch(struct drm_i915_gem_object *from_obj,
-		     struct i915_hw_context *to,
-		     u32 seqno)
+static int do_switch(struct i915_hw_context *to)
 {
-	struct intel_ring_buffer *ring = NULL;
+	struct intel_ring_buffer *ring = to->ring;
+	struct drm_i915_gem_object *from_obj = ring->last_context_obj;
 	u32 hw_flags = 0;
 	int ret;
 
-	BUG_ON(to == NULL);
 	BUG_ON(from_obj != NULL && from_obj->pin_count == 0);
 
+	if (from_obj == to->obj)
+		return 0;
+
 	ret = i915_gem_object_pin(to->obj, CONTEXT_ALIGN, false);
 	if (ret)
 		return ret;
@@ -389,7 +390,6 @@  static int do_switch(struct drm_i915_gem_object *from_obj,
 	else if (WARN_ON_ONCE(from_obj == to->obj)) /* not yet expected */
 		hw_flags |= MI_FORCE_RESTORE;
 
-	ring = to->ring;
 	ret = mi_set_context(ring, to, hw_flags);
 	if (ret) {
 		i915_gem_object_unpin(to->obj);
@@ -403,6 +403,7 @@  static int do_switch(struct drm_i915_gem_object *from_obj,
 	 * MI_SET_CONTEXT instead of when the next seqno has completed.
 	 */
 	if (from_obj != NULL) {
+		u32 seqno = i915_gem_next_request_seqno(ring);
 		from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
 		i915_gem_object_move_to_active(from_obj, ring, seqno);
 		/* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
@@ -413,7 +414,7 @@  static int do_switch(struct drm_i915_gem_object *from_obj,
 		 * swapped, but there is no way to do that yet.
 		 */
 		from_obj->dirty = 1;
-		BUG_ON(from_obj->ring != to->ring);
+		BUG_ON(from_obj->ring != ring);
 		i915_gem_object_unpin(from_obj);
 
 		drm_gem_object_unreference(&from_obj->base);
@@ -444,10 +445,7 @@  int i915_switch_context(struct intel_ring_buffer *ring,
 			int to_id)
 {
 	struct drm_i915_private *dev_priv = ring->dev->dev_private;
-	struct drm_i915_file_private *file_priv = NULL;
 	struct i915_hw_context *to;
-	struct drm_i915_gem_object *from_obj = ring->last_context_obj;
-	int ret;
 
 	if (dev_priv->hw_contexts_disabled)
 		return 0;
@@ -455,21 +453,18 @@  int i915_switch_context(struct intel_ring_buffer *ring,
 	if (ring != &dev_priv->ring[RCS])
 		return 0;
 
-	if (file)
-		file_priv = file->driver_priv;
-
 	if (to_id == DEFAULT_CONTEXT_ID) {
 		to = ring->default_context;
 	} else {
-		to = i915_gem_context_get(file_priv, to_id);
+		if (file == NULL)
+			return -EINVAL;
+
+		to = i915_gem_context_get(file->driver_priv, to_id);
 		if (to == NULL)
 			return -ENOENT;
 	}
 
-	if (from_obj == to->obj)
-		return 0;
-
-	return do_switch(from_obj, to, i915_gem_next_request_seqno(to->ring));
+	return do_switch(to);
 }
 
 int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,