[PATCH 3/4] drm/i915/userptr: Probe existence of backing struct pages upon creation

Fri Jul 16 14:36:10 UTC 2021

On 15/07/2021 11:15, Matthew Auld wrote:
> From: Chris Wilson <chris at chris-wilson.co.uk>
> 
> 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...
> 
> With discrete are going to drop support for set_domain(), so offering a
> way to probe the pages, without having to resort to dummy batches has
> been requested.
> 
> v2:
> - add new query param for the PROPBE flag, so userspace can easily

PROBE

>    check if the kernel supports it(Jason).
> - use mmap_read_{lock, unlock}.
> - add some kernel-doc.
> 
> Testcase: igt/gem_userptr_blits/probe
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Cc: Jordan Justen <jordan.l.justen at intel.com>
> Cc: Kenneth Graunke <kenneth at whitecape.org>
> Cc: Jason Ekstrand <jason at jlekstrand.net>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Ramalingam C <ramalingam.c at intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++-
>   drivers/gpu/drm/i915/i915_getparam.c        |  3 ++
>   include/uapi/drm/i915_drm.h                 | 18 ++++++++++
>   3 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index 56edfeff8c02..fd6880328596 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
>   
>   #endif
>   
> +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;
> +
> +	mmap_read_lock(mm);
> +	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;
> +	}
> +	mmap_read_unlock(mm);

Logic here looks good to me.

> +
> +	return ret;
> +}
> +
>   /*
>    * Creates a new mm object that wraps some normal memory from the process
>    * context - user memory.
> @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
>   	}
>   
>   	if (args->flags & ~(I915_USERPTR_READ_ONLY |
> -			    I915_USERPTR_UNSYNCHRONIZED))
> +			    I915_USERPTR_UNSYNCHRONIZED |
> +			    I915_USERPTR_PROBE))
>   		return -EINVAL;
>   
>   	if (i915_gem_object_size_2big(args->user_size))
> @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
>   			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;
> +	}
> +
>   #ifdef CONFIG_MMU_NOTIFIER
>   	obj = i915_gem_object_alloc();
>   	if (obj == NULL)
> diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c
> index 24e18219eb50..d6d2e1a10d14 100644
> --- a/drivers/gpu/drm/i915/i915_getparam.c
> +++ b/drivers/gpu/drm/i915/i915_getparam.c
> @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
>   	case I915_PARAM_PERF_REVISION:
>   		value = i915_perf_ioctl_version();
>   		break;
> +	case I915_PARAM_HAS_USERPTR_PROBE:
> +		value = true;
> +		break;
>   	default:
>   		DRM_DEBUG("Unknown parameter %d\n", param->param);
>   		return -EINVAL;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index e20eeeca7a1c..2e4112bf4d38 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait {
>    */
>   #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55
>   
> +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */
> +#define I915_PARAM_HAS_USERPTR_PROBE 56
> +
>   /* Must be kept compact -- no holes and well documented */
>   
>   typedef struct drm_i915_getparam {
> @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr {
>   	 * through the GTT. If the HW can't support readonly access, an error is
>   	 * returned.
>   	 *
> +	 * I915_USERPTR_PROBE:
> +	 *
> +	 * Probe the provided @user_ptr range and validate that the @user_ptr is
> +	 * indeed pointing to normal memory and that the range is also valid.
> +	 * For example if some garbage address is given to the kernel, then this
> +	 * should complain.
> +	 *
> +	 * Returns -EFAULT if the probe failed.
> +	 *
> +	 * Note that this doesn't populate the backing pages.

I'd also add a note on how validation at create time may not mean object 
will still be valid at use time.

With that added, and ignoring the question of whether to have setparam 
or not:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko

> +	 *
> +	 * The kernel supports this feature if I915_PARAM_HAS_USERPTR_PROBE
> +	 * returns a non-zero value.
> +	 *
>   	 * I915_USERPTR_UNSYNCHRONIZED:
>   	 *
>   	 * NOT USED. Setting this flag will result in an error.
>   	 */
>   	__u32 flags;
>   #define I915_USERPTR_READ_ONLY 0x1
> +#define I915_USERPTR_PROBE 0x2
>   #define I915_USERPTR_UNSYNCHRONIZED 0x80000000
>   	/**
>   	 * @handle: Returned handle for the object.
> 