| Message ID | 20190318095204.9913-11-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/22] drm/i915: Flush pages on acquisition | expand |
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@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@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.. 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? > context_close(ctx); > - > mutex_unlock(&dev->struct_mutex); > > -out: > - i915_gem_context_put(ctx); > return 0; > } > > Regards, Tvrtko
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@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@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. > 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
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@chris-wilson.co.uk> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@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 >
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@chris-wilson.co.uk> > >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@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? 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
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 */ 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); context_close(ctx); - mutex_unlock(&dev->struct_mutex); -out: - i915_gem_context_put(ctx); return 0; }
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@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@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(-)