| Message ID | 20190412085410.10392-25-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/50] drm/i915: Introduce struct class_instance for engines across the uAPI | expand |
On 12/04/2019 09:53, Chris Wilson wrote: > A usecase arose out of handling context recovery in mesa, whereby they > wish to recreate a context with fresh logical state but preserving all > other details of the original. Currently, they create a new context and > iterate over which bits they want to copy across, but it would much more > convenient if they were able to just pass in a target context to clone > during creation. This essentially extends the setparam during creation > to pull the details from a target context instead of the user supplied > parameters. > > The ideal here is that we don't expose control over anything more than > can be obtained via CONTEXT_PARAM. That is userspace retains explicit > control over all features, and this api is just convenience. > > For example, you could replace > > struct context_param p = { .param = CONTEXT_PARAM_VM }; > > param.ctx_id = old_id; > gem_context_get_param(&p.param); > > new_id = gem_context_create(); > > param.ctx_id = new_id; > gem_context_set_param(&p.param); > > gem_vm_destroy(param.value); /* drop the ref to VM_ID handle */ > > with > > struct create_ext_param p = { > { .name = CONTEXT_CREATE_CLONE }, > .clone_id = old_id, > .flags = CLONE_FLAGS_VM > } > new_id = gem_context_create_ext(&p); > > and not have to worry about stray namespace pollution etc. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 207 ++++++++++++++++++++++++ > include/uapi/drm/i915_drm.h | 15 ++ > 2 files changed, 222 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index ad9920f703c6..27c22df59475 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -1685,8 +1685,215 @@ static int create_setparam(struct i915_user_extension __user *ext, void *data) > return ctx_setparam(arg->fpriv, arg->ctx, &local.param); > } > > +static int clone_engines(struct i915_gem_context *dst, > + struct i915_gem_context *src) > +{ > + struct i915_gem_engines *e, *clone; > + bool user_engines; > + unsigned long n; > + > + e = i915_gem_context_engine_list_lock(src); > + > + clone = kmalloc(struct_size(e, engines, e->num_engines), GFP_KERNEL); > + if (!clone) > + goto err_unlock; > + > + clone->i915 = dst->i915; > + for (n = 0; n < e->num_engines; n++) { > + if (!e->engines[n]) { > + clone->engines[n] = NULL; > + continue; > + } > + > + clone->engines[n] = > + intel_context_create(dst, e->engines[n]->engine); > + if (!clone->engines[n]) { > + __free_engines(clone, n); > + goto err_unlock; > + } > + } > + clone->num_engines = n; > + > + user_engines = i915_gem_context_user_engines(src); > + i915_gem_context_engine_list_unlock(src); > + > + free_engines(dst->engines); > + RCU_INIT_POINTER(dst->engines, clone); > + if (user_engines) > + i915_gem_context_set_user_engines(dst); > + else > + i915_gem_context_clear_user_engines(dst); > + return 0; > + > +err_unlock: > + i915_gem_context_engine_list_unlock(src); > + return -ENOMEM; > +} > + > +static int clone_flags(struct i915_gem_context *dst, > + struct i915_gem_context *src) > +{ > + dst->user_flags = src->user_flags; > + return 0; > +} > + > +static int clone_schedattr(struct i915_gem_context *dst, > + struct i915_gem_context *src) > +{ > + dst->sched = src->sched; > + return 0; > +} > + > +static int clone_sseu(struct i915_gem_context *dst, > + struct i915_gem_context *src) > +{ > + struct i915_gem_engines *e, *clone; > + unsigned long n; > + int err; > + > + clone = dst->engines; /* no locking required; sole access */ > + e = i915_gem_context_engine_list_lock(src); > + if (e->num_engines != clone->num_engines) { > + err = -EINVAL; > + goto unlock; > + } > + > + for (n = 0; n < e->num_engines; n++) { > + struct intel_context *ce = e->engines[n]; > + > + if (clone->engines[n]->engine->class != ce->engine->class) { > + /* Must have compatible engine maps! */ > + err = -EINVAL; > + goto unlock; > + } > + > + /* serialises with set_sseu */ > + err = intel_context_lock_pinned(ce); > + if (err) > + goto unlock; > + > + clone->engines[n]->sseu = ce->sseu; > + intel_context_unlock_pinned(ce); > + } > + > + err = 0; > +unlock: > + i915_gem_context_engine_list_unlock(src); > + return err; > +} > + > +static int clone_timeline(struct i915_gem_context *dst, > + struct i915_gem_context *src) > +{ > + if (src->timeline) { > + GEM_BUG_ON(src->timeline == dst->timeline); > + > + if (dst->timeline) > + i915_timeline_put(dst->timeline); > + dst->timeline = i915_timeline_get(src->timeline); > + } > + > + return 0; > +} > + > +static int clone_vm(struct i915_gem_context *dst, > + struct i915_gem_context *src) > +{ > + struct i915_hw_ppgtt *ppgtt; > + > + rcu_read_lock(); > + do { > + ppgtt = READ_ONCE(src->ppgtt); > + if (!ppgtt) > + break; > + > + if (!kref_get_unless_zero(&ppgtt->ref)) > + continue; > + > + /* > + * This ppgtt may have be reallocated between > + * the read and the kref, and reassigned to a third > + * context. In order to avoid inadvertent sharing > + * of this ppgtt with that third context (and not > + * src), we have to confirm that we have the same > + * ppgtt after passing through the strong memory > + * barrier implied by a successful > + * kref_get_unless_zero(). > + * > + * Once we have acquired the current ppgtt of src, > + * we no longer care if it is released from src, as > + * it cannot be reallocated elsewhere. > + */ > + > + if (ppgtt == READ_ONCE(src->ppgtt)) > + break; > + > + i915_ppgtt_put(ppgtt); > + } while (1); > + rcu_read_unlock(); > + > + if (ppgtt) { > + __assign_ppgtt(dst, ppgtt); > + i915_ppgtt_put(ppgtt); > + } > + > + return 0; > +} > + > +static int create_clone(struct i915_user_extension __user *ext, void *data) > +{ > + static int (* const fn[])(struct i915_gem_context *dst, > + struct i915_gem_context *src) = { > +#define MAP(x, y) [ilog2(I915_CONTEXT_CLONE_##x)] = y > + MAP(ENGINES, clone_engines), > + MAP(FLAGS, clone_flags), > + MAP(SCHEDATTR, clone_schedattr), > + MAP(SSEU, clone_sseu), > + MAP(TIMELINE, clone_timeline), > + MAP(VM, clone_vm), > +#undef MAP > + }; > + struct drm_i915_gem_context_create_ext_clone local; > + const struct create_ext *arg = data; > + struct i915_gem_context *dst = arg->ctx; > + struct i915_gem_context *src; > + int err, bit; > + > + if (copy_from_user(&local, ext, sizeof(local))) > + return -EFAULT; > + > + BUILD_BUG_ON(GENMASK(BITS_PER_TYPE(local.flags) - 1, ARRAY_SIZE(fn)) != > + I915_CONTEXT_CLONE_UNKNOWN); > + > + if (local.flags & I915_CONTEXT_CLONE_UNKNOWN) > + return -EINVAL; > + > + if (local.rsvd) > + return -EINVAL; > + > + rcu_read_lock(); > + src = __i915_gem_context_lookup_rcu(arg->fpriv, local.clone_id); > + rcu_read_unlock(); > + if (!src) > + return -ENOENT; > + > + GEM_BUG_ON(src == dst); > + > + for (bit = 0; bit < ARRAY_SIZE(fn); bit++) { > + if (!(local.flags & BIT(bit))) > + continue; > + > + err = fn[bit](dst, src); > + if (err) > + return err; > + } > + > + return 0; > +} > + > static const i915_user_extension_fn create_extensions[] = { > [I915_CONTEXT_CREATE_EXT_SETPARAM] = create_setparam, > + [I915_CONTEXT_CREATE_EXT_CLONE] = create_clone, > }; > > static bool client_is_banned(struct drm_i915_file_private *file_priv) > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 7aef672ab3c7..7694113362d4 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -1623,6 +1623,21 @@ struct drm_i915_gem_context_create_ext_setparam { > struct drm_i915_gem_context_param param; > }; > > +struct drm_i915_gem_context_create_ext_clone { > +#define I915_CONTEXT_CREATE_EXT_CLONE 1 > + struct i915_user_extension base; > + __u32 clone_id; > + __u32 flags; > +#define I915_CONTEXT_CLONE_ENGINES (1u << 0) > +#define I915_CONTEXT_CLONE_FLAGS (1u << 1) > +#define I915_CONTEXT_CLONE_SCHEDATTR (1u << 2) > +#define I915_CONTEXT_CLONE_SSEU (1u << 3) > +#define I915_CONTEXT_CLONE_TIMELINE (1u << 4) > +#define I915_CONTEXT_CLONE_VM (1u << 5) > +#define I915_CONTEXT_CLONE_UNKNOWN -(I915_CONTEXT_CLONE_VM << 1) Have we talked about whether CLONE_UNKNOWN makes sense or instead we say "-1" is CLONE_EVERYTHING? Currently the latter sounds more usable and easier to maintain in userspace to me. Regards, Tvrtko > + __u64 rsvd; > +}; > + > struct drm_i915_gem_context_destroy { > __u32 ctx_id; > __u32 pad; >
Quoting Tvrtko Ursulin (2019-04-15 13:56:10) > > On 12/04/2019 09:53, Chris Wilson wrote: > > +struct drm_i915_gem_context_create_ext_clone { > > +#define I915_CONTEXT_CREATE_EXT_CLONE 1 > > + struct i915_user_extension base; > > + __u32 clone_id; > > + __u32 flags; > > +#define I915_CONTEXT_CLONE_ENGINES (1u << 0) > > +#define I915_CONTEXT_CLONE_FLAGS (1u << 1) > > +#define I915_CONTEXT_CLONE_SCHEDATTR (1u << 2) > > +#define I915_CONTEXT_CLONE_SSEU (1u << 3) > > +#define I915_CONTEXT_CLONE_TIMELINE (1u << 4) > > +#define I915_CONTEXT_CLONE_VM (1u << 5) > > +#define I915_CONTEXT_CLONE_UNKNOWN -(I915_CONTEXT_CLONE_VM << 1) > > Have we talked about whether CLONE_UNKNOWN makes sense or instead we say > "-1" is CLONE_EVERYTHING? Currently the latter sounds more usable and > easier to maintain in userspace to me. We haven't talked. I went for you have to opt in explicitly for the bits you want cloned so that when we extend it in future, old userspace suddenly doesn't start sharing more than they were previously. Oversharing, I felt, was an accident waiting to happen. Now you can argue that if they simply use ~CLONE_UNKNOWN they get exactly the same change if they recompile... But old binaries retain their existing behaviour if the interface changes. -Chris
On 17/04/2019 08:53, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-04-15 13:56:10) >> >> On 12/04/2019 09:53, Chris Wilson wrote: >>> +struct drm_i915_gem_context_create_ext_clone { >>> +#define I915_CONTEXT_CREATE_EXT_CLONE 1 >>> + struct i915_user_extension base; >>> + __u32 clone_id; >>> + __u32 flags; >>> +#define I915_CONTEXT_CLONE_ENGINES (1u << 0) >>> +#define I915_CONTEXT_CLONE_FLAGS (1u << 1) >>> +#define I915_CONTEXT_CLONE_SCHEDATTR (1u << 2) >>> +#define I915_CONTEXT_CLONE_SSEU (1u << 3) >>> +#define I915_CONTEXT_CLONE_TIMELINE (1u << 4) >>> +#define I915_CONTEXT_CLONE_VM (1u << 5) >>> +#define I915_CONTEXT_CLONE_UNKNOWN -(I915_CONTEXT_CLONE_VM << 1) >> >> Have we talked about whether CLONE_UNKNOWN makes sense or instead we say >> "-1" is CLONE_EVERYTHING? Currently the latter sounds more usable and >> easier to maintain in userspace to me. > > We haven't talked. I went for you have to opt in explicitly for the bits > you want cloned so that when we extend it in future, old userspace > suddenly doesn't start sharing more than they were previously. > Oversharing, I felt, was an accident waiting to happen. > > Now you can argue that if they simply use ~CLONE_UNKNOWN they get > exactly the same change if they recompile... But old binaries retain > their existing behaviour if the interface changes. The thing I was thinking about was if context gets some new functional category which can be cloned, but old binaries would therefore not clone it and as such end up as non-functional or degraded context. I don't have an example in mind so just hypothetical. I went to remind myself what clone(2) does and it matches your idea. So that makes sense. We just have to keep in mind not to add a new category which makes the cloned context has surprising behaviour. Patch did look fine so: Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index ad9920f703c6..27c22df59475 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -1685,8 +1685,215 @@ static int create_setparam(struct i915_user_extension __user *ext, void *data) return ctx_setparam(arg->fpriv, arg->ctx, &local.param); } +static int clone_engines(struct i915_gem_context *dst, + struct i915_gem_context *src) +{ + struct i915_gem_engines *e, *clone; + bool user_engines; + unsigned long n; + + e = i915_gem_context_engine_list_lock(src); + + clone = kmalloc(struct_size(e, engines, e->num_engines), GFP_KERNEL); + if (!clone) + goto err_unlock; + + clone->i915 = dst->i915; + for (n = 0; n < e->num_engines; n++) { + if (!e->engines[n]) { + clone->engines[n] = NULL; + continue; + } + + clone->engines[n] = + intel_context_create(dst, e->engines[n]->engine); + if (!clone->engines[n]) { + __free_engines(clone, n); + goto err_unlock; + } + } + clone->num_engines = n; + + user_engines = i915_gem_context_user_engines(src); + i915_gem_context_engine_list_unlock(src); + + free_engines(dst->engines); + RCU_INIT_POINTER(dst->engines, clone); + if (user_engines) + i915_gem_context_set_user_engines(dst); + else + i915_gem_context_clear_user_engines(dst); + return 0; + +err_unlock: + i915_gem_context_engine_list_unlock(src); + return -ENOMEM; +} + +static int clone_flags(struct i915_gem_context *dst, + struct i915_gem_context *src) +{ + dst->user_flags = src->user_flags; + return 0; +} + +static int clone_schedattr(struct i915_gem_context *dst, + struct i915_gem_context *src) +{ + dst->sched = src->sched; + return 0; +} + +static int clone_sseu(struct i915_gem_context *dst, + struct i915_gem_context *src) +{ + struct i915_gem_engines *e, *clone; + unsigned long n; + int err; + + clone = dst->engines; /* no locking required; sole access */ + e = i915_gem_context_engine_list_lock(src); + if (e->num_engines != clone->num_engines) { + err = -EINVAL; + goto unlock; + } + + for (n = 0; n < e->num_engines; n++) { + struct intel_context *ce = e->engines[n]; + + if (clone->engines[n]->engine->class != ce->engine->class) { + /* Must have compatible engine maps! */ + err = -EINVAL; + goto unlock; + } + + /* serialises with set_sseu */ + err = intel_context_lock_pinned(ce); + if (err) + goto unlock; + + clone->engines[n]->sseu = ce->sseu; + intel_context_unlock_pinned(ce); + } + + err = 0; +unlock: + i915_gem_context_engine_list_unlock(src); + return err; +} + +static int clone_timeline(struct i915_gem_context *dst, + struct i915_gem_context *src) +{ + if (src->timeline) { + GEM_BUG_ON(src->timeline == dst->timeline); + + if (dst->timeline) + i915_timeline_put(dst->timeline); + dst->timeline = i915_timeline_get(src->timeline); + } + + return 0; +} + +static int clone_vm(struct i915_gem_context *dst, + struct i915_gem_context *src) +{ + struct i915_hw_ppgtt *ppgtt; + + rcu_read_lock(); + do { + ppgtt = READ_ONCE(src->ppgtt); + if (!ppgtt) + break; + + if (!kref_get_unless_zero(&ppgtt->ref)) + continue; + + /* + * This ppgtt may have be reallocated between + * the read and the kref, and reassigned to a third + * context. In order to avoid inadvertent sharing + * of this ppgtt with that third context (and not + * src), we have to confirm that we have the same + * ppgtt after passing through the strong memory + * barrier implied by a successful + * kref_get_unless_zero(). + * + * Once we have acquired the current ppgtt of src, + * we no longer care if it is released from src, as + * it cannot be reallocated elsewhere. + */ + + if (ppgtt == READ_ONCE(src->ppgtt)) + break; + + i915_ppgtt_put(ppgtt); + } while (1); + rcu_read_unlock(); + + if (ppgtt) { + __assign_ppgtt(dst, ppgtt); + i915_ppgtt_put(ppgtt); + } + + return 0; +} + +static int create_clone(struct i915_user_extension __user *ext, void *data) +{ + static int (* const fn[])(struct i915_gem_context *dst, + struct i915_gem_context *src) = { +#define MAP(x, y) [ilog2(I915_CONTEXT_CLONE_##x)] = y + MAP(ENGINES, clone_engines), + MAP(FLAGS, clone_flags), + MAP(SCHEDATTR, clone_schedattr), + MAP(SSEU, clone_sseu), + MAP(TIMELINE, clone_timeline), + MAP(VM, clone_vm), +#undef MAP + }; + struct drm_i915_gem_context_create_ext_clone local; + const struct create_ext *arg = data; + struct i915_gem_context *dst = arg->ctx; + struct i915_gem_context *src; + int err, bit; + + if (copy_from_user(&local, ext, sizeof(local))) + return -EFAULT; + + BUILD_BUG_ON(GENMASK(BITS_PER_TYPE(local.flags) - 1, ARRAY_SIZE(fn)) != + I915_CONTEXT_CLONE_UNKNOWN); + + if (local.flags & I915_CONTEXT_CLONE_UNKNOWN) + return -EINVAL; + + if (local.rsvd) + return -EINVAL; + + rcu_read_lock(); + src = __i915_gem_context_lookup_rcu(arg->fpriv, local.clone_id); + rcu_read_unlock(); + if (!src) + return -ENOENT; + + GEM_BUG_ON(src == dst); + + for (bit = 0; bit < ARRAY_SIZE(fn); bit++) { + if (!(local.flags & BIT(bit))) + continue; + + err = fn[bit](dst, src); + if (err) + return err; + } + + return 0; +} + static const i915_user_extension_fn create_extensions[] = { [I915_CONTEXT_CREATE_EXT_SETPARAM] = create_setparam, + [I915_CONTEXT_CREATE_EXT_CLONE] = create_clone, }; static bool client_is_banned(struct drm_i915_file_private *file_priv) diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 7aef672ab3c7..7694113362d4 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -1623,6 +1623,21 @@ struct drm_i915_gem_context_create_ext_setparam { struct drm_i915_gem_context_param param; }; +struct drm_i915_gem_context_create_ext_clone { +#define I915_CONTEXT_CREATE_EXT_CLONE 1 + struct i915_user_extension base; + __u32 clone_id; + __u32 flags; +#define I915_CONTEXT_CLONE_ENGINES (1u << 0) +#define I915_CONTEXT_CLONE_FLAGS (1u << 1) +#define I915_CONTEXT_CLONE_SCHEDATTR (1u << 2) +#define I915_CONTEXT_CLONE_SSEU (1u << 3) +#define I915_CONTEXT_CLONE_TIMELINE (1u << 4) +#define I915_CONTEXT_CLONE_VM (1u << 5) +#define I915_CONTEXT_CLONE_UNKNOWN -(I915_CONTEXT_CLONE_VM << 1) + __u64 rsvd; +}; + struct drm_i915_gem_context_destroy { __u32 ctx_id; __u32 pad;
A usecase arose out of handling context recovery in mesa, whereby they wish to recreate a context with fresh logical state but preserving all other details of the original. Currently, they create a new context and iterate over which bits they want to copy across, but it would much more convenient if they were able to just pass in a target context to clone during creation. This essentially extends the setparam during creation to pull the details from a target context instead of the user supplied parameters. The ideal here is that we don't expose control over anything more than can be obtained via CONTEXT_PARAM. That is userspace retains explicit control over all features, and this api is just convenience. For example, you could replace struct context_param p = { .param = CONTEXT_PARAM_VM }; param.ctx_id = old_id; gem_context_get_param(&p.param); new_id = gem_context_create(); param.ctx_id = new_id; gem_context_set_param(&p.param); gem_vm_destroy(param.value); /* drop the ref to VM_ID handle */ with struct create_ext_param p = { { .name = CONTEXT_CREATE_CLONE }, .clone_id = old_id, .flags = CLONE_FLAGS_VM } new_id = gem_context_create_ext(&p); and not have to worry about stray namespace pollution etc. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem_context.c | 207 ++++++++++++++++++++++++ include/uapi/drm/i915_drm.h | 15 ++ 2 files changed, 222 insertions(+)