[Intel-gfx] [PATCH 1/2] drm/i915/userptr: Probe existence of backing struct pages upon creation

[Tvrtko Ursulin]

On 08/11/2017 18:51, Chris Wilson wrote:
> Jason Ekstrand requested a more efficient method than userptr+set-domain
> to determine if the userptr object was backed by a complete set of pages
> upon creation. To be more efficient than simply populating the userptr
> using get_user_pages() (as done by the call to set-domain or execbuf),
> we can walk the tree of vm_area_struct and check for gaps or vma not
> backed by struct page (VM_PFNMAP). The question is how to handle
> VM_MIXEDMAP which may be either struct page or pfn backed...

First reaction is that it sounds like a bad idea. Well maybe not bad, 
but a bit questionable since if the userspace doesn't know, then there 
are no guarantees the result of probe will remain valid until the time 
to actually instantiate the backing store comes.

The populate idea from the next patch sounds okay on the other hand. So 
is there an use case where extremely controlling userspace would benefit 
from probe rather than just using populate?

Regards,

Tvrtko

> 
> Testcase: igt/gem_userptr_blits/probe
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Michał Winiarski <michal.winiarski at intel.com>
> Cc: Jason Ekstrand <jason at jlekstrand.net>
> ---
>   drivers/gpu/drm/i915/i915_gem_userptr.c | 38 +++++++++++++++++++++++++++++++++
>   include/uapi/drm/i915_drm.h             |  5 +++--
>   2 files changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index e26b23171b56..dbc5818dd28b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -719,6 +719,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
>   	.release = i915_gem_userptr_release,
>   };
>   
> +static int
> +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
> +{
> +	const unsigned long end = addr + len;
> +	struct vm_area_struct *vma;
> +	int ret = -EFAULT;
> +
> +	down_read(&mm->mmap_sem);
> +	for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
> +		if (vma->vm_start > addr)
> +			break;
> +
> +		if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
> +			break;
> +
> +		if (vma->vm_end >= end) {
> +			ret = 0;
> +			break;
> +		}
> +
> +		addr = vma->vm_end;
> +	}
> +	up_read(&mm->mmap_sem);
> +
> +	return ret;
> +}
> +
>   /**
>    * Creates a new mm object that wraps some normal memory from the process
>    * context - user memory.
> @@ -771,6 +798,7 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void *data, struct drm_file *file
>   	}
>   
>   	if (args->flags & ~(I915_USERPTR_READ_ONLY |
> +			    I915_USERPTR_PROBE |
>   			    I915_USERPTR_UNSYNCHRONIZED))
>   		return -EINVAL;
>   
> @@ -788,6 +816,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void *data, struct drm_file *file
>   		return -ENODEV;
>   	}
>   
> +	if (args->flags & I915_USERPTR_PROBE) {
> +		/*
> +		 * Check that the range pointed to represents real struct
> +		 * pages and not iomappings (at this moment in time!)
> +		 */
> +		ret = probe_range(current->mm, args->user_ptr, args->user_size);
> +		if (ret)
> +			return ret;
> +	}
> +
>   	obj = i915_gem_object_alloc(dev_priv);
>   	if (obj == NULL)
>   		return -ENOMEM;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index ac3c6503ca27..392ede2ca63e 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1352,8 +1352,9 @@ struct drm_i915_gem_userptr {
>   	__u64 user_ptr;
>   	__u64 user_size;
>   	__u32 flags;
> -#define I915_USERPTR_READ_ONLY 0x1
> -#define I915_USERPTR_UNSYNCHRONIZED 0x80000000
> +#define I915_USERPTR_READ_ONLY		0x1
> +#define I915_USERPTR_PROBE		0x2
> +#define I915_USERPTR_UNSYNCHRONIZED	0x80000000
>   	/**
>   	 * Returned handle for the object.
>   	 *
>