[Intel-gfx] [PATCH] drm/i915: Ensure associated VMAs are inactive when contexts are destroyed

[Tvrtko Ursulin]

On 26/10/15 12:10, Chris Wilson wrote:
> On Mon, Oct 26, 2015 at 12:00:06PM +0000, Tvrtko Ursulin wrote:
>>
>> On 26/10/15 11:23, Chris Wilson wrote:
>>> On Mon, Oct 26, 2015 at 11:05:03AM +0000, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>
>>>> In the following commit:
>>>>
>>>>      commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae
>>>>      Author: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>      Date:   Mon Oct 5 13:26:36 2015 +0100
>>>>
>>>>          drm/i915: Clean up associated VMAs on context destruction
>>>>
>>>> I added a WARN_ON assertion that VM's active list must be empty
>>>> at the time of owning context is getting freed, but that turned
>>>> out to be a wrong assumption.
>>>>
>>>> Due ordering of operations in i915_gem_object_retire__read, where
>>>> contexts are unreferenced before VMAs are moved to the inactive
>>>> list, the described situation can in fact happen.
>>>
>>> The context is being unreferenced indirectly. Adding a direct reference
>>> here is even more bizarre.
>>
>> Perhaps is not the prettiest, but it sounds logical to me to ensure
>> that order of destruction of involved object hierarchy goes from the
>> bottom-up and is not interleaved.
>>
>> If you consider the active/inactive list position as part of the
>> retire process, doing it at the very place in code, and the very
>> object that looked to be destroyed out of sequence, to me sounded
>> logical.
>>
>> How would you do it, can you think of a better way?
> 
> The reference is via the request. We are handling requests, it makes
> more sense that you take the reference on the request.

Hm, so you would be happy with:

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9b2048c7077d..c238481a8090 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2373,19 +2373,26 @@ static void
 i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
 {
        struct i915_vma *vma;
+       struct drm_i915_gem_request *req;
 
        RQ_BUG_ON(obj->last_read_req[ring] == NULL);
        RQ_BUG_ON(!(obj->active & (1 << ring)));
 
        list_del_init(&obj->ring_list[ring]);
+
+       /* Ensure context cannot be destroyed with VMAs on the active list. */
+       req = i915_gem_request_reference(obj->last_read_req[ring]);
+
        i915_gem_request_assign(&obj->last_read_req[ring], NULL);
 
        if (obj->last_write_req && obj->last_write_req->ring->id == ring)
                i915_gem_object_retire__write(obj);
 
        obj->active &= ~(1 << ring);
-       if (obj->active)
+       if (obj->active) {
+               i915_gem_request_unreference(req);
                return;
+       }
 
        /* Bump our place on the bound list to keep it roughly in LRU order
         * so that we don't steal from recently used but inactive objects
@@ -2399,6 +2406,8 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
                        list_move_tail(&vma->mm_list, &vma->vm->inactive_list);
        }
 
+       i915_gem_request_unreference(req);
+
        i915_gem_request_assign(&obj->last_fenced_req, NULL);
        drm_gem_object_unreference(&obj->base);
 }
 
> I would just revert the patch, it doesn't fix the problem you tried to
> solve and just adds more.

It solves one problem, just not all of them.

Regards,

Tvrtko