[Intel-gfx] [PATCH 01/17] drm/i915: Do not add an interrupt for a context switch

[Damien Lespiau]

On Mon, Aug 26, 2013 at 07:50:53PM -0300, Rodrigo Vivi wrote:
> From: Chris Wilson <chris at chris-wilson.co.uk>
> 
> We use the request to ensure we hold a reference to the context for the
> duration that it remains in use by the ring. Each request only holds a
> reference to the current context, hence we emit a request after
> switching contexts with the final reference to the old context. However,
> the extra interrupt caused by that request is not useful (no timing
> critical function will wait for the context object), instead the overhead
> of servicing the IRQ shows up in some (lightweight) benchmarks. In order
> to keep the useful property of using the request to manage the context
> lifetime, we want to add a dummy request that is associated with the
> interrupt from the subsequent real request following the batch.
> 
> The extra interrupt was added as a side-effect of using
> i915_add_request() in
> 
> commit 112522f6789581824903f6f72082b5b841a7f0f9
> Author: Chris Wilson <chris at chris-wilson.co.uk>
> Date:   Thu May 2 16:48:07 2013 +0300
> 
>     drm/i915: put context upon switching
> 
> v2: Daniel convinced me that the request here was solely for context
> lifetime tracking and that we have the active ref to keep the object
> alive whilst the MI_SET_CONTEXT. So the only concern then is which
> context should get the blame for MI_SET_CONTEXT failing. The old scheme
> added a request for the old context so that any hang upto and including
> the switch away would mark the old context as guilty. Now any hang here
> implicates the new context. However since we have already gone through a
> complete flush with the last context in its last request, and all that
> lies in no-man's-land is an invalidate flush and the MI_SET_CONTEXT, we
> should be safe in not unduly placing blame on the new context.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Ben Widawsky <ben at bwidawsk.net>
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>

I'm somewhat new to the core GEM code, but I'm convinced by the commit
message and the patch does what it says. So:

Reviewed-by: Damien Lespiau <damien.lespiau at intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/i915_gem.c         |  1 -
>  drivers/gpu/drm/i915/i915_gem_context.c | 12 +-----------
>  2 files changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 2d1cb10..56c9104 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2046,7 +2046,6 @@ int __i915_add_request(struct intel_ring_buffer *ring,
>  	if (request == NULL)
>  		return -ENOMEM;
>  
> -
>  	/* Record the position of the start of the request so that
>  	 * should we detect the updated seqno part-way through the
>  	 * GPU processing the request, we never over-estimate the
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 403309c..b6da70b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -451,17 +451,7 @@ static int do_switch(struct i915_hw_context *to)
>  		from->obj->dirty = 1;
>  		BUG_ON(from->obj->ring != ring);
>  
> -		ret = i915_add_request(ring, NULL);
> -		if (ret) {
> -			/* Too late, we've already scheduled a context switch.
> -			 * Try to undo the change so that the hw state is
> -			 * consistent with out tracking. In case of emergency,
> -			 * scream.
> -			 */
> -			WARN_ON(mi_set_context(ring, from, MI_RESTORE_INHIBIT));
> -			return ret;
> -		}
> -
> +		/* obj is kept alive until the next request by its active ref */
>  		i915_gem_object_unpin(from->obj);
>  		i915_gem_context_unreference(from);
>  	}
> -- 
> 1.8.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx