[Intel-gfx] [PATCH 3/6] drm/i915: Unify execlist and legacy request life-cycles

[Chris Wilson]

On Tue, Oct 20, 2015 at 10:23:53AM +0100, Nick Hoath wrote:
> There is a desire to simplify the i915 driver by reducing the number of
> different code paths introduced by the LRC / execlists support.  As the
> execlists request is now part of the gem request it is possible and
> desirable to unify the request life-cycles for execlist and legacy
> requests.
> 
> A request is considered retireable if its seqno passed (i.e. the request
> has completed) and either it was never submitted to the ELSP or its
> context completed.  This ensures that context save is carried out before
> the last request for a context is considered retireable. request_complete()
> now checks the elsp_submitted count when deciding if a request is complete.
> Requests that were not waiting for a context
> switch interrupt (either as a result of being merged into a following
> request or by being a legacy request) will be considered retireable as
> soon as their seqno has passed.
> 
> Removed the extra request reference held for the execlist request.
> 
> Removed intel_execlists_retire_requests() and all references to
> intel_engine_cs.execlist_retired_req_list.
> 
> Changed gen8_cs_irq_handler() so that notify_ring() is called when
> contexts complete as well as when a user interrupt occurs so that
> notification happens when a request is complete and context save has
> finished.
> 
> v2: Rebase over the read-read optimisation changes
> 
> v3: Reworked IRQ handler after removing IRQ handler cleanup patch
> 
> v4: Fixed various pin leaks
> 
> v5: Removed ctx_complete flag & associated changes. Removed extraneous
> 	request pin of context.
> 	(Chris Wilson/Daniel Vetter)
> 
> Issue: VIZ-4277
> Signed-off-by: Thomas Daniel <thomas.daniel at intel.com>
> Signed-off-by: Nick Hoath <nicholas.hoath at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  2 +-
>  drivers/gpu/drm/i915/i915_gem.c         | 23 ++++++++---------
>  drivers/gpu/drm/i915/i915_irq.c         |  7 ++---
>  drivers/gpu/drm/i915/intel_lrc.c        | 45 ++++-----------------------------
>  drivers/gpu/drm/i915/intel_lrc.h        |  2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  1 -
>  6 files changed, 21 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8afda45..ae08e57 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2914,7 +2914,7 @@ static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req,
>  
>  	seqno = req->ring->get_seqno(req->ring, lazy_coherency);
>  
> -	return i915_seqno_passed(seqno, req->seqno);
> +	return i915_seqno_passed(seqno, req->seqno) && !req->elsp_submitted;
>  }
>  
>  int __must_check i915_gem_get_seqno(struct drm_device *dev, u32 *seqno);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e57061a..290a1ac 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2848,12 +2848,16 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
>  
>  		if (!list_empty(&obj->last_read_req[ring->id]->list))
>  			break;
> +		if (!i915_gem_request_completed(obj->last_read_req[ring->id],
> +				true))
> +			break;

What???? No. request_completed is and should only ever be has the seqno
passed.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre