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@xxxxxxxxxxxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > ---
> > 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.
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
