Quoting Tvrtko Ursulin (2019-03-18 16:45:41)
>
> 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@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.
>
> Ugh, bad.. Can we have struct_mutex nest under the context_idr_lock?
General rule is to keep struct_mutex the outer lock. If you take
struct_mutex inside another lock, that lock picks up all the
struct_mutex bad habits from lockdep, and you suddenly find yourself in
a mighty world of pain.
Removing the requirement of struct_mutex around close isn't a huge
problem after
https://patchwork.freedesktop.org/patch/291947/?series=57942&rev=2 as
that gives us the locking we need to serialise the lut handles (in fact
it the struct_mutex requirement drops out of that patch, and we can
simply do it then.)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
