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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 09/11/2017 08:20, 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.
> 
> 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@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Matthew Auld <matthew.william.auld@gmail.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_context.c | 10 +++-------
>   1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 10affb35ac56..0e1c4626870e 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,10 +195,11 @@ static void context_close(struct i915_gem_context *ctx)
>   {
>   	i915_gem_context_set_closed(ctx);
>   
> -	lut_close(ctx);
>   	if (ctx->ppgtt)
>   		i915_ppgtt_close(&ctx->ppgtt->base);
>   
> +	lut_close(ctx);
> +
>   	ctx->file_priv = ERR_PTR(-EBADF);
>   	i915_gem_context_put(ctx);
>   }
> 

Looks straightforward enough. Was a bit unsure about how the pre-ppgtt 
world works but that too looks fine.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx