Re: [PATCH] drm/i915: Allow userptr backchannel for passing aroung GTT mappings

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

Hi,

Typo in subject, then below.

On 04/02/2015 04:04 PM, Chris Wilson wrote:
> Once userptr becomes part of client API, it is almost a certainly that
> eventually someone will try to create a new object from a mapping of
> another client object, e.g.
>
> new = vaImport(vaMap(old, &size), size);
>
> (using a hypothethical API, not meaning to pick on anyone!)
>
> Since this is actually fairly safe to implement and to allow (since it
> is within a single process space and the memory access passes the
> standard permissions test) let us not limit the Client possibilities.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Gwenole Beauchesne <gwenole.beauchesne@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_userptr.c | 46 ++++++++++++++++++++++++++++++---
>   1 file changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index d96276caab49..8031ebe424fd 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -750,6 +750,35 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
>   	.release = i915_gem_userptr_release,
>   };
>
> +static struct drm_i915_gem_object *
> +find_object_from_vma(struct drm_device *dev,
> +		     struct drm_i915_gem_userptr *args)
> +{
> +	struct drm_i915_gem_object *obj = NULL;
> +	struct vm_area_struct *vma;
> +
> +	down_read(&current->mm->mmap_sem);
> +	vma = find_vma(current->mm, args->user_ptr);
> +	if (vma == NULL)
> +		goto out;
> +
> +	if (vma->vm_ops != dev->driver->gem_vm_ops)
> +		goto out;
> +
> +	if (vma->vm_start != args->user_ptr ||
> +	    vma->vm_end != args->user_ptr + args->user_size) {
> +		obj = ERR_PTR(-EINVAL);
> +		goto out;
> +	}
> +
> +	obj = to_intel_bo(vma->vm_private_data);
> +	drm_gem_object_reference(obj);

Hm, can't this race with last unreference in general, and with cleanup 
worker with userptr objects?

> +
> +out:
> +	up_read(&current->mm->mmap_sem);
> +	return obj;
> +}
> +
>   /**
>    * Creates a new mm object that wraps some normal memory from the process
>    * context - user memory.
> @@ -757,8 +786,11 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
>    * We impose several restrictions upon the memory being mapped
>    * into the GPU.
>    * 1. It must be page aligned (both start/end addresses, i.e ptr and size).
> - * 2. It must be normal system memory, not a pointer into another map of IO
> - *    space (e.g. it must not be a GTT mmapping of another object).
> + * 2. It must either be:
> + *    a) normal system memory, not a pointer into another map of IO
> + *       space (e.g. it must not be part of a GTT mmapping of another object).
> + *    b) a pointer to the complete GTT mmap of another object in your
> + *       address space.
>    * 3. We only allow a bo as large as we could in theory map into the GTT,
>    *    that is we limit the size to the total size of the GTT.
>    * 4. The bo is marked as being snoopable. The backing pages are left
> @@ -812,6 +844,14 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void *data, struct drm_file *file
>   		return -ENODEV;
>   	}
>
> +	obj = find_object_from_vma(dev, args);
> +	if (obj) {
> +		if (IS_ERR(obj))
> +			return PTR_ERR(obj);
> +		else
> +			goto out;
> +	}
> +
>   	obj = i915_gem_object_alloc(dev);
>   	if (obj == NULL)
>   		return -ENOMEM;
> @@ -833,7 +873,7 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void *data, struct drm_file *file
>   	if (ret == 0)
>   		ret = i915_gem_userptr_init__mmu_notifier(obj, args->flags);
>   	if (ret == 0)
> -		ret = drm_gem_handle_create(file, &obj->base, &handle);
> +out:		 ret = drm_gem_handle_create(file, &obj->base, &handle);
>
>   	/* drop reference from allocate - handle holds it now */
>   	drm_gem_object_unreference_unlocked(&obj->base);

Thing I don't like is how the user of this has no idea what kind of 
object it "imported". Maybe it doesn't matter, hm. Need to think about 
it more.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx