Re: [PATCH] drm/i915: Free requests after object release when retiring requests

[Ben Widawsky <ben@bwidawsk.net>]

On Tue, Jan 07, 2014 at 11:45:14AM +0000, Chris Wilson wrote:
> Freeing a request triggers the destruction of the context. This needs to
> occur after all objects are themselves unbound from the context, and so
> the free request needs to occur after the object release during retire.
> 
> This tidies up
> 
> commit e20780439b26ba95aeb29d3e27cd8cc32bc82a4c
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Fri Dec 6 14:11:22 2013 -0800
> 
>     drm/i915: Defer request freeing
> 
> by simply swapping the order of operations rather than introducing
> further complexity - as noted during review.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 47 ++++++++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a5579a317b85..0a2055b736c4 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2560,8 +2560,6 @@ void i915_gem_reset(struct drm_device *dev)
>  void
>  i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
>  {
> -	LIST_HEAD(deferred_request_free);
> -	struct drm_i915_gem_request *request;
>  	uint32_t seqno;
>  
>  	if (list_empty(&ring->request_list))
> @@ -2571,7 +2569,27 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
>  
>  	seqno = ring->get_seqno(ring, true);
>  
> +	/* 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_seqno_passed(seqno, obj->last_read_seqno))
> +			break;
> +
> +		i915_gem_object_move_to_inactive(obj);
> +	}
> +
> +
>  	while (!list_empty(&ring->request_list)) {
> +		struct drm_i915_gem_request *request;
> +
>  		request = list_first_entry(&ring->request_list,
>  					   struct drm_i915_gem_request,
>  					   list);
> @@ -2587,23 +2605,7 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
>  		 */
>  		ring->last_retired_head = request->tail;
>  
> -		list_move_tail(&request->list, &deferred_request_free);
> -	}
> -
> -	/* Move any buffers on the active list that are no longer referenced
> -	 * by the ringbuffer to the flushing/inactive lists as appropriate.
> -	 */
> -	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_seqno_passed(seqno, obj->last_read_seqno))
> -			break;
> -
> -		i915_gem_object_move_to_inactive(obj);
> +		i915_gem_free_request(request);
>  	}
>  
>  	if (unlikely(ring->trace_irq_seqno &&
> @@ -2612,13 +2614,6 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring)
>  		ring->trace_irq_seqno = 0;
>  	}
>  
> -	/* Finish processing active list before freeing request */
> -	while (!list_empty(&deferred_request_free)) {
> -		request = list_first_entry(&deferred_request_free,
> -					   struct drm_i915_gem_request,
> -					   list);
> -		i915_gem_free_request(request);
> -	}
>  	WARN_ON(i915_verify_lists(ring->dev));
>  }
>  
> -- 
> 1.8.5.2
> 

I had a reason for not doing this originally - but I can no longer
reconstruct what it was. Looking at it again now, it seems the only
difference is with setting ring->last_retired_head, with your patch that
gets set after the object is on the inactive list. Nothing seems wrong
with that, to me.

So with the caveat that I thought this was a bad idea at one point in
time:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>



-- 
Ben Widawsky, Intel Open Source Technology Center