[01/13] drm/i915: Flush the context object from the CPU caches upon creation
diff mbox

Message ID 1342185256-16024-2-git-send-email-chris@chris-wilson.co.uk
State New, archived
Headers show

Commit Message

Chris Wilson July 13, 2012, 1:14 p.m. UTC
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Ben Widawsky July 13, 2012, 3:28 p.m. UTC | #1
On Fri, 13 Jul 2012 14:14:04 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>

I guess setting the read_domains is now superfluous.
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> ---
>  drivers/gpu/drm/i915/i915_gem_context.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 9ae3f2c..90857f8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -225,6 +225,13 @@ static int create_default_context(struct drm_i915_private *dev_priv)
>  		return ret;
>  	}
>  
> +	ret = i915_gem_object_set_to_gtt_domain(ctx->obj, true);
> +	if (ret) {
> +		i915_gem_object_unpin(ctx->obj);
> +		do_destroy(ctx);
> +		return ret;
> +	}
> +
>  	ret = do_switch(NULL, ctx, 0);
>  	if (ret) {
>  		i915_gem_object_unpin(ctx->obj);
> @@ -396,8 +403,6 @@ 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) {
> -		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
>  		 * whole damn pipeline, we don't need to explicitly mark the
>  		 * object dirty. The only exception is that the context must be
> @@ -405,6 +410,9 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
>  		 * able to defer doing this until we know the object would be
>  		 * swapped, but there is no way to do that yet.
>  		 */
> +		from_obj->base.write_domain = I915_GEM_DOMAIN_INSTRUCTION;
> +		from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> +		i915_gem_object_move_to_active(from_obj, ring, seqno);
>  		from_obj->dirty = 1;
>  		BUG_ON(from_obj->ring != to->ring);
>  		i915_gem_object_unpin(from_obj);
Daniel Vetter July 13, 2012, 3:54 p.m. UTC | #2
On Fri, Jul 13, 2012 at 02:14:04PM +0100, Chris Wilson wrote:
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 9ae3f2c..90857f8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -225,6 +225,13 @@ static int create_default_context(struct drm_i915_private *dev_priv)
>  		return ret;
>  	}
>  
> +	ret = i915_gem_object_set_to_gtt_domain(ctx->obj, true);
> +	if (ret) {
> +		i915_gem_object_unpin(ctx->obj);
> +		do_destroy(ctx);
> +		return ret;
> +	}
> +
>  	ret = do_switch(NULL, ctx, 0);
>  	if (ret) {
>  		i915_gem_object_unpin(ctx->obj);
> @@ -396,8 +403,6 @@ 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) {
> -		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
>  		 * whole damn pipeline, we don't need to explicitly mark the
>  		 * object dirty. The only exception is that the context must be
> @@ -405,6 +410,9 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
>  		 * able to defer doing this until we know the object would be
>  		 * swapped, but there is no way to do that yet.
>  		 */
> +		from_obj->base.write_domain = I915_GEM_DOMAIN_INSTRUCTION;
> +		from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> +		i915_gem_object_move_to_active(from_obj, ring, seqno);
>  		from_obj->dirty = 1;
>  		BUG_ON(from_obj->ring != to->ring);
>  		i915_gem_object_unpin(from_obj);

I think only the first hunk should be part of this patch - the later two
hunks make more sense squashed together with the last patch. At least that
would avoid me going a bit wtf here and then again on the last patch where
the from_obj->dirty=1 gets removed and smashed into move_to_active. Until
I've realized what's going on here ;-)
-Daniel
Chris Wilson July 14, 2012, 9:38 a.m. UTC | #3
On Fri, 13 Jul 2012 17:54:54 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> I think only the first hunk should be part of this patch - the later two
> hunks make more sense squashed together with the last patch. At least that
> would avoid me going a bit wtf here and then again on the last patch where
> the from_obj->dirty=1 gets removed and smashed into move_to_active. Until
> I've realized what's going on here ;-)

When I've rewritten this patch to be more correct (handle the swap-in
case as well), it will make more sense the way it is in this patch.
Honestly. :)
-Chris
Daniel Vetter July 14, 2012, 11:58 a.m. UTC | #4
On Fri, Jul 13, 2012 at 02:14:04PM +0100, Chris Wilson wrote:
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 9ae3f2c..90857f8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -225,6 +225,13 @@ static int create_default_context(struct drm_i915_private *dev_priv)
>  		return ret;
>  	}
>  
> +	ret = i915_gem_object_set_to_gtt_domain(ctx->obj, true);
> +	if (ret) {
> +		i915_gem_object_unpin(ctx->obj);
> +		do_destroy(ctx);
> +		return ret;
> +	}
> +
>  	ret = do_switch(NULL, ctx, 0);
>  	if (ret) {
>  		i915_gem_object_unpin(ctx->obj);
> @@ -396,8 +403,6 @@ 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) {
> -		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
>  		 * whole damn pipeline, we don't need to explicitly mark the
>  		 * object dirty. The only exception is that the context must be
> @@ -405,6 +410,9 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
>  		 * able to defer doing this until we know the object would be
>  		 * swapped, but there is no way to do that yet.
>  		 */
> +		from_obj->base.write_domain = I915_GEM_DOMAIN_INSTRUCTION;
> +		from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;

Presuming I understand things correctly, setting write_domain to non-zero
will result in the ctx object landing on the flushing list when we retire
it from the active list. But it isn't being added to the ring's
gpu_write_list, so it won't ever get off that flushing list and eventually
result in the BUG_ON(seqno == 0) when we try to wait for it after a flush.

So afact this first patch here seems to add another instance of the very
bug this patch series tries squash ... Additionally I'm still hunting for
that other failure case, which can't be fixed by adding the flush in
execbuffer if ring->gpu_caches_dirty is set.

/me is still lost

-Daniel
Chris Wilson July 14, 2012, 12:48 p.m. UTC | #5
On Sat, 14 Jul 2012 13:58:58 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> So afact this first patch here seems to add another instance of the very
> bug this patch series tries squash ... Additionally I'm still hunting for
> that other failure case, which can't be fixed by adding the flush in
> execbuffer if ring->gpu_caches_dirty is set.
> 
> /me is still lost

Now all that you are missing is that we only flush GPU_DOMAINS when on
the gpu_write_list.
-Chris
Daniel Vetter July 14, 2012, 12:59 p.m. UTC | #6
On Sat, Jul 14, 2012 at 2:48 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Sat, 14 Jul 2012 13:58:58 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
>> So afact this first patch here seems to add another instance of the very
>> bug this patch series tries squash ... Additionally I'm still hunting for
>> that other failure case, which can't be fixed by adding the flush in
>> execbuffer if ring->gpu_caches_dirty is set.
>>
>> /me is still lost
>
> Now all that you are missing is that we only flush GPU_DOMAINS when on
> the gpu_write_list.

Hm, I guess we've seen that one before ;-) But that would require that
we add an object with a non-gpu write domain to the active list, but
both with or without this patch I don't see how that should happen ...

I guess a WARN in move_to_active won't hurt (and we should have done
that when fixing the GTT_DOMAIN relocation hole in execbuf), but
besides that I'm still as dense as ever, it seems.
-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 9ae3f2c..90857f8 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -225,6 +225,13 @@  static int create_default_context(struct drm_i915_private *dev_priv)
 		return ret;
 	}
 
+	ret = i915_gem_object_set_to_gtt_domain(ctx->obj, true);
+	if (ret) {
+		i915_gem_object_unpin(ctx->obj);
+		do_destroy(ctx);
+		return ret;
+	}
+
 	ret = do_switch(NULL, ctx, 0);
 	if (ret) {
 		i915_gem_object_unpin(ctx->obj);
@@ -396,8 +403,6 @@  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) {
-		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
 		 * whole damn pipeline, we don't need to explicitly mark the
 		 * object dirty. The only exception is that the context must be
@@ -405,6 +410,9 @@  static int do_switch(struct drm_i915_gem_object *from_obj,
 		 * able to defer doing this until we know the object would be
 		 * swapped, but there is no way to do that yet.
 		 */
+		from_obj->base.write_domain = I915_GEM_DOMAIN_INSTRUCTION;
+		from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
+		i915_gem_object_move_to_active(from_obj, ring, seqno);
 		from_obj->dirty = 1;
 		BUG_ON(from_obj->ring != to->ring);
 		i915_gem_object_unpin(from_obj);