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@xxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxx>
> Cc: Ben Widawsky <ben@xxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx>
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
