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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux