| Message ID | 20190330100349.30642-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Avoid using ctx->file_priv during construction | expand |
I think the change is focused mainly around setting the vm param, so perhaps the subject should mention that. Maybe something like: drm/i915: Avoid using ctx->file_priv for VM param during construction I guess a similar issue could arise if other context params are added later. Hopefully any issues can be caught by making sure to add tests for setting the new context params at context creation time. Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> and, I verified it fixes the segfault when setting vm at context create time, so: Tested-by: Jordan Justen <jordan.l.justen@intel.com> On 2019-03-30 03:03:49, Chris Wilson wrote: > As we only set ctx->file_priv on registering the GEM context after > construction, it is invalid to try and use it in the middle for setting > various parameters. Indeed, we put the file_priv into struct create_ext > so that we have the right file_private available without having to look > at ctx->file_priv. However, it helps to use it! > > Reported-by: Jordan Justen <jordan.l.justen@intel.com> > Fixes: b91715417244 ("drm/i915: Extend CONTEXT_CREATE to set parameters upon construction") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 662da485e15f..141da4e71e46 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -969,10 +969,10 @@ int i915_gem_switch_to_kernel_context(struct drm_i915_private *i915, > return 0; > } > > -static int get_ppgtt(struct i915_gem_context *ctx, > +static int get_ppgtt(struct drm_i915_file_private *file_priv, > + struct i915_gem_context *ctx, > struct drm_i915_gem_context_param *args) > { > - struct drm_i915_file_private *file_priv = ctx->file_priv; > struct i915_hw_ppgtt *ppgtt; > int ret; > > @@ -1071,10 +1071,10 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data) > return 0; > } > > -static int set_ppgtt(struct i915_gem_context *ctx, > +static int set_ppgtt(struct drm_i915_file_private *file_priv, > + struct i915_gem_context *ctx, > struct drm_i915_gem_context_param *args) > { > - struct drm_i915_file_private *file_priv = ctx->file_priv; > struct i915_hw_ppgtt *ppgtt, *old; > int err; > > @@ -1416,7 +1416,8 @@ static int set_sseu(struct i915_gem_context *ctx, > return 0; > } > > -static int ctx_setparam(struct i915_gem_context *ctx, > +static int ctx_setparam(struct drm_i915_file_private *fpriv, > + struct i915_gem_context *ctx, > struct drm_i915_gem_context_param *args) > { > int ret = 0; > @@ -1485,7 +1486,7 @@ static int ctx_setparam(struct i915_gem_context *ctx, > break; > > case I915_CONTEXT_PARAM_VM: > - ret = set_ppgtt(ctx, args); > + ret = set_ppgtt(fpriv, ctx, args); > break; > > case I915_CONTEXT_PARAM_BAN_PERIOD: > @@ -1513,7 +1514,7 @@ static int create_setparam(struct i915_user_extension __user *ext, void *data) > if (local.param.ctx_id) > return -EINVAL; > > - return ctx_setparam(arg->ctx, &local.param); > + return ctx_setparam(arg->fpriv, arg->ctx, &local.param); > } > > static const i915_user_extension_fn create_extensions[] = { > @@ -1712,7 +1713,7 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data, > break; > > case I915_CONTEXT_PARAM_VM: > - ret = get_ppgtt(ctx, args); > + ret = get_ppgtt(file_priv, ctx, args); > break; > > case I915_CONTEXT_PARAM_BAN_PERIOD: > @@ -1737,7 +1738,7 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data, > if (!ctx) > return -ENOENT; > > - ret = ctx_setparam(ctx, args); > + ret = ctx_setparam(file_priv, ctx, args); > > i915_gem_context_put(ctx); > return ret; > -- > 2.20.1 >
Quoting Jordan Justen (2019-03-31 04:03:44) > I think the change is focused mainly around setting the vm param, so > perhaps the subject should mention that. Maybe something like: It's not just about that, it's the design in how create_ext is run before registration which caters for more than just vm. The problem already exists for the other extensions posted, I caught the bug in create_ext_clone and overlooked that I had been exclusively using normal ctx_setparam to manipulate the ppgtt. -Chris
On Sat, 30 Mar 2019 11:03:49 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > As we only set ctx->file_priv on registering the GEM context after > construction, it is invalid to try and use it in the middle for setting Other option would be to set ctx->file_priv ahead of gem_context_register and use gem_context_register only for registering (per function name) Extra bonus would be that changed here static ctx functions will continue to take ctx as first parameter (same as other existing ctx functions) Michal > various parameters. Indeed, we put the file_priv into struct create_ext > so that we have the right file_private available without having to look > at ctx->file_priv. However, it helps to use it! >
On 2019-03-31 00:32:52, Chris Wilson wrote: > Quoting Jordan Justen (2019-03-31 04:03:44) > > I think the change is focused mainly around setting the vm param, so > > perhaps the subject should mention that. Maybe something like: > > It's not just about that, it's the design in how create_ext is run > before registration which caters for more than just vm. The problem > already exists for the other extensions posted, I caught the bug in > create_ext_clone and overlooked that I had been exclusively using normal > ctx_setparam to manipulate the ppgtt. I guess I disagree for two reasons. 1. I think this patch only addresses the symptom with the vm param 2. It doesn't really prevent someone from adding a new param and accidentally doing the same thing. But, feel free to keep the r-b and t-b even if you don't want to change the subject line. -Jordan
Quoting Michal Wajdeczko (2019-03-31 10:12:52) > On Sat, 30 Mar 2019 11:03:49 +0100, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > > > As we only set ctx->file_priv on registering the GEM context after > > construction, it is invalid to try and use it in the middle for setting > > Other option would be to set ctx->file_priv ahead of gem_context_register > and use gem_context_register only for registering (per function name) I thought it might be useful for us to distinguish between unregistered protocontexts and unregistered kernel contexts with registered GEM contexts. > Extra bonus would be that changed here static ctx functions will continue > to take ctx as first parameter (same as other existing ctx functions) The only real use for ctx->file is for identifying the right lut in i915_gem_close_object() (the other is for charging a hang against a file, which is going to be more complicated in future, so expect changes). Ideas for avoiding the search along the linked list in close are most welcome. https://patchwork.freedesktop.org/patch/291947/?series=57942&rev=2 -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 662da485e15f..141da4e71e46 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -969,10 +969,10 @@ int i915_gem_switch_to_kernel_context(struct drm_i915_private *i915, return 0; } -static int get_ppgtt(struct i915_gem_context *ctx, +static int get_ppgtt(struct drm_i915_file_private *file_priv, + struct i915_gem_context *ctx, struct drm_i915_gem_context_param *args) { - struct drm_i915_file_private *file_priv = ctx->file_priv; struct i915_hw_ppgtt *ppgtt; int ret; @@ -1071,10 +1071,10 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data) return 0; } -static int set_ppgtt(struct i915_gem_context *ctx, +static int set_ppgtt(struct drm_i915_file_private *file_priv, + struct i915_gem_context *ctx, struct drm_i915_gem_context_param *args) { - struct drm_i915_file_private *file_priv = ctx->file_priv; struct i915_hw_ppgtt *ppgtt, *old; int err; @@ -1416,7 +1416,8 @@ static int set_sseu(struct i915_gem_context *ctx, return 0; } -static int ctx_setparam(struct i915_gem_context *ctx, +static int ctx_setparam(struct drm_i915_file_private *fpriv, + struct i915_gem_context *ctx, struct drm_i915_gem_context_param *args) { int ret = 0; @@ -1485,7 +1486,7 @@ static int ctx_setparam(struct i915_gem_context *ctx, break; case I915_CONTEXT_PARAM_VM: - ret = set_ppgtt(ctx, args); + ret = set_ppgtt(fpriv, ctx, args); break; case I915_CONTEXT_PARAM_BAN_PERIOD: @@ -1513,7 +1514,7 @@ static int create_setparam(struct i915_user_extension __user *ext, void *data) if (local.param.ctx_id) return -EINVAL; - return ctx_setparam(arg->ctx, &local.param); + return ctx_setparam(arg->fpriv, arg->ctx, &local.param); } static const i915_user_extension_fn create_extensions[] = { @@ -1712,7 +1713,7 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data, break; case I915_CONTEXT_PARAM_VM: - ret = get_ppgtt(ctx, args); + ret = get_ppgtt(file_priv, ctx, args); break; case I915_CONTEXT_PARAM_BAN_PERIOD: @@ -1737,7 +1738,7 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data, if (!ctx) return -ENOENT; - ret = ctx_setparam(ctx, args); + ret = ctx_setparam(file_priv, ctx, args); i915_gem_context_put(ctx); return ret;
As we only set ctx->file_priv on registering the GEM context after construction, it is invalid to try and use it in the middle for setting various parameters. Indeed, we put the file_priv into struct create_ext so that we have the right file_private available without having to look at ctx->file_priv. However, it helps to use it! Reported-by: Jordan Justen <jordan.l.justen@intel.com> Fixes: b91715417244 ("drm/i915: Extend CONTEXT_CREATE to set parameters upon construction") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> --- drivers/gpu/drm/i915/i915_gem_context.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)