[Intel-gfx] [PATCH] drm/i915: Keep ring->active_list and ring->requests_list consistent

[Daniel Vetter]

On Wed, Mar 18, 2015 at 06:19:22PM +0000, Chris Wilson wrote:
> If we retire requests last, we may use a later seqno and so clear
> the requests lists without clearing the active list, leading to
> confusion. Hence we should retire requests first for consistency with
> the early return. The order used to be important as the lifecycle for
> the object on the active list was determined by request->seqno. However,
> the requests themselves are now reference counted removing the
> constraint from the order of retirement.
> 
> Fixes regression from
> 
> commit 1b5a433a4dd967b125131da42b89b5cc0d5b1f57
> Author: John Harrison <John.C.Harrison at Intel.com>
> Date:   Mon Nov 24 18:49:42 2014 +0000
> 
>     drm/i915: Convert 'i915_seqno_passed' calls into 'i915_gem_request_completed
> '
> 
> and a
> 
> 	WARNING: CPU: 0 PID: 1383 at drivers/gpu/drm/i915/i915_gem_evict.c:279 i915_gem_evict_vm+0x10c/0x140()
> 	WARN_ON(!list_empty(&vm->active_list))
> 
> Identified by updating WATCH_LISTS:
> 
> 	[drm:i915_verify_lists] *ERROR* blitter ring: active list not empty, but no requests
> 	WARNING: CPU: 0 PID: 681 at drivers/gpu/drm/i915/i915_gem.c:2751 i915_gem_retire_requests_ring+0x149/0x230()
> 	WARN_ON(i915_verify_lists(ring->dev))
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: John Harrison <John.C.Harrison at Intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>

In case it's burried too much in the thread:

Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>

Addadendum for the commit:

"Note that this is only a problem in evict_vm where the following happens
after a retire_request has cleaned out all requests, but not all active
bo:
- intel_ring_idle called from i915_gpu_idle notices that no requests are
  outstanding and immediately returns.
- i915_gem_retire_requests_ring called from i915_gem_retire_requests also
  immediately returns when there's no request, still leaving the bo on the
  active list.
- evict_vm hits the WARN_ON(!list_empty(&vm->active_list)) after evicting
  all active objects that there's still stuff left that shouldn't be
  there."

Chris, is that an accurate enough description for Jani to add to the
patch?
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 38 +++++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 092f25cfb8d5..7a9589f38bbc 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2660,24 +2660,11 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
>  
>  	WARN_ON(i915_verify_lists(ring->dev));
>  
> -	/* Move any buffers on the active list that are no longer referenced
> -	 * by the ringbuffer to the flushing/inactive lists as appropriate,
> -	 * before we free the context associated with the requests.
> +	/* Retire requests first as we use it above for the early return.
> +	 * If we retire requests last, we may use a later seqno and so clear
> +	 * the requests lists without clearing the active list, leading to
> +	 * confusion.
>  	 */
> -	while (!list_empty(&ring->active_list)) {
> -		struct drm_i915_gem_object *obj;
> -
> -		obj = list_first_entry(&ring->active_list,
> -				      struct drm_i915_gem_object,
> -				      ring_list);
> -
> -		if (!i915_gem_request_completed(obj->last_read_req, true))
> -			break;
> -
> -		i915_gem_object_move_to_inactive(obj);
> -	}
> -
> -
>  	while (!list_empty(&ring->request_list)) {
>  		struct drm_i915_gem_request *request;
>  
> @@ -2700,6 +2687,23 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
>  		i915_gem_free_request(request);
>  	}
>  
> +	/* Move any buffers on the active list that are no longer referenced
> +	 * by the ringbuffer to the flushing/inactive lists as appropriate,
> +	 * before we free the context associated with the requests.
> +	 */
> +	while (!list_empty(&ring->active_list)) {
> +		struct drm_i915_gem_object *obj;
> +
> +		obj = list_first_entry(&ring->active_list,
> +				      struct drm_i915_gem_object,
> +				      ring_list);
> +
> +		if (!i915_gem_request_completed(obj->last_read_req, true))
> +			break;
> +
> +		i915_gem_object_move_to_inactive(obj);
> +	}
> +
>  	if (unlikely(ring->trace_irq_req &&
>  		     i915_gem_request_completed(ring->trace_irq_req, true))) {
>  		ring->irq_put(ring);
> -- 
> 2.1.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch