[Intel-gfx] [PATCH 11/22] drm/i915: Introduce a mutex for file_priv->context_idr

[Tvrtko Ursulin]

On 18/03/2019 09:51, 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.)
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h         |  2 ++
>   drivers/gpu/drm/i915/i915_gem_context.c | 43 +++++++++++--------------
>   2 files changed, 21 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 86080a6e0f45..90389333dd47 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_lock; /* guards context_idr */

context_idr_lock then?

>   
>   	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 5df3d423ec6c..94c466d4b29e 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;
>   }
>   
> @@ -596,8 +594,10 @@ static int gem_context_register(struct i915_gem_context *ctx,
>   		ctx->ppgtt->vm.file = fpriv;
>   
>   	/* And (nearly) finally expose ourselves to userspace via the idr */
> +	mutex_lock(&fpriv->context_lock);
>   	ret = idr_alloc(&fpriv->context_idr, ctx,
>   			DEFAULT_CONTEXT_HANDLE, 0, GFP_KERNEL);
> +	mutex_unlock(&fpriv->context_lock);
>   	if (ret < 0)
>   		goto err_pid;
>   
> @@ -616,7 +616,9 @@ static int gem_context_register(struct i915_gem_context *ctx,
>   	return 0;
>   
>   err_idr:
> +	mutex_lock(&fpriv->context_lock);
>   	idr_remove(&fpriv->context_idr, ctx->user_handle);
> +	mutex_unlock(&fpriv->context_lock);
>   	ctx->file_priv = NULL;
>   err_pid:
>   	put_pid(ctx->pid);
> @@ -632,10 +634,11 @@ int i915_gem_context_open(struct drm_i915_private *i915,
>   	int err;
>   
>   	idr_init(&file_priv->context_idr);
> +	mutex_init(&file_priv->context_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;
> @@ -648,14 +651,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_lock);
>   	idr_destroy(&file_priv->context_idr);
>   	return PTR_ERR(ctx);
>   }
> @@ -668,6 +671,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_lock);
>   }
>   
>   static struct i915_request *
> @@ -850,25 +854,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;
>   }
> @@ -879,7 +880,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;
> @@ -887,21 +887,16 @@ 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);
> +	mutex_lock(&file_priv->context_lock);
> +	ctx = idr_remove(&file_priv->context_idr, args->ctx_id);
> +	mutex_lock(&file_priv->context_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);

I'd keep this one interruptible. Hm bummer, there was more of them before..

I mean the new mutex we can probably get away by not bothering, since it 
guards so little, but struct mutex, since you are touching those lines 
anyway, what do you think about making it interruptible in all ioctl paths?

>   	context_close(ctx);
> -
>   	mutex_unlock(&dev->struct_mutex);
>   
> -out:
> -	i915_gem_context_put(ctx);
>   	return 0;
>   }
>   
> 

Regards,

Tvrtko