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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 23 Mar 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> 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@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@chris-wilson.co.uk>
>> Cc: John Harrison <John.C.Harrison@Intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> In case it's burried too much in the thread:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@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."

Pushed to drm-intel-fixes with the above note added. Thanks for the
patch and review.

BR,
Jani.


>
> 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx