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

[Tvrtko Ursulin]

On 19/03/2019 11:57, 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.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem_context.c | 154 ++++++++++++++++++++++++
>   include/uapi/drm/i915_drm.h             |  14 +++
>   2 files changed, 168 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index fc1f64e19507..f36648329074 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -1500,8 +1500,162 @@ static int create_setparam(struct i915_user_extension __user *ext, void *data)
>   	return ctx_setparam(arg->ctx, &local.param);
>   }
>   
> +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)
> +{
> +	const struct intel_sseu default_sseu =
> +		intel_device_default_sseu(dst->i915);
> +	struct intel_engine_cs *engine;
> +	enum intel_engine_id id;
> +
> +	for_each_engine(engine, dst->i915, id) {

Hm in the load balancing patch this needs to be extended so the veng ce 
is also handled here.

Possibly even when adding engine map the loop needs to iterate the map 
and not for_each_engine?

> +		struct intel_context *ce;
> +		struct intel_sseu sseu;
> +
> +		ce = intel_context_lookup(src, engine);
> +		if (!ce)
> +			continue;
> +
> +		sseu = ce->sseu;
> +		if (!memcmp(&sseu, &default_sseu, sizeof(sseu)))

Could memcmp against &ce->sseu directly and keep src_ce and dst_ce so 
you can copy over without a temporary copy on stack?

> +			continue;
> +
> +		ce = intel_context_pin_lock(dst, engine);
> +		if (IS_ERR(ce))
> +			return PTR_ERR(ce);
> +
> +		ce->sseu = sseu;
> +		intel_context_pin_unlock(ce);
> +	}
> +
> +	return 0;
> +}
> +
> +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();

I still have the same problem. What if you added here:

GEM_BUG_ON(ppgtt != READ_ONCE(src->ppgtt));

Could it trigger? If so what is the point in the last check in the loop 
above?

> +
> +	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(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 9999f7d6a5a9..a5bdb86858f6 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1581,6 +1581,20 @@ 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_FLAGS	(1u << 0)
> +#define I915_CONTEXT_CLONE_SCHEDATTR	(1u << 1)
> +#define I915_CONTEXT_CLONE_SSEU		(1u << 2)
> +#define I915_CONTEXT_CLONE_TIMELINE	(1u << 3)
> +#define I915_CONTEXT_CLONE_VM		(1u << 4)
> +#define I915_CONTEXT_CLONE_UNKNOWN -(I915_CONTEXT_CLONE_VM << 1)
> +	__u64 rsvd;
> +};
> +
>   struct drm_i915_gem_context_destroy {
>   	__u32 ctx_id;
>   	__u32 pad;
> 

Regards,

Tvrtko