Re: [Intel-gfx] [PATCH 4/5] drm/i915: Drop the CONTEXT_CLONE API

On 17/03/2021 23:40, Jason Ekstrand wrote:

It's never been used by any real userspace. It's used by IGT as a short-cut for sharing VMs and other stuff between contexts.

I haven't checked if userspace uses this so assuming that is fine, the only thing that remains is preparing the IGTs for the transition which cannot be avoided.

Regards,

Tvrtko

Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Tvrtko Ursulin tvrtko.ursulin@intel.com

---

drivers/gpu/drm/i915/gem/i915_gem_context.c | 217 +------------------- include/uapi/drm/i915_drm.h | 16 +- 2 files changed, 6 insertions(+), 227 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index ca37d93ef5e78..92256f4e50764 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -2103,225 +2103,14 @@ static int create_setparam(struct i915_user_extension __user *ext, void *data) return ctx_setparam(arg->fpriv, arg->ctx, &local.param); }

-static int copy_ring_size(struct intel_context *dst,

• 	  struct intel_context *src)
  

+static int invalid_ext(struct i915_user_extension __user *ext, void *data) {

• long sz;
• sz = intel_context_get_ring_size(src);
• if (sz < 0)

• return sz;
  

• return intel_context_set_ring_size(dst, sz);

-}

-static int clone_engines(struct i915_gem_context *dst,

• 	 struct i915_gem_context *src)
  

-{

• struct i915_gem_engines *clone, *e;
• bool user_engines;
• unsigned long n;
• e = __context_engines_await(src, &user_engines);
• if (!e)

• return -ENOENT;
  

• clone = alloc_engines(e->num_engines);
• if (!clone)

• goto err_unlock;
  

• for (n = 0; n < e->num_engines; n++) {

• struct intel_engine_cs *engine;
  

• if (!e->engines[n]) {
  

• 	clone->engines[n] = NULL;
  

• 	continue;
  

• }
  

• engine = e->engines[n]->engine;
  

• /*
  

•  * Virtual engines are singletons; they can only exist
  

•  * inside a single context, because they embed their
  

•  * HW context... As each virtual context implies a single
  

•  * timeline (each engine can only dequeue a single request
  

•  * at any time), it would be surprising for two contexts
  

•  * to use the same engine. So let's create a copy of
  

•  * the virtual engine instead.
  

•  */
  

• if (intel_engine_is_virtual(engine))
  

• 	clone->engines[n] =
  

• 		intel_execlists_clone_virtual(engine);
  

• else
  

• 	clone->engines[n] = intel_context_create(engine);
  

• if (IS_ERR_OR_NULL(clone->engines[n])) {
  

• 	__free_engines(clone, n);
  

• 	goto err_unlock;
  

• }
  

• intel_context_set_gem(clone->engines[n], dst);
  

• /* Copy across the preferred ringsize */
  

• if (copy_ring_size(clone->engines[n], e->engines[n])) {
  

• 	__free_engines(clone, n + 1);
  

• 	goto err_unlock;
  

• }
  

• }
• clone->num_engines = n;
• i915_sw_fence_complete(&e->fence);
• /* Serialised by constructor */
• engines_idle_release(dst, rcu_replace_pointer(dst->engines, clone, 1));
• if (user_engines)

• i915_gem_context_set_user_engines(dst);
  

• else

• i915_gem_context_clear_user_engines(dst);
  

• return 0;

-err_unlock:

• i915_sw_fence_complete(&e->fence);
• 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 = i915_gem_context_lock_engines(src);
• struct i915_gem_engines *clone;
• unsigned long n;
• int err;
• /* no locking required; sole access under constructor*/
• clone = __context_engines_static(dst);
• 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_unlock_engines(src);
• return err;

-}

-static int clone_timeline(struct i915_gem_context *dst,

• 	  struct i915_gem_context *src)
  

-{

• if (src->timeline)

• __assign_timeline(dst, src->timeline);
  

• return 0;

-}

-static int clone_vm(struct i915_gem_context *dst,

•     struct i915_gem_context *src)
  

-{

• struct i915_address_space *vm;
• int err = 0;
• if (!rcu_access_pointer(src->vm))

• return 0;
  

• rcu_read_lock();
• vm = context_get_vm_rcu(src);
• rcu_read_unlock();
• if (!mutex_lock_interruptible(&dst->mutex)) {

• __assign_ppgtt(dst, vm);
  

• mutex_unlock(&dst->mutex);
  

• } else {

• err = -EINTR;
  

• }
• i915_vm_put(vm);
• return err;

-}

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

• return -EINVAL; }

  static const i915_user_extension_fn create_extensions[] = { [I915_CONTEXT_CREATE_EXT_SETPARAM] = create_setparam,

• [I915_CONTEXT_CREATE_EXT_CLONE] = create_clone,

• [I915_CONTEXT_CREATE_EXT_CLONE] = invalid_ext, };

  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 ddc47bbf48b6d..0ed1b88afedd5 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -1853,20 +1853,10 @@ struct drm_i915_gem_context_create_ext_setparam { struct drm_i915_gem_context_param param; };

-struct drm_i915_gem_context_create_ext_clone { +/* This API has been removed. On the off chance someone somewhere has

• • attempted to use it, never re-use this extension number.
• */ #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;