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

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

[Chris Wilson]

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...

Testcase: igt/gem_userptr_blits/probe
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Jason Ekstrand <jason@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.
 	 *
-- 
2.15.0

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

[PATCH 2/2] drm/i915/userptr: Add a flag to populate the userptr on creation

[Chris Wilson]

Acquiring the backing struct pages for the userptr range is not free;
the first client for userptr would insist on frequently creating userptr
objects ahead of time and not use them. For that first client, deferring
the cost of populating the userptr (calling get_user_pages()) to the
actual execbuf was a substantial improvement. However, not all clients
are the same, and most would like to validate that the userptr is valid
and backed by struct pages upon creation, so offer a
I915_USERPTR_POPULATE flag to do just that.

Note that big difference between I915_USERPTR_POPULATE and the deferred
scheme is that POPULATE is guaranteed to be synchronous, the result is
known before the ioctl returns (and the handle exposed). However, due to
system memory pressure, the object may be paged out before use,
requiring them to be paged back in on execbuf (as may always happen).

Testcase: igt/gem_userptr_blits/populate
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Jason Ekstrand <jason@jlekstrand.net>
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 63 ++++++++++++++++++++++++++++++++-
 include/uapi/drm/i915_drm.h             |  1 +
 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index dbc5818dd28b..f05f1322ec27 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -746,6 +746,64 @@ probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
 	return ret;
 }
 
+static int populate(struct drm_i915_gem_object *obj)
+{
+	const unsigned long num_pages = obj->base.size >> PAGE_SHIFT;
+	const unsigned long addr = obj->userptr.ptr;
+	struct sg_table *pages;
+	struct page **pvec;
+	int pinned;
+	int ret = 0;
+
+	pvec = kvmalloc_array(num_pages, sizeof(struct page *), GFP_KERNEL);
+	if (!pvec)
+		return -ENOMEM;
+
+	pinned = __get_user_pages_fast(addr, num_pages,
+				       !obj->userptr.read_only,
+				       pvec);
+	if (pinned < 0) {
+		ret = pinned;
+		goto err;
+	}
+
+	if (pinned < num_pages) {
+		unsigned int flags;
+
+		flags = 0;
+		if (!obj->userptr.read_only)
+			flags |= FOLL_WRITE;
+
+		down_read(&current->mm->mmap_sem);
+		do {
+			ret = get_user_pages(addr + pinned * PAGE_SIZE,
+					     num_pages - pinned,
+					     flags,
+					     pvec + pinned, NULL);
+			if (ret < 0)
+				break;
+
+			pinned += ret;
+		} while (pinned < num_pages);
+		up_read(&current->mm->mmap_sem);
+		if (ret < 0)
+			goto err;
+	}
+
+	mutex_lock(&obj->mm.lock);
+	pages = __i915_gem_userptr_alloc_pages(obj, pvec, num_pages);
+	mutex_unlock(&obj->mm.lock);
+	if (!IS_ERR(pages))
+		pinned = 0;
+	else
+		ret = PTR_ERR(pages);
+
+err:
+	release_pages(pvec, pinned, 0);
+	kvfree(pvec);
+	return ret;
+}
+
 /**
  * Creates a new mm object that wraps some normal memory from the process
  * context - user memory.
@@ -799,6 +857,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_POPULATE |
 			    I915_USERPTR_UNSYNCHRONIZED))
 		return -EINVAL;
 
@@ -816,7 +875,7 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void *data, struct drm_file *file
 		return -ENODEV;
 	}
 
-	if (args->flags & I915_USERPTR_PROBE) {
+	if (args->flags & (I915_USERPTR_PROBE | I915_USERPTR_POPULATE)) {
 		/*
 		 * Check that the range pointed to represents real struct
 		 * pages and not iomappings (at this moment in time!)
@@ -846,6 +905,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void *data, struct drm_file *file
 	ret = i915_gem_userptr_init__mm_struct(obj);
 	if (ret == 0)
 		ret = i915_gem_userptr_init__mmu_notifier(obj, args->flags);
+	if (ret == 0 && args->flags & I915_USERPTR_POPULATE)
+		ret = populate(obj);
 	if (ret == 0)
 		ret = drm_gem_handle_create(file, &obj->base, &handle);
 
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 392ede2ca63e..500c408c912a 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1354,6 +1354,7 @@ struct drm_i915_gem_userptr {
 	__u32 flags;
 #define I915_USERPTR_READ_ONLY		0x1
 #define I915_USERPTR_PROBE		0x2
+#define I915_USERPTR_POPULATE		0x4
 #define I915_USERPTR_UNSYNCHRONIZED	0x80000000
 	/**
 	 * Returned handle for the object.
-- 
2.15.0

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

Re: [PATCH 2/2] drm/i915/userptr: Add a flag to populate the userptr on creation

[Jason Ekstrand]

[-- Attachment #1.1: Type: text/plain, Size: 5871 bytes --]

Both of these patches need I915_PARAM_HAS_USERPTR_WHATEVER queries.

On Wed, Nov 8, 2017 at 10:51 AM, Chris Wilson <chris@chris-wilson.co.uk>
wrote:

> Acquiring the backing struct pages for the userptr range is not free;
> the first client for userptr would insist on frequently creating userptr
> objects ahead of time and not use them. For that first client, deferring
> the cost of populating the userptr (calling get_user_pages()) to the
> actual execbuf was a substantial improvement. However, not all clients
> are the same, and most would like to validate that the userptr is valid
> and backed by struct pages upon creation, so offer a
> I915_USERPTR_POPULATE flag to do just that.
>
> Note that big difference between I915_USERPTR_POPULATE and the deferred
> scheme is that POPULATE is guaranteed to be synchronous, the result is
> known before the ioctl returns (and the handle exposed). However, due to
> system memory pressure, the object may be paged out before use,
> requiring them to be paged back in on execbuf (as may always happen).
>
> Testcase: igt/gem_userptr_blits/populate
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_userptr.c | 63
> ++++++++++++++++++++++++++++++++-
>  include/uapi/drm/i915_drm.h             |  1 +
>  2 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c
> b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index dbc5818dd28b..f05f1322ec27 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -746,6 +746,64 @@ probe_range(struct mm_struct *mm, unsigned long addr,
> unsigned long len)
>         return ret;
>  }
>
> +static int populate(struct drm_i915_gem_object *obj)
> +{
> +       const unsigned long num_pages = obj->base.size >> PAGE_SHIFT;
> +       const unsigned long addr = obj->userptr.ptr;
> +       struct sg_table *pages;
> +       struct page **pvec;
> +       int pinned;
> +       int ret = 0;
> +
> +       pvec = kvmalloc_array(num_pages, sizeof(struct page *),
> GFP_KERNEL);
> +       if (!pvec)
> +               return -ENOMEM;
> +
> +       pinned = __get_user_pages_fast(addr, num_pages,
> +                                      !obj->userptr.read_only,
> +                                      pvec);
> +       if (pinned < 0) {
> +               ret = pinned;
> +               goto err;
> +       }
> +
> +       if (pinned < num_pages) {
> +               unsigned int flags;
> +
> +               flags = 0;
> +               if (!obj->userptr.read_only)
> +                       flags |= FOLL_WRITE;
> +
> +               down_read(&current->mm->mmap_sem);
> +               do {
> +                       ret = get_user_pages(addr + pinned * PAGE_SIZE,
> +                                            num_pages - pinned,
> +                                            flags,
> +                                            pvec + pinned, NULL);
> +                       if (ret < 0)
> +                               break;
> +
> +                       pinned += ret;
> +               } while (pinned < num_pages);
> +               up_read(&current->mm->mmap_sem);
> +               if (ret < 0)
> +                       goto err;
> +       }
> +
> +       mutex_lock(&obj->mm.lock);
> +       pages = __i915_gem_userptr_alloc_pages(obj, pvec, num_pages);
> +       mutex_unlock(&obj->mm.lock);
> +       if (!IS_ERR(pages))
> +               pinned = 0;
> +       else
> +               ret = PTR_ERR(pages);
> +
> +err:
> +       release_pages(pvec, pinned, 0);
> +       kvfree(pvec);
> +       return ret;
> +}
> +
>  /**
>   * Creates a new mm object that wraps some normal memory from the process
>   * context - user memory.
> @@ -799,6 +857,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_POPULATE |
>                             I915_USERPTR_UNSYNCHRONIZED))
>                 return -EINVAL;
>
> @@ -816,7 +875,7 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void
> *data, struct drm_file *file
>                 return -ENODEV;
>         }
>
> -       if (args->flags & I915_USERPTR_PROBE) {
> +       if (args->flags & (I915_USERPTR_PROBE | I915_USERPTR_POPULATE)) {
>                 /*
>                  * Check that the range pointed to represents real struct
>                  * pages and not iomappings (at this moment in time!)
> @@ -846,6 +905,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void
> *data, struct drm_file *file
>         ret = i915_gem_userptr_init__mm_struct(obj);
>         if (ret == 0)
>                 ret = i915_gem_userptr_init__mmu_notifier(obj,
> args->flags);
> +       if (ret == 0 && args->flags & I915_USERPTR_POPULATE)
> +               ret = populate(obj);
>         if (ret == 0)
>                 ret = drm_gem_handle_create(file, &obj->base, &handle);
>
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 392ede2ca63e..500c408c912a 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1354,6 +1354,7 @@ struct drm_i915_gem_userptr {
>         __u32 flags;
>  #define I915_USERPTR_READ_ONLY         0x1
>  #define I915_USERPTR_PROBE             0x2
> +#define I915_USERPTR_POPULATE          0x4
>  #define I915_USERPTR_UNSYNCHRONIZED    0x80000000
>         /**
>          * Returned handle for the object.
> --
> 2.15.0
>
>

[-- Attachment #1.2: Type: text/html, Size: 7418 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/userptr: Probe existence of backing struct pages upon creation

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/userptr: Probe existence of backing struct pages upon creation
URL   : https://patchwork.freedesktop.org/series/33449/
State : success

== Summary ==

Series 33449v1 series starting with [1/2] drm/i915/userptr: Probe existence of backing struct pages upon creation
https://patchwork.freedesktop.org/api/1.0/series/33449/revisions/1/mbox/

Test gem_ringfill:
        Subgroup basic-default-hang:
                dmesg-warn -> INCOMPLETE (fi-pnv-d510) fdo#101600

fdo#101600 https://bugs.freedesktop.org/show_bug.cgi?id=101600

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:447s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:449s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:383s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:546s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:277s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:501s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:508s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:503s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:495s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:426s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:265s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:544s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:432s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:441s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:433s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:496s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:464s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:491s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:531s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:479s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:532s
fi-pnv-d510      total:156  pass:113  dwarn:0   dfail:0   fail:0   skip:42 
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:458s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:544s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:566s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:524s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:493s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:465s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:559s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:425s
Blacklisted hosts:
fi-cnl-y         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:572s

087c404bd6d56a52e0656ac7c79faa376c25b796 drm-tip: 2017y-11m-08d-15h-44m-06s UTC integration manifest
07487e664018 drm/i915/userptr: Add a flag to populate the userptr on creation
369db28de782 drm/i915/userptr: Probe existence of backing struct pages upon creation

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7017/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915/userptr: Probe existence of backing struct pages upon creation

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/userptr: Probe existence of backing struct pages upon creation
URL   : https://patchwork.freedesktop.org/series/33449/
State : success

== Summary ==

Test perf:
        Subgroup polling:
                fail       -> PASS       (shard-hsw) fdo#102252 +1

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252

shard-hsw        total:2540 pass:1432 dwarn:1   dfail:0   fail:10  skip:1097 time:9310s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7017/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Jason Ekstrand <jason@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.
>   	 *
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-09 16:10:47)
> 
> 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?

Populate has exactly the same inconsistency as probe. Neither is
absolute and the answer may change before use. The advantage for
userspace is in the question of whether it can reject bad calls before
committing itself. For which it currently uses set-domain after userptr.

From the pov of the code, doing the populate early is a synchronous
operations, but a late populate can be done asynchronously in parallel
to the client preparing the next request.

However, since userspace already has the controls to do a populate
itself, the only real improvement here is in a cheap probe.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx