[Intel-gfx] [PATCH 07/13] drm/i915: Allow userspace to clone contexts on creation

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-03-08 16:13:56)
> 
> On 08/03/2019 14:12, 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.
> 
> This one is not used by media so it will likely have to find a separate 
> route upstream.

Eh?

However, I do think there is one quite handy usecase:

i915_query -> engines[]
gem_context_set_param(0 /* default context */, ENGINES, engines[]);

Then whenever you want a context,
ctx = gem_context_clone(0, CLONE_ENGINES);
so that you don't have to store your query results, or build a param for
every create.

> > +static int create_clone(struct i915_user_extension __user *ext, void *data)
> > +{
> > +     struct drm_i915_gem_context_create_ext_clone local;
> > +     struct i915_gem_context *dst = data;
> > +     struct i915_gem_context *src;
> > +     int err;
> > +
> > +     if (copy_from_user(&local, ext, sizeof(local)))
> > +             return -EFAULT;
> > +
> > +     if (local.flags & I915_CONTEXT_CLONE_UNKNOWN)
> > +             return -EINVAL;
> > +
> > +     if (local.rsvd)
> > +             return -EINVAL;
> > +
> > +     if (local.clone == dst->user_handle) /* good guess! denied. */
> > +             return -ENOENT;
> 
> :) Good one, but put a more obvious comment like "Cannot clone itself!".
> 
> > +
> > +     rcu_read_lock();
> > +     src = __i915_gem_context_lookup_rcu(dst->file_priv, local.clone);
> > +     rcu_read_unlock();
> > +     if (!src)
> > +             return -ENOENT;
> > +
> > +     GEM_BUG_ON(src == dst);
> > +
> > +     if (local.flags & I915_CONTEXT_CLONE_FLAGS)
> > +             dst->user_flags = src->user_flags;
> > +
> > +     if (local.flags & I915_CONTEXT_CLONE_SCHED)
> > +             dst->sched = src->sched;
> > +
> > +     if (local.flags & I915_CONTEXT_CLONE_SSEU) {
> > +             err = clone_sseu(dst, src);
> > +             if (err)
> > +                     return err;
> > +     }
> > +
> > +     if (local.flags & I915_CONTEXT_CLONE_TIMELINE && src->timeline) {
> 
> Do we want to error out if no timeline and cloning was requested?

No. Because a clone of that situation is no common timeline. Basically
it allows one to ask "give me everything that can be cloned" without
having to worry about what you setup beforehand.

> 
> > +             if (dst->timeline)
> > +                     i915_timeline_put(dst->timeline);
> > +             dst->timeline = i915_timeline_get(src->timeline);
> 
> What prevents a different thread from changing either context in 
> parallel and making reference counting go bad?

It should be that userspace isn't allowed to access dst before the
constructor returns. As hinted earlier, we insert ourselves into the idr
too early. So what should be impossible... isn't quite imposibble.

src->timeline is unchangeable so we don't have to worry about
serialisation there (yet, haven't found anyone to pitch first class
timelines to).

> > +     }
> > +
> > +     if (local.flags & I915_CONTEXT_CLONE_VM && src->ppgtt) {
> 
> Also fail if impossible was requested?

Nope. Because !src->ppgtt implies that dst is already using the same
ppgtt (the aliasing ppgtt).

> > +             GEM_BUG_ON(dst->ppgtt == src->ppgtt);
> 
> Hm... what prevents this? Set_vm extension followed by clone could 
> trigger it I think.

True.

> > +
> > +             if (dst->ppgtt)
> > +                     i915_ppgtt_put(dst->ppgtt);
> > +
> > +             dst->ppgtt = i915_ppgtt_get(src->ppgtt);
> > +             i915_ppgtt_open(dst->ppgtt);
> 
> Also some locking is needed I think to make the exchange atomic.

None required for dest, but we should serialise src (well and you'll
love this, since this is under RCU).

> Could use __assign_ppgtt?

Ah. I knew there should be something.

> >   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 007d77ff7295..50d154954d5f 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -1579,6 +1579,20 @@ struct drm_i915_gem_context_create_ext_setparam {
> >       struct drm_i915_gem_context_param setparam;
> >   };
> >   
> > +struct drm_i915_gem_context_create_ext_clone {
> > +#define I915_CONTEXT_CREATE_EXT_CLONE 1
> > +     struct i915_user_extension base;
> > +     __u32 clone;
> 
> id, clone_id, source_id?

What is with _id? I think ctx_id is an abomination. Of the selection,
source_id. Or share_id. So clone_id.
-Chris