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@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxx>
> Cc: John Harrison <John.C.Harrison@xxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
In case it's burried too much in the thread:
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
