On Mon, 23 Mar 2015, Daniel Vetter <daniel@xxxxxxxx> 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@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."
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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
