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

[Tvrtko Ursulin]

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.
> 
> 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