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

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org, Ben Widawsky <ben@bwidawsk.net>
Subject: Re: [PATCH 01/13] drm/i915: Flush the context object from the CPU caches upon creation
Date: Sat, 14 Jul 2012 13:58:58 +0200	[thread overview]
Message-ID: <20120714115858.GB5498@phenom.ffwll.local> (raw)
In-Reply-To: <1342185256-16024-2-git-send-email-chris@chris-wilson.co.uk>

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