On Thu, Jun 26, 2014 at 06:24:01PM +0100, John.C.Harrison@xxxxxxxxx wrote:
> From: John Harrison <John.C.Harrison@xxxxxxxxx>
>
> A major point of the GPU scheduler is that it re-orders batch buffers after they
> have been submitted to the driver. Rather than attempting to re-assign seqno
> values, it is much simpler to have each batch buffer keep its initially assigned
> number and modify the rest of the driver to cope with seqnos being returned out
> of order. In practice, very little code actually needs updating to cope.
>
> One such place is the retire request handler. Rather than stopping as soon as an
> uncompleted seqno is found, it must now keep iterating through the requests in
> case later seqnos have completed. There is also a problem with doing the free of
> the request before the move to inactive. Thus the requests are now moved to a
> temporary list first, then the objects de-activated and finally the requests on
> the temporary list are freed.
I still hold that we should track requests, not seqno+ring pairs. At least
the plan with Maarten's fencing patches is to embedded the generic struct
fence into our i915_gem_request structure. And struct fence will also be
the kernel-internal represenation of a android native sync fence.
So splatter ring+seqno->request/fence lookups all over the place isn't a
good way forward. It's ok for bring up, but for merging we should do that
kind of large-scale refactoring upfront to reduce rebase churn. Oscar
knows how this works.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_gem.c | 60 +++++++++++++++++++++------------------
> 1 file changed, 32 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index b784eb2..7e53446 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2602,7 +2602,10 @@ void i915_gem_reset(struct drm_device *dev)
> void
> i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
> {
> + struct drm_i915_gem_object *obj, *obj_next;
> + struct drm_i915_gem_request *req, *req_next;
> uint32_t seqno;
> + LIST_HEAD(deferred_request_free);
>
> if (list_empty(&ring->request_list))
> return;
> @@ -2611,43 +2614,35 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *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.
> + /* Note that seqno values might be out of order due to rescheduling and
> + * pre-emption. Thus both lists must be processed in their entirety
> + * rather than stopping at the first 'non-passed' entry.
> */
> - 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);
> -
> - if (!i915_seqno_passed(seqno, request->seqno))
> - break;
> + list_for_each_entry_safe(req, req_next, &ring->request_list, list) {
> + if (!i915_seqno_passed(seqno, req->seqno))
> + continue;
>
> - trace_i915_gem_request_retire(ring, request->seqno);
> + trace_i915_gem_request_retire(ring, req->seqno);
> /* We know the GPU must have read the request to have
> * sent us the seqno + interrupt, so use the position
> * of tail of the request to update the last known position
> * of the GPU head.
> */
> - ring->buffer->last_retired_head = request->tail;
> + ring->buffer->last_retired_head = req->tail;
>
> - i915_gem_free_request(request);
> + list_move_tail(&req->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,
> + * before we free the context associated with the requests.
> + */
> + list_for_each_entry_safe(obj, obj_next, &ring->active_list, ring_list) {
> + if (!i915_seqno_passed(seqno, obj->last_read_seqno))
> + continue;
> +
> + i915_gem_object_move_to_inactive(obj);
> }
>
> if (unlikely(ring->trace_irq_seqno &&
> @@ -2656,6 +2651,15 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
> ring->trace_irq_seqno = 0;
> }
>
> + /* Finish processing active list before freeing request */
> + while (!list_empty(&deferred_request_free)) {
> + req = list_first_entry(&deferred_request_free,
> + struct drm_i915_gem_request,
> + list);
> +
> + i915_gem_free_request(req);
> + }
> +
> WARN_ON(i915_verify_lists(ring->dev));
> }
>
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
