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

[Tvrtko Ursulin]

On 13/03/2019 14:43, 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.

No change log again.

> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem_context.c | 103 ++++++++++++++++++++++++
>   include/uapi/drm/i915_drm.h             |  14 ++++
>   2 files changed, 117 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 4e4b0b5c4be0..bac548584091 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -1488,8 +1488,111 @@ static int create_setparam(struct i915_user_extension __user *ext, void *data)
>   	return ctx_setparam(data, &local.setparam);
>   }
>   
> +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) {
> +		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)))
> +			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 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_id == dst->user_handle) /* good guess! denied. */

As I said it's a funny comment but please put in something more obvious.

> +		return -ENOENT;
> +
> +	rcu_read_lock();
> +	src = __i915_gem_context_lookup_rcu(dst->file_priv, local.clone_id);
> +	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) {
> +		if (dst->timeline)
> +			i915_timeline_put(dst->timeline);
> +		dst->timeline = i915_timeline_get(src->timeline);
> +	}
> +
> +	if (local.flags & I915_CONTEXT_CLONE_VM) {
> +		struct i915_hw_ppgtt *ppgtt;
> +
> +		do {
> +			ppgtt = READ_ONCE(src->ppgtt);
> +			if (!ppgtt)
> +				break;
> +
> +			if (!kref_get_unless_zero(&ppgtt->ref))
> +				continue;
> +
> +			if (ppgtt == READ_ONCE(src->ppgtt))
> +				break;

This loop needs a comment. For instance I don't understand what is this 
line for. Firstly, we have a reference on ppgtt, so what do we care if 
the source context in the meantime changed it? Secondly, even though it 
matches now, why would it still match when we exit the loop?

> +
> +			i915_ppgtt_put(ppgtt);
> +		} while (1);
> +
> +		if (ppgtt) {
> +			__assign_ppgtt(dst, ppgtt);
> +			i915_ppgtt_put(ppgtt);
> +		}
> +	}
> +
> +	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 5206d0006043..9714520f43da 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 setparam;
>   };
>   
> +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_SCHED	(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;
> +};

Some uapi docs are needed.

> +
>   struct drm_i915_gem_context_destroy {
>   	__u32 ctx_id;
>   	__u32 pad;
> 

Regards,

Tvrtko