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: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




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

  Powered by Linux