[PATCH 1/3] drm/i915: Flush the context object from the CPU caches upon switching

[PATCH 1/3] drm/i915: Flush the context object from the CPU caches upon switching

[Chris Wilson]

The issue is that we stale data in the CPU caches, when we come to
swap-out the object, the CPU may short-circuit the reads from those
cacheline and so corrupt the context object.

Secondary, leaving the context object as being marked in the CPU write
domain whilst on the GPU active list is a bad idea and will throw
warnings later.

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

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 9ae3f2c..fd978bb 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -374,6 +374,13 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
 	if (ret)
 		return ret;
 
+	/* Clear this page out of any CPU caches for coherent swap-in/out */
+	ret = i915_gem_object_set_to_gtt_domain(to->obj, false);
+	if (ret) {
+		i915_gem_object_unpin(to->obj);
+		return ret;
+	}
+
 	if (!to->obj->has_global_gtt_mapping)
 		i915_gem_gtt_bind_object(to->obj, to->obj->cache_level);
 
-- 
1.7.10.4

[PATCH 2/3] drm/i915: fix invalid reference handling of the default ctx obj

[Chris Wilson]

Otherwise we end up trying to unpin a freed object and BUG.

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

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index fd978bb..0cfc9d2 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -415,8 +415,11 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
 		from_obj->dirty = 1;
 		BUG_ON(from_obj->ring != to->ring);
 		i915_gem_object_unpin(from_obj);
+
+		drm_gem_object_unreference(&from_obj->base);
 	}
 
+	drm_gem_object_reference(&to->obj->base);
 	ring->last_context_obj = to->obj;
 	to->is_initialized = true;
 
@@ -466,20 +469,7 @@ int i915_switch_context(struct intel_ring_buffer *ring,
 	if (from_obj == to->obj)
 		return 0;
 
-	ret = do_switch(from_obj, to, i915_gem_next_request_seqno(to->ring));
-	if (ret)
-		return ret;
-
-	/* Just to make the code a little cleaner we take the object reference
-	 * after the switch was successful. It would be more intuitive to ref
-	 * the 'to' object before the switch but we know the refcount must be >0
-	 * if context_get() succeeded, and we hold struct mutex. So it's safe to
-	 * do this here/now
-	 */
-	drm_gem_object_reference(&to->obj->base);
-	if (from_obj != NULL)
-		drm_gem_object_unreference(&from_obj->base);
-	return ret;
+	return do_switch(from_obj, to, i915_gem_next_request_seqno(to->ring));
 }
 
 int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
-- 
1.7.10.4

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

[Chris Wilson]

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(-)

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,
-- 
1.7.10.4

Re: [PATCH 3/3] drm/i915: Cleanup context switching through do_switch()

[Ben Widawsky]

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,



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 3/3] drm/i915: Cleanup context switching through do_switch()

[Daniel Vetter]

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
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/3] drm/i915: Flush the context object from the CPU caches upon switching

[Daniel Vetter]

On Sun, Jul 15, 2012 at 12:34:22PM +0100, Chris Wilson wrote:
> The issue is that we stale data in the CPU caches, when we come to
> swap-out the object, the CPU may short-circuit the reads from those
> cacheline and so corrupt the context object.
> 
> Secondary, leaving the context object as being marked in the CPU write
> domain whilst on the GPU active list is a bad idea and will throw
> warnings later.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 9ae3f2c..fd978bb 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -374,6 +374,13 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
>  	if (ret)
>  		return ret;
>  
> +	/* Clear this page out of any CPU caches for coherent swap-in/out */
> +	ret = i915_gem_object_set_to_gtt_domain(to->obj, false);
> +	if (ret) {
> +		i915_gem_object_unpin(to->obj);
> +		return ret;
> +	}
> +

Do I understand things correctly that thanks to clever use of
set_to_gtt_domain with write = false and not setting a write_domain when
moving the old context object to the active list we won't block? If so,
I'll add a short note to that effect to the commit message when merging.

And given that we have similar clever interface abuse in the pwrite/pread
code, some code rework is in order I guess. We could replace all that
domain tracking with bool gpu_coherent would be simpler (after the
flushing_list's permanent demise, of course).
-Daniel

>  	if (!to->obj->has_global_gtt_mapping)
>  		i915_gem_gtt_bind_object(to->obj, to->obj->cache_level);
>  
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/3] drm/i915: Flush the context object from the CPU caches upon switching

[Chris Wilson]

On Sun, 15 Jul 2012 17:16:34 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sun, Jul 15, 2012 at 12:34:22PM +0100, Chris Wilson wrote:
> > The issue is that we stale data in the CPU caches, when we come to
> > swap-out the object, the CPU may short-circuit the reads from those
> > cacheline and so corrupt the context object.
> > 
> > Secondary, leaving the context object as being marked in the CPU write
> > domain whilst on the GPU active list is a bad idea and will throw
> > warnings later.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c |    7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 9ae3f2c..fd978bb 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -374,6 +374,13 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
> >  	if (ret)
> >  		return ret;
> >  
> > +	/* Clear this page out of any CPU caches for coherent swap-in/out */
> > +	ret = i915_gem_object_set_to_gtt_domain(to->obj, false);
> > +	if (ret) {
> > +		i915_gem_object_unpin(to->obj);
> > +		return ret;
> > +	}
> > +
> 
> Do I understand things correctly that thanks to clever use of
> set_to_gtt_domain with write = false and not setting a write_domain when
> moving the old context object to the active list we won't block? If so,
> I'll add a short note to that effect to the commit message when merging.

Worse, it was accidentally very clever. :(

Definitely needs a comment and in the future a new function to dtrt
intentionally!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/3] drm/i915: Flush the context object from the CPU caches upon switching

[Daniel Vetter]

On Sun, Jul 15, 2012 at 08:09:36PM +0100, Chris Wilson wrote:
> On Sun, 15 Jul 2012 17:16:34 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Sun, Jul 15, 2012 at 12:34:22PM +0100, Chris Wilson wrote:
> > > The issue is that we stale data in the CPU caches, when we come to
> > > swap-out the object, the CPU may short-circuit the reads from those
> > > cacheline and so corrupt the context object.
> > > 
> > > Secondary, leaving the context object as being marked in the CPU write
> > > domain whilst on the GPU active list is a bad idea and will throw
> > > warnings later.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_context.c |    7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > > index 9ae3f2c..fd978bb 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > > @@ -374,6 +374,13 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	/* Clear this page out of any CPU caches for coherent swap-in/out */
> > > +	ret = i915_gem_object_set_to_gtt_domain(to->obj, false);
> > > +	if (ret) {
> > > +		i915_gem_object_unpin(to->obj);
> > > +		return ret;
> > > +	}
> > > +
> > 
> > Do I understand things correctly that thanks to clever use of
> > set_to_gtt_domain with write = false and not setting a write_domain when
> > moving the old context object to the active list we won't block? If so,
> > I'll add a short note to that effect to the commit message when merging.
> 
> Worse, it was accidentally very clever. :(
> 
> Definitely needs a comment and in the future a new function to dtrt
> intentionally!
Queued for -next (with comments to explain this trickery added as
discussed on irc), thanks for the patch. I'll pick up the other two
patches once Ben has smashed an r-b onto them.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48