[Intel-gfx] [PATCH v2] drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU

[Tvrtko Ursulin]

On 09/11/2017 09:32, Tvrtko Ursulin wrote:
> 
> On 09/11/2017 08:55, Chris Wilson wrote:
>> When we close the VMA, we unbind it from the ppgtt and tear down the
>> page directory pointing at it. That may trigger us to return WC pages
>> back to the system, requiring conversion back to WB which itself may
>> sleep. That makes i915_vma_close() unsuitable for use inside the RCU
>> read lock, which we need to hold to iterate the radixtree.

Would it be worth adding might_sleep to i915_vma_close? Or at a lower layer?

Regards,

Tvrtko

>>
>> The fix is quite simple, we can close all the VMA as we close the ppgtt,
>> we only need to do that instead of closing them during destruction of
>> the LUT.
>>
>> v2: Order between closing the LUT and the ppgtt is important; we use the
>> vma inside the LUT as a means of retrieving the object, and so we must
>> clear the LUT before freeing the VMA when closing the ppgtt.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103638
>> Fixes: 547da76b5777 ("drm/i915: Hold rcu_read_lock when iterating over 
>> the radixtree (vma idr)")
>> Fixes: d1b48c1e7184 ("drm/i915: Replace execbuf vma ht with an idr")
>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Matthew Auld <matthew.william.auld at gmail.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem_context.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
>> b/drivers/gpu/drm/i915/i915_gem_context.c
>> index 10affb35ac56..c05c3d7d21a5 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>> @@ -107,14 +107,9 @@ static void lut_close(struct i915_gem_context *ctx)
>>       rcu_read_lock();
>>       radix_tree_for_each_slot(slot, &ctx->handles_vma, &iter, 0) {
>>           struct i915_vma *vma = rcu_dereference_raw(*slot);
>> -        struct drm_i915_gem_object *obj = vma->obj;
>>           radix_tree_iter_delete(&ctx->handles_vma, &iter, slot);
>> -
>> -        if (!i915_vma_is_ggtt(vma))
>> -            i915_vma_close(vma);
>> -
>> -        __i915_gem_object_release_unless_active(obj);
>> +        __i915_gem_object_release_unless_active(vma->obj);
>>       }
>>       rcu_read_unlock();
>>   }
>> @@ -200,6 +195,11 @@ static void context_close(struct i915_gem_context 
>> *ctx)
>>   {
>>       i915_gem_context_set_closed(ctx);
>> +    /*
>> +     * The LUT uses the VMA as a backpointer to unref the object,
>> +     * so we need to clear the LUT before we close all the VMA (inside
>> +     * the ppgtt).
>> +     */
>>       lut_close(ctx);
>>       if (ctx->ppgtt)
>>           i915_ppgtt_close(&ctx->ppgtt->base);
>>
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> Regards,
> 
> Tvrtko