Re: [PATCH 05/18] drm/i915: Introduce a mutex for file_priv->context_idr

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 19/03/2019 11:57, Chris Wilson wrote:
> Define a mutex for the exclusive use of interacting with the per-file
> context-idr, that was previously guarded by struct_mutex. This allows us
> to reduce the coverage of struct_mutex, with a view to removing the last
> bits coordinating GEM context later. (In the short term, we avoid taking
> struct_mutex while using the extended constructor functions, preventing
> some nasty recursion.)
>
> v2: s/context_lock/context_idr_lock/
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/i915_drv.h         |  2 ++
>   drivers/gpu/drm/i915/i915_gem_context.c | 47 +++++++++++--------------
>   2 files changed, 23 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 86080a6e0f45..219348121897 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -216,7 +216,9 @@ struct drm_i915_file_private {
>    */
>   #define DRM_I915_THROTTLE_JIFFIES msecs_to_jiffies(20)
>   	} mm;
> +
>   	struct idr context_idr;
> +	struct mutex context_idr_lock; /* guards context_idr */
>   
>   	unsigned int bsd_engine;
>   
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index dff4220df911..799684d05704 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -579,9 +579,7 @@ void i915_gem_contexts_fini(struct drm_i915_private *i915)
>   
>   static int context_idr_cleanup(int id, void *p, void *data)
>   {
> -	struct i915_gem_context *ctx = p;
> -
> -	context_close(ctx);
> +	context_close(p);
>   	return 0;
>   }
>   
> @@ -603,13 +601,15 @@ static int gem_context_register(struct i915_gem_context *ctx,
>   	}
>   
>   	/* And finally expose ourselves to userspace via the idr */
> +	mutex_lock(&fpriv->context_idr_lock);
>   	ret = idr_alloc(&fpriv->context_idr, ctx,
>   			DEFAULT_CONTEXT_HANDLE, 0, GFP_KERNEL);
> +	if (ret >= 0)
> +		ctx->user_handle = ret;
> +	mutex_unlock(&fpriv->context_idr_lock);
>   	if (ret < 0)
>   		goto err_name;
>   
> -	ctx->user_handle = ret;
> -
>   	return 0;
>   
>   err_name:
> @@ -627,10 +627,11 @@ int i915_gem_context_open(struct drm_i915_private *i915,
>   	int err;
>   
>   	idr_init(&file_priv->context_idr);
> +	mutex_init(&file_priv->context_idr_lock);
>   
>   	mutex_lock(&i915->drm.struct_mutex);
> -
>   	ctx = i915_gem_create_context(i915);
> +	mutex_unlock(&i915->drm.struct_mutex);
>   	if (IS_ERR(ctx)) {
>   		err = PTR_ERR(ctx);
>   		goto err;
> @@ -643,14 +644,14 @@ int i915_gem_context_open(struct drm_i915_private *i915,
>   	GEM_BUG_ON(ctx->user_handle != DEFAULT_CONTEXT_HANDLE);
>   	GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
>   
> -	mutex_unlock(&i915->drm.struct_mutex);
> -
>   	return 0;
>   
>   err_ctx:
> +	mutex_lock(&i915->drm.struct_mutex);
>   	context_close(ctx);
> -err:
>   	mutex_unlock(&i915->drm.struct_mutex);
> +err:
> +	mutex_destroy(&file_priv->context_idr_lock);
>   	idr_destroy(&file_priv->context_idr);
>   	return PTR_ERR(ctx);
>   }
> @@ -663,6 +664,7 @@ void i915_gem_context_close(struct drm_file *file)
>   
>   	idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
>   	idr_destroy(&file_priv->context_idr);
> +	mutex_destroy(&file_priv->context_idr_lock);
>   }
>   
>   static struct i915_request *
> @@ -845,25 +847,22 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>   		return ret;
>   
>   	ctx = i915_gem_create_context(i915);
> -	if (IS_ERR(ctx)) {
> -		ret = PTR_ERR(ctx);
> -		goto err_unlock;
> -	}
> +	mutex_unlock(&dev->struct_mutex);
> +	if (IS_ERR(ctx))
> +		return PTR_ERR(ctx);
>   
>   	ret = gem_context_register(ctx, file_priv);
>   	if (ret)
>   		goto err_ctx;
>   
> -	mutex_unlock(&dev->struct_mutex);
> -
>   	args->ctx_id = ctx->user_handle;
>   	DRM_DEBUG("HW context %d created\n", args->ctx_id);
>   
>   	return 0;
>   
>   err_ctx:
> +	mutex_lock(&dev->struct_mutex);
>   	context_close(ctx);
> -err_unlock:
>   	mutex_unlock(&dev->struct_mutex);
>   	return ret;
>   }
> @@ -874,7 +873,6 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>   	struct drm_i915_gem_context_destroy *args = data;
>   	struct drm_i915_file_private *file_priv = file->driver_priv;
>   	struct i915_gem_context *ctx;
> -	int ret;
>   
>   	if (args->pad != 0)
>   		return -EINVAL;
> @@ -882,21 +880,18 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>   	if (args->ctx_id == DEFAULT_CONTEXT_HANDLE)
>   		return -ENOENT;
>   
> -	ctx = i915_gem_context_lookup(file_priv, args->ctx_id);
> +	if (mutex_lock_interruptible(&file_priv->context_idr_lock))
> +		return -EINTR;
> +
> +	ctx = idr_remove(&file_priv->context_idr, args->ctx_id);
> +	mutex_unlock(&file_priv->context_idr_lock);
>   	if (!ctx)
>   		return -ENOENT;
>   
> -	ret = mutex_lock_interruptible(&dev->struct_mutex);
> -	if (ret)
> -		goto out;
> -
> -	idr_remove(&file_priv->context_idr, ctx->user_handle);
> +	mutex_lock(&dev->struct_mutex);
>   	context_close(ctx);
> -
>   	mutex_unlock(&dev->struct_mutex);
>   
> -out:
> -	i915_gem_context_put(ctx);
>   	return 0;
>   }
>   

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

Regards,

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