[Intel-gfx] [PATCH 11/22] drm/i915: Introduce a mutex for file_priv->context_idr
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Mon Mar 18 16:45:41 UTC 2019
On 18/03/2019 16:35, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-03-18 16:28:35)
>>
>> 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..
>
> At this point, interrupt handling becomes problematic, as we have to
> then re-insert the ctx_id into the idr and that may have already been
> claimed elsewhere.
Ugh, bad.. Can we have struct_mutex nest under the context_idr_lock?
Regards,
Tvrtko
>> 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?
>
> Not practical imo; but we won't need struct_mutex here in about another 20
> patches :)
> -Chris
>
More information about the Intel-gfx
mailing list