intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation
@ 2021-07-23 11:34 Matthew Auld
  2021-07-23 16:50 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Matthew Auld @ 2021-07-23 11:34 UTC (permalink / raw)
  To: intel-gfx
  Cc: Thomas Hellström, dri-devel, Chris Wilson, Kenneth Graunke,
	Daniel Vetter

From: Chris Wilson <chris@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 we 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 PROBE flag, so userspace can easily
  check if the kernel supports it(Jason).
- use mmap_read_{lock, unlock}.
- add some kernel-doc.
v3:
- In the docs also mention that PROBE doesn't guarantee that the pages
  will remain valid by the time they are actually used(Tvrtko).
- Add a small comment for the hole finding logic(Jason).
- Move the param next to all the other params which just return true.

Testcase: igt/gem_userptr_blits/probe
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Kenneth Graunke <kenneth@whitecape.org>
Cc: Jason Ekstrand <jason@jlekstrand.net>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ramalingam C <ramalingam.c@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Acked-by: Kenneth Graunke <kenneth@whitecape.org>
Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
---
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_getparam.c        |  1 +
 include/uapi/drm/i915_drm.h                 | 20 ++++++++++
 3 files changed, 61 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..468a7a617fbf 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -422,6 +422,34 @@ 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) {
+		/* Check for holes, note that we also update the addr below */
+		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);
+
+	return ret;
+}
+
 /*
  * Creates a new mm object that wraps some normal memory from the process
  * context - user memory.
@@ -477,7 +505,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 +533,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..bbb7cac43eb4 100644
--- a/drivers/gpu/drm/i915/i915_getparam.c
+++ b/drivers/gpu/drm/i915/i915_getparam.c
@@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
 	case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
 	case I915_PARAM_HAS_EXEC_SUBMIT_FENCE:
 	case I915_PARAM_HAS_EXEC_TIMELINE_FENCES:
+	case I915_PARAM_HAS_USERPTR_PROBE:
 		/* For the time being all of these are always true;
 		 * if some supported hardware does not have one of these
 		 * features this value needs to be provided from
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 975087553ea0..0d290535a6e5 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 {
@@ -2222,12 +2225,29 @@ 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, and also doesn't
+	 * guarantee that the object will remain valid when the object is
+	 * eventually used.
+	 *
+	 * 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.
-- 
2.26.3

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/userptr: Probe existence of backing struct pages upon creation
  2021-07-23 11:34 [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation Matthew Auld
@ 2021-07-23 16:50 ` Patchwork
  2021-07-23 17:47 ` [Intel-gfx] [PATCH] " Jason Ekstrand
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2021-07-23 16:50 UTC (permalink / raw)
  To: Matthew Auld; +Cc: intel-gfx


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

== Series Details ==

Series: drm/i915/userptr: Probe existence of backing struct pages upon creation
URL   : https://patchwork.freedesktop.org/series/92948/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_10379 -> Patchwork_20695
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/index.html

Known issues
------------

  Here are the changes found in Patchwork_20695 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s0:
    - fi-tgl-u2:          [PASS][1] -> [FAIL][2] ([i915#1888])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/fi-tgl-u2/igt@gem_exec_suspend@basic-s0.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/fi-tgl-u2/igt@gem_exec_suspend@basic-s0.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7500u:       [PASS][3] -> [FAIL][4] ([i915#3449])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/fi-kbl-7500u/igt@kms_chamelium@common-hpd-after-suspend.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/fi-kbl-7500u/igt@kms_chamelium@common-hpd-after-suspend.html

  
  [i915#1888]: https://gitlab.freedesktop.org/drm/intel/issues/1888
  [i915#3449]: https://gitlab.freedesktop.org/drm/intel/issues/3449


Participating hosts (43 -> 36)
------------------------------

  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-bsw-cyan bat-adls-4 fi-ctg-p8600 bat-adls-3 fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_10379 -> Patchwork_20695

  CI-20190529: 20190529
  CI_DRM_10379: 4f3e66252fc1d20871b7d187d719b82d7b3d102d @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6149: 34ff2cf2bc352dce691593db803389fe0eb2be03 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_20695: 89a6ddd95d00e54dad61c31d34bab04f9e484fd1 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

89a6ddd95d00 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_20695/index.html

[-- Attachment #1.2: Type: text/html, Size: 2832 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation
  2021-07-23 11:34 [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation Matthew Auld
  2021-07-23 16:50 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
@ 2021-07-23 17:47 ` Jason Ekstrand
  2021-07-23 17:49   ` Jason Ekstrand
  2021-07-26  8:06   ` Matthew Auld
  2021-07-24  1:38 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork
  2021-07-26  8:31 ` [Intel-gfx] [PATCH] " Maarten Lankhorst
  3 siblings, 2 replies; 16+ messages in thread
From: Jason Ekstrand @ 2021-07-23 17:47 UTC (permalink / raw)
  To: Matthew Auld
  Cc: Thomas Hellström, Intel GFX, Maling list - DRI developers,
	Chris Wilson, Kenneth Graunke, Daniel Vetter

https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/12044

On Fri, Jul 23, 2021 at 6:35 AM Matthew Auld <matthew.auld@intel.com> wrote:
>
> From: Chris Wilson <chris@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 we 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 PROBE flag, so userspace can easily
>   check if the kernel supports it(Jason).
> - use mmap_read_{lock, unlock}.
> - add some kernel-doc.
> v3:
> - In the docs also mention that PROBE doesn't guarantee that the pages
>   will remain valid by the time they are actually used(Tvrtko).
> - Add a small comment for the hole finding logic(Jason).
> - Move the param next to all the other params which just return true.
>
> Testcase: igt/gem_userptr_blits/probe
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Kenneth Graunke <kenneth@whitecape.org>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ramalingam C <ramalingam.c@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Acked-by: Kenneth Graunke <kenneth@whitecape.org>
> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_getparam.c        |  1 +
>  include/uapi/drm/i915_drm.h                 | 20 ++++++++++
>  3 files changed, 61 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..468a7a617fbf 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -422,6 +422,34 @@ 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) {
> +               /* Check for holes, note that we also update the addr below */
> +               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);
> +
> +       return ret;
> +}
> +
>  /*
>   * Creates a new mm object that wraps some normal memory from the process
>   * context - user memory.
> @@ -477,7 +505,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 +533,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..bbb7cac43eb4 100644
> --- a/drivers/gpu/drm/i915/i915_getparam.c
> +++ b/drivers/gpu/drm/i915/i915_getparam.c
> @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
>         case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
>         case I915_PARAM_HAS_EXEC_SUBMIT_FENCE:
>         case I915_PARAM_HAS_EXEC_TIMELINE_FENCES:
> +       case I915_PARAM_HAS_USERPTR_PROBE:
>                 /* For the time being all of these are always true;
>                  * if some supported hardware does not have one of these
>                  * features this value needs to be provided from
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 975087553ea0..0d290535a6e5 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 {
> @@ -2222,12 +2225,29 @@ 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, and also doesn't
> +        * guarantee that the object will remain valid when the object is
> +        * eventually used.
> +        *
> +        * 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.
> --
> 2.26.3
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation
  2021-07-23 17:47 ` [Intel-gfx] [PATCH] " Jason Ekstrand
@ 2021-07-23 17:49   ` Jason Ekstrand
  2021-07-26  8:03     ` Matthew Auld
  2021-07-26  8:06   ` Matthew Auld
  1 sibling, 1 reply; 16+ messages in thread
From: Jason Ekstrand @ 2021-07-23 17:49 UTC (permalink / raw)
  To: Matthew Auld
  Cc: Thomas Hellström, Intel GFX, Maling list - DRI developers,
	Chris Wilson, Kenneth Graunke, Daniel Vetter

Are there IGTs for this anywhere?

On Fri, Jul 23, 2021 at 12:47 PM Jason Ekstrand <jason@jlekstrand.net> wrote:
>
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/12044
>
> On Fri, Jul 23, 2021 at 6:35 AM Matthew Auld <matthew.auld@intel.com> wrote:
> >
> > From: Chris Wilson <chris@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 we 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 PROBE flag, so userspace can easily
> >   check if the kernel supports it(Jason).
> > - use mmap_read_{lock, unlock}.
> > - add some kernel-doc.
> > v3:
> > - In the docs also mention that PROBE doesn't guarantee that the pages
> >   will remain valid by the time they are actually used(Tvrtko).
> > - Add a small comment for the hole finding logic(Jason).
> > - Move the param next to all the other params which just return true.
> >
> > Testcase: igt/gem_userptr_blits/probe
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Kenneth Graunke <kenneth@whitecape.org>
> > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Ramalingam C <ramalingam.c@intel.com>
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Acked-by: Kenneth Graunke <kenneth@whitecape.org>
> > Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++-
> >  drivers/gpu/drm/i915/i915_getparam.c        |  1 +
> >  include/uapi/drm/i915_drm.h                 | 20 ++++++++++
> >  3 files changed, 61 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..468a7a617fbf 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > @@ -422,6 +422,34 @@ 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) {
> > +               /* Check for holes, note that we also update the addr below */
> > +               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);
> > +
> > +       return ret;
> > +}
> > +
> >  /*
> >   * Creates a new mm object that wraps some normal memory from the process
> >   * context - user memory.
> > @@ -477,7 +505,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 +533,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..bbb7cac43eb4 100644
> > --- a/drivers/gpu/drm/i915/i915_getparam.c
> > +++ b/drivers/gpu/drm/i915/i915_getparam.c
> > @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
> >         case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
> >         case I915_PARAM_HAS_EXEC_SUBMIT_FENCE:
> >         case I915_PARAM_HAS_EXEC_TIMELINE_FENCES:
> > +       case I915_PARAM_HAS_USERPTR_PROBE:
> >                 /* For the time being all of these are always true;
> >                  * if some supported hardware does not have one of these
> >                  * features this value needs to be provided from
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index 975087553ea0..0d290535a6e5 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 {
> > @@ -2222,12 +2225,29 @@ 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, and also doesn't
> > +        * guarantee that the object will remain valid when the object is
> > +        * eventually used.
> > +        *
> > +        * 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.
> > --
> > 2.26.3
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915/userptr: Probe existence of backing struct pages upon creation
  2021-07-23 11:34 [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation Matthew Auld
  2021-07-23 16:50 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
  2021-07-23 17:47 ` [Intel-gfx] [PATCH] " Jason Ekstrand
@ 2021-07-24  1:38 ` Patchwork
  2021-07-26  8:31 ` [Intel-gfx] [PATCH] " Maarten Lankhorst
  3 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2021-07-24  1:38 UTC (permalink / raw)
  To: Matthew Auld; +Cc: intel-gfx


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

== Series Details ==

Series: drm/i915/userptr: Probe existence of backing struct pages upon creation
URL   : https://patchwork.freedesktop.org/series/92948/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_10379_full -> Patchwork_20695_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_20695_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@feature_discovery@chamelium:
    - shard-iclb:         NOTRUN -> [SKIP][1] ([fdo#111827])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@feature_discovery@chamelium.html

  * igt@gem_create@create-massive:
    - shard-apl:          NOTRUN -> [DMESG-WARN][2] ([i915#3002])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl6/igt@gem_create@create-massive.html

  * igt@gem_ctx_persistence@engines-mixed:
    - shard-snb:          NOTRUN -> [SKIP][3] ([fdo#109271] / [i915#1099]) +2 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-snb5/igt@gem_ctx_persistence@engines-mixed.html

  * igt@gem_eio@unwedge-stress:
    - shard-tglb:         [PASS][4] -> [TIMEOUT][5] ([i915#2369] / [i915#3063] / [i915#3648])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-tglb6/igt@gem_eio@unwedge-stress.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb1/igt@gem_eio@unwedge-stress.html

  * igt@gem_exec_fair@basic-none-share@rcs0:
    - shard-tglb:         [PASS][6] -> [FAIL][7] ([i915#2842])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-tglb5/igt@gem_exec_fair@basic-none-share@rcs0.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb7/igt@gem_exec_fair@basic-none-share@rcs0.html

  * igt@gem_exec_fair@basic-pace-solo@rcs0:
    - shard-iclb:         NOTRUN -> [FAIL][8] ([i915#2842]) +1 similar issue
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@gem_exec_fair@basic-pace-solo@rcs0.html
    - shard-glk:          NOTRUN -> [FAIL][9] ([i915#2842])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk5/igt@gem_exec_fair@basic-pace-solo@rcs0.html

  * igt@gem_exec_fair@basic-pace@rcs0:
    - shard-tglb:         [PASS][10] -> [FAIL][11] ([i915#2876])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-tglb3/igt@gem_exec_fair@basic-pace@rcs0.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb3/igt@gem_exec_fair@basic-pace@rcs0.html

  * igt@gem_exec_fair@basic-pace@vecs0:
    - shard-kbl:          [PASS][12] -> [SKIP][13] ([fdo#109271])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl2/igt@gem_exec_fair@basic-pace@vecs0.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl1/igt@gem_exec_fair@basic-pace@vecs0.html

  * igt@gem_exec_flush@basic-batch-kernel-default-cmd:
    - shard-snb:          NOTRUN -> [SKIP][14] ([fdo#109271]) +286 similar issues
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-snb5/igt@gem_exec_flush@basic-batch-kernel-default-cmd.html

  * igt@gem_huc_copy@huc-copy:
    - shard-glk:          NOTRUN -> [SKIP][15] ([fdo#109271] / [i915#2190])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk5/igt@gem_huc_copy@huc-copy.html
    - shard-iclb:         NOTRUN -> [SKIP][16] ([i915#2190])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@gem_huc_copy@huc-copy.html

  * igt@gem_mmap_gtt@cpuset-big-copy:
    - shard-glk:          [PASS][17] -> [FAIL][18] ([i915#1888] / [i915#307])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-glk1/igt@gem_mmap_gtt@cpuset-big-copy.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk9/igt@gem_mmap_gtt@cpuset-big-copy.html

  * igt@gem_pwrite@basic-exhaustion:
    - shard-kbl:          NOTRUN -> [WARN][19] ([i915#2658])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl6/igt@gem_pwrite@basic-exhaustion.html

  * igt@gem_render_copy@y-tiled-ccs-to-yf-tiled-mc-ccs:
    - shard-iclb:         NOTRUN -> [SKIP][20] ([i915#768])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@gem_render_copy@y-tiled-ccs-to-yf-tiled-mc-ccs.html

  * igt@gen7_exec_parse@basic-allowed:
    - shard-skl:          NOTRUN -> [SKIP][21] ([fdo#109271]) +34 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl4/igt@gen7_exec_parse@basic-allowed.html

  * igt@i915_pm_rpm@basic-rte:
    - shard-iclb:         NOTRUN -> [FAIL][22] ([i915#579])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@i915_pm_rpm@basic-rte.html

  * igt@i915_pm_rpm@drm-resources-equal:
    - shard-tglb:         NOTRUN -> [SKIP][23] ([i915#579])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb2/igt@i915_pm_rpm@drm-resources-equal.html

  * igt@i915_pm_rpm@gem-mmap-type:
    - shard-iclb:         NOTRUN -> [SKIP][24] ([i915#579])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@i915_pm_rpm@gem-mmap-type.html

  * igt@i915_pm_rpm@modeset-lpsp-stress:
    - shard-apl:          NOTRUN -> [SKIP][25] ([fdo#109271]) +144 similar issues
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl3/igt@i915_pm_rpm@modeset-lpsp-stress.html

  * igt@i915_selftest@live@gt_heartbeat:
    - shard-tglb:         [PASS][26] -> [DMESG-FAIL][27] ([i915#3768])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-tglb6/igt@i915_selftest@live@gt_heartbeat.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb2/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_suspend@forcewake:
    - shard-kbl:          [PASS][28] -> [DMESG-WARN][29] ([i915#180]) +2 similar issues
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl1/igt@i915_suspend@forcewake.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl7/igt@i915_suspend@forcewake.html

  * igt@kms_big_fb@x-tiled-64bpp-rotate-90:
    - shard-iclb:         NOTRUN -> [SKIP][30] ([fdo#110725] / [fdo#111614])
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_big_fb@x-tiled-64bpp-rotate-90.html

  * igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-0-hflip:
    - shard-kbl:          NOTRUN -> [SKIP][31] ([fdo#109271] / [i915#3777])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl2/igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-0-hflip.html

  * igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-0-hflip:
    - shard-apl:          NOTRUN -> [SKIP][32] ([fdo#109271] / [i915#3777])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl1/igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-0-hflip.html

  * igt@kms_big_fb@yf-tiled-64bpp-rotate-0:
    - shard-iclb:         NOTRUN -> [SKIP][33] ([fdo#110723])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_big_fb@yf-tiled-64bpp-rotate-0.html

  * igt@kms_big_fb@yf-tiled-64bpp-rotate-180:
    - shard-tglb:         NOTRUN -> [SKIP][34] ([fdo#111615])
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb2/igt@kms_big_fb@yf-tiled-64bpp-rotate-180.html

  * igt@kms_ccs@pipe-c-bad-pixel-format-y_tiled_ccs:
    - shard-tglb:         NOTRUN -> [SKIP][35] ([i915#3689])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb7/igt@kms_ccs@pipe-c-bad-pixel-format-y_tiled_ccs.html

  * igt@kms_chamelium@dp-mode-timings:
    - shard-apl:          NOTRUN -> [SKIP][36] ([fdo#109271] / [fdo#111827]) +15 similar issues
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl3/igt@kms_chamelium@dp-mode-timings.html

  * igt@kms_chamelium@hdmi-hpd-with-enabled-mode:
    - shard-snb:          NOTRUN -> [SKIP][37] ([fdo#109271] / [fdo#111827]) +6 similar issues
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-snb5/igt@kms_chamelium@hdmi-hpd-with-enabled-mode.html

  * igt@kms_chamelium@vga-frame-dump:
    - shard-glk:          NOTRUN -> [SKIP][38] ([fdo#109271] / [fdo#111827]) +3 similar issues
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk5/igt@kms_chamelium@vga-frame-dump.html

  * igt@kms_color_chamelium@pipe-c-ctm-limited-range:
    - shard-iclb:         NOTRUN -> [SKIP][39] ([fdo#109284] / [fdo#111827]) +3 similar issues
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_color_chamelium@pipe-c-ctm-limited-range.html

  * igt@kms_color_chamelium@pipe-c-degamma:
    - shard-skl:          NOTRUN -> [SKIP][40] ([fdo#109271] / [fdo#111827]) +3 similar issues
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl4/igt@kms_color_chamelium@pipe-c-degamma.html

  * igt@kms_content_protection@atomic:
    - shard-iclb:         NOTRUN -> [SKIP][41] ([fdo#109300] / [fdo#111066])
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_content_protection@atomic.html

  * igt@kms_content_protection@atomic-dpms:
    - shard-apl:          NOTRUN -> [TIMEOUT][42] ([i915#1319])
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl1/igt@kms_content_protection@atomic-dpms.html

  * igt@kms_cursor_crc@pipe-a-cursor-32x32-random:
    - shard-iclb:         NOTRUN -> [SKIP][43] ([fdo#109278]) +15 similar issues
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_cursor_crc@pipe-a-cursor-32x32-random.html

  * igt@kms_cursor_crc@pipe-a-cursor-512x170-random:
    - shard-iclb:         NOTRUN -> [SKIP][44] ([fdo#109278] / [fdo#109279])
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_cursor_crc@pipe-a-cursor-512x170-random.html

  * igt@kms_cursor_legacy@cursorb-vs-flipa-legacy:
    - shard-iclb:         NOTRUN -> [SKIP][45] ([fdo#109274] / [fdo#109278])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_cursor_legacy@cursorb-vs-flipa-legacy.html

  * igt@kms_flip@flip-vs-expired-vblank@c-edp1:
    - shard-skl:          [PASS][46] -> [FAIL][47] ([i915#79])
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-skl10/igt@kms_flip@flip-vs-expired-vblank@c-edp1.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl10/igt@kms_flip@flip-vs-expired-vblank@c-edp1.html

  * igt@kms_flip@flip-vs-suspend@b-edp1:
    - shard-skl:          [PASS][48] -> [INCOMPLETE][49] ([i915#146] / [i915#198])
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-skl8/igt@kms_flip@flip-vs-suspend@b-edp1.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl1/igt@kms_flip@flip-vs-suspend@b-edp1.html

  * igt@kms_flip@flip-vs-suspend@c-dp1:
    - shard-kbl:          NOTRUN -> [DMESG-WARN][50] ([i915#180])
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl1/igt@kms_flip@flip-vs-suspend@c-dp1.html

  * igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-32bpp-ytileccs:
    - shard-skl:          NOTRUN -> [INCOMPLETE][51] ([i915#3699])
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl4/igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-32bpp-ytileccs.html

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-pri-shrfb-draw-blt:
    - shard-iclb:         NOTRUN -> [SKIP][52] ([fdo#109280]) +8 similar issues
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-pri-shrfb-draw-blt.html

  * igt@kms_frontbuffer_tracking@psr-2p-scndscrn-spr-indfb-draw-render:
    - shard-glk:          NOTRUN -> [SKIP][53] ([fdo#109271]) +29 similar issues
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk5/igt@kms_frontbuffer_tracking@psr-2p-scndscrn-spr-indfb-draw-render.html

  * igt@kms_frontbuffer_tracking@psr-rgb101010-draw-mmap-wc:
    - shard-kbl:          NOTRUN -> [SKIP][54] ([fdo#109271]) +54 similar issues
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl2/igt@kms_frontbuffer_tracking@psr-rgb101010-draw-mmap-wc.html

  * igt@kms_pipe_crc_basic@disable-crc-after-crtc-pipe-d:
    - shard-apl:          NOTRUN -> [SKIP][55] ([fdo#109271] / [i915#533]) +1 similar issue
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl1/igt@kms_pipe_crc_basic@disable-crc-after-crtc-pipe-d.html

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-d:
    - shard-kbl:          NOTRUN -> [SKIP][56] ([fdo#109271] / [i915#533])
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl6/igt@kms_pipe_crc_basic@hang-read-crc-pipe-d.html

  * igt@kms_plane_alpha_blend@pipe-a-alpha-7efc:
    - shard-apl:          NOTRUN -> [FAIL][57] ([fdo#108145] / [i915#265])
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl3/igt@kms_plane_alpha_blend@pipe-a-alpha-7efc.html

  * igt@kms_plane_alpha_blend@pipe-b-alpha-transparent-fb:
    - shard-apl:          NOTRUN -> [FAIL][58] ([i915#265]) +1 similar issue
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl6/igt@kms_plane_alpha_blend@pipe-b-alpha-transparent-fb.html

  * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max:
    - shard-kbl:          NOTRUN -> [FAIL][59] ([fdo#108145] / [i915#265])
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl6/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max.html

  * igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min:
    - shard-skl:          NOTRUN -> [FAIL][60] ([fdo#108145] / [i915#265])
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl10/igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min.html

  * igt@kms_plane_lowres@pipe-b-tiling-none:
    - shard-iclb:         NOTRUN -> [SKIP][61] ([i915#3536]) +1 similar issue
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_plane_lowres@pipe-b-tiling-none.html

  * igt@kms_psr2_sf@overlay-primary-update-sf-dmg-area-2:
    - shard-iclb:         NOTRUN -> [SKIP][62] ([i915#658])
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_psr2_sf@overlay-primary-update-sf-dmg-area-2.html
    - shard-glk:          NOTRUN -> [SKIP][63] ([fdo#109271] / [i915#658])
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk5/igt@kms_psr2_sf@overlay-primary-update-sf-dmg-area-2.html

  * igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-5:
    - shard-kbl:          NOTRUN -> [SKIP][64] ([fdo#109271] / [i915#658]) +1 similar issue
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl6/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-5.html

  * igt@kms_psr2_su@page_flip:
    - shard-apl:          NOTRUN -> [SKIP][65] ([fdo#109271] / [i915#658]) +2 similar issues
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl1/igt@kms_psr2_su@page_flip.html

  * igt@kms_psr@psr2_basic:
    - shard-iclb:         [PASS][66] -> [SKIP][67] ([fdo#109441]) +2 similar issues
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb2/igt@kms_psr@psr2_basic.html
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb5/igt@kms_psr@psr2_basic.html

  * igt@kms_psr@psr2_cursor_mmap_cpu:
    - shard-iclb:         NOTRUN -> [SKIP][68] ([fdo#109441])
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_psr@psr2_cursor_mmap_cpu.html

  * igt@kms_setmode@basic:
    - shard-snb:          NOTRUN -> [FAIL][69] ([i915#31])
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-snb5/igt@kms_setmode@basic.html

  * igt@kms_vrr@flip-suspend:
    - shard-iclb:         NOTRUN -> [SKIP][70] ([fdo#109502])
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_vrr@flip-suspend.html

  * igt@kms_writeback@writeback-invalid-parameters:
    - shard-apl:          NOTRUN -> [SKIP][71] ([fdo#109271] / [i915#2437])
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl3/igt@kms_writeback@writeback-invalid-parameters.html

  * igt@perf@polling-parameterized:
    - shard-tglb:         [PASS][72] -> [FAIL][73] ([i915#1542])
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-tglb3/igt@perf@polling-parameterized.html
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb2/igt@perf@polling-parameterized.html

  * igt@perf@unprivileged-single-ctx-counters:
    - shard-iclb:         NOTRUN -> [SKIP][74] ([fdo#109289]) +1 similar issue
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@perf@unprivileged-single-ctx-counters.html

  * igt@prime_nv_pcopy@test3_3:
    - shard-iclb:         NOTRUN -> [SKIP][75] ([fdo#109291])
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@prime_nv_pcopy@test3_3.html

  * igt@sysfs_clients@sema-10:
    - shard-kbl:          NOTRUN -> [SKIP][76] ([fdo#109271] / [i915#2994])
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl6/igt@sysfs_clients@sema-10.html

  * igt@sysfs_clients@sema-25:
    - shard-apl:          NOTRUN -> [SKIP][77] ([fdo#109271] / [i915#2994])
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl1/igt@sysfs_clients@sema-25.html

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@preservation-s3@rcs0:
    - shard-skl:          [INCOMPLETE][78] ([i915#198]) -> [PASS][79] +1 similar issue
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-skl10/igt@gem_ctx_isolation@preservation-s3@rcs0.html
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl4/igt@gem_ctx_isolation@preservation-s3@rcs0.html

  * igt@gem_eio@hibernate:
    - {shard-rkl}:        [INCOMPLETE][80] ([i915#3811] / [i915#3833]) -> [PASS][81]
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-rkl-1/igt@gem_eio@hibernate.html
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-rkl-1/igt@gem_eio@hibernate.html

  * igt@gem_eio@in-flight-contexts-10ms:
    - shard-tglb:         [TIMEOUT][82] ([i915#3063]) -> [PASS][83]
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-tglb6/igt@gem_eio@in-flight-contexts-10ms.html
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb6/igt@gem_eio@in-flight-contexts-10ms.html

  * igt@gem_eio@unwedge-stress:
    - {shard-rkl}:        [TIMEOUT][84] ([i915#3063]) -> [PASS][85]
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-rkl-5/igt@gem_eio@unwedge-stress.html
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-rkl-5/igt@gem_eio@unwedge-stress.html

  * igt@gem_exec_fair@basic-deadline:
    - shard-glk:          [FAIL][86] ([i915#2846]) -> [PASS][87]
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-glk4/igt@gem_exec_fair@basic-deadline.html
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk1/igt@gem_exec_fair@basic-deadline.html

  * igt@gem_exec_fair@basic-none@vcs0:
    - shard-kbl:          [FAIL][88] ([i915#2842]) -> [PASS][89]
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl3/igt@gem_exec_fair@basic-none@vcs0.html
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl6/igt@gem_exec_fair@basic-none@vcs0.html

  * igt@gem_exec_fair@basic-pace@rcs0:
    - {shard-rkl}:        [FAIL][90] ([i915#2842]) -> [PASS][91] +2 similar issues
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-rkl-5/igt@gem_exec_fair@basic-pace@rcs0.html
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-rkl-5/igt@gem_exec_fair@basic-pace@rcs0.html

  * igt@gem_exec_fair@basic-throttle@rcs0:
    - shard-iclb:         [FAIL][92] ([i915#2849]) -> [PASS][93]
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb2/igt@gem_exec_fair@basic-throttle@rcs0.html
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb5/igt@gem_exec_fair@basic-throttle@rcs0.html

  * igt@gem_exec_whisper@basic-contexts-forked:
    - shard-iclb:         [INCOMPLETE][94] ([i915#1895]) -> [PASS][95]
   [94]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb8/igt@gem_exec_whisper@basic-contexts-forked.html
   [95]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@gem_exec_whisper@basic-contexts-forked.html

  * igt@gem_mmap_gtt@cpuset-medium-copy:
    - shard-glk:          [FAIL][96] ([i915#1888] / [i915#307]) -> [PASS][97]
   [96]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-glk2/igt@gem_mmap_gtt@cpuset-medium-copy.html
   [97]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk2/igt@gem_mmap_gtt@cpuset-medium-copy.html

  * igt@i915_pm_rc6_residency@rc6-fence:
    - shard-tglb:         [WARN][98] ([i915#2681]) -> [PASS][99]
   [98]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-tglb7/igt@i915_pm_rc6_residency@rc6-fence.html
   [99]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb5/igt@i915_pm_rc6_residency@rc6-fence.html

  * igt@i915_selftest@live@hangcheck:
    - shard-snb:          [INCOMPLETE][100] ([i915#2782]) -> [PASS][101]
   [100]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-snb6/igt@i915_selftest@live@hangcheck.html
   [101]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-snb7/igt@i915_selftest@live@hangcheck.html

  * igt@kms_big_fb@linear-64bpp-rotate-180:
    - shard-iclb:         [DMESG-WARN][102] ([i915#3621]) -> [PASS][103]
   [102]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb1/igt@kms_big_fb@linear-64bpp-rotate-180.html
   [103]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_big_fb@linear-64bpp-rotate-180.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-kbl:          [DMESG-WARN][104] ([i915#180]) -> [PASS][105] +1 similar issue
   [104]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl7/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [105]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl2/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
    - shard-skl:          [FAIL][106] ([i915#2346] / [i915#533]) -> [PASS][107]
   [106]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-skl1/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
   [107]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl10/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html

  * igt@kms_fbcon_fbt@fbc-suspend:
    - shard-kbl:          [INCOMPLETE][108] ([i915#155] / [i915#180] / [i915#636]) -> [PASS][109]
   [108]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl7/igt@kms_fbcon_fbt@fbc-suspend.html
   [109]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl6/igt@kms_fbcon_fbt@fbc-suspend.html

  * igt@kms_flip@flip-vs-expired-vblank@a-dp1:
    - shard-apl:          [FAIL][110] ([i915#79]) -> [PASS][111]
   [110]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-apl1/igt@kms_flip@flip-vs-expired-vblank@a-dp1.html
   [111]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl1/igt@kms_flip@flip-vs-expired-vblank@a-dp1.html

  * igt@kms_flip@flip-vs-expired-vblank@a-edp1:
    - shard-skl:          [FAIL][112] ([i915#79]) -> [PASS][113]
   [112]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-skl10/igt@kms_flip@flip-vs-expired-vblank@a-edp1.html
   [113]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl10/igt@kms_flip@flip-vs-expired-vblank@a-edp1.html

  * igt@kms_flip@flip-vs-suspend@a-dp1:
    - shard-kbl:          [INCOMPLETE][114] ([i915#155]) -> [PASS][115]
   [114]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl4/igt@kms_flip@flip-vs-suspend@a-dp1.html
   [115]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl1/igt@kms_flip@flip-vs-suspend@a-dp1.html

  * igt@kms_flip@plain-flip-ts-check-interruptible@b-edp1:
    - shard-skl:          [FAIL][116] ([i915#2122]) -> [PASS][117]
   [116]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-skl4/igt@kms_flip@plain-flip-ts-check-interruptible@b-edp1.html
   [117]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl5/igt@kms_flip@plain-flip-ts-check-interruptible@b-edp1.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [FAIL][118] ([fdo#108145] / [i915#265]) -> [PASS][119]
   [118]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-skl9/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [119]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl2/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-vs-premult-vs-constant:
    - shard-iclb:         [SKIP][120] ([fdo#109278]) -> [PASS][121]
   [120]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb2/igt@kms_plane_alpha_blend@pipe-c-coverage-vs-premult-vs-constant.html
   [121]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb5/igt@kms_plane_alpha_blend@pipe-c-coverage-vs-premult-vs-constant.html

  * igt@kms_psr@psr2_cursor_render:
    - shard-iclb:         [SKIP][122] ([fdo#109441]) -> [PASS][123]
   [122]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb3/igt@kms_psr@psr2_cursor_render.html
   [123]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb2/igt@kms_psr@psr2_cursor_render.html

  * igt@perf@blocking:
    - {shard-rkl}:        [FAIL][124] ([i915#1542]) -> [PASS][125]
   [124]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-rkl-6/igt@perf@blocking.html
   [125]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-rkl-6/igt@perf@blocking.html

  * igt@perf@polling-parameterized:
    - shard-apl:          [FAIL][126] ([i915#1542]) -> [PASS][127]
   [126]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-apl3/igt@perf@polling-parameterized.html
   [127]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl7/igt@perf@polling-parameterized.html

  * igt@prime_vgem@coherency-blt:
    - shard-glk:          [INCOMPLETE][128] ([i915#2944]) -> [PASS][129]
   [128]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-glk5/igt@prime_vgem@coherency-blt.html
   [129]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk5/igt@prime_vgem@coherency-blt.html

  
#### Warnings ####

  * igt@i915_pm_rc6_residency@rc6-fence:
    - shard-iclb:         [WARN][130] ([i915#1804] / [i915#2684]) -> [WARN][131] ([i915#2684])
   [130]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb3/igt@i915_pm_rc6_residency@rc6-fence.html
   [131]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb8/igt@i915_pm_rc6_residency@rc6-fence.html

  * igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-2:
    - shard-iclb:         [SKIP][132] ([i915#658]) -> [SKIP][133] ([i915#2920]) +1 similar issue
   [132]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb3/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-2.html
   [133]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb2/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-2.html

  * igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-4:
    - shard-iclb:         [SKIP][134] ([i915#2920]) -> [SKIP][135] ([i915#658])
   [134]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb2/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-4.html
   [135]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb5/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-4.html

  * igt@runner@aborted:
    - shard-kbl:          ([FAIL][136], [FAIL][137], [FAIL][138], [FAIL][139], [FAIL][140], [FAIL][141], [FAIL][142], [FAIL][143]) ([i915#1436] / [i915#180] / [i915#1814] / [i915#2505] / [i915#3002] / [i915#3363] / [i915#602] / [i915#92]) -> ([FAIL][144], [FAIL][145], [FAIL][146], [FAIL][147], [FAIL][148], [FAIL][149], [FAIL][150]) ([i915#180] / [i915#1814] / [i915#3002] / [i915#3363])
   [136]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl7/igt@runner@aborted.html
   [137]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl4/igt@runner@aborted.html
   [138]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl6/igt@runner@aborted.html
   [139]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl7/igt@runner@aborted.html
   [140]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl6/igt@runner@aborted.html
   [141]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl7/igt@runner@aborted.html
   [142]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl4/igt@runner@aborted.html
   [143]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl7/igt@runner@aborted.html
   [144]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl7/igt@runner@aborted.html
   [145]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl7/igt@runner@aborted.html
   [146]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl7/igt@runner@aborted.html
   [147]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl7/igt@runner@aborted.html
   [148]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl2/igt@runner@aborted.html
   [149]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl2/igt@runner@aborted.html
   [150]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl1/igt@runner@aborted.html
    - shard-iclb:         ([FAIL][151], [FAIL][152], [FAIL][153]) ([i915#1814] / [i915#3

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/index.html

[-- Attachment #1.2: Type: text/html, Size: 34117 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation
  2021-07-23 17:49   ` Jason Ekstrand
@ 2021-07-26  8:03     ` Matthew Auld
  0 siblings, 0 replies; 16+ messages in thread
From: Matthew Auld @ 2021-07-26  8:03 UTC (permalink / raw)
  To: Jason Ekstrand
  Cc: Thomas Hellström, Daniel Vetter, Intel GFX,
	Maling list - DRI developers, Chris Wilson, Kenneth Graunke,
	Matthew Auld

On Fri, 23 Jul 2021 at 18:49, Jason Ekstrand <jason@jlekstrand.net> wrote:
>
> Are there IGTs for this anywhere?

https://patchwork.freedesktop.org/series/92580/

>
> On Fri, Jul 23, 2021 at 12:47 PM Jason Ekstrand <jason@jlekstrand.net> wrote:
> >
> > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/12044
> >
> > On Fri, Jul 23, 2021 at 6:35 AM Matthew Auld <matthew.auld@intel.com> wrote:
> > >
> > > From: Chris Wilson <chris@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 we 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 PROBE flag, so userspace can easily
> > >   check if the kernel supports it(Jason).
> > > - use mmap_read_{lock, unlock}.
> > > - add some kernel-doc.
> > > v3:
> > > - In the docs also mention that PROBE doesn't guarantee that the pages
> > >   will remain valid by the time they are actually used(Tvrtko).
> > > - Add a small comment for the hole finding logic(Jason).
> > > - Move the param next to all the other params which just return true.
> > >
> > > Testcase: igt/gem_userptr_blits/probe
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > > Cc: Kenneth Graunke <kenneth@whitecape.org>
> > > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Ramalingam C <ramalingam.c@intel.com>
> > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Acked-by: Kenneth Graunke <kenneth@whitecape.org>
> > > Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
> > > ---
> > >  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++-
> > >  drivers/gpu/drm/i915/i915_getparam.c        |  1 +
> > >  include/uapi/drm/i915_drm.h                 | 20 ++++++++++
> > >  3 files changed, 61 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..468a7a617fbf 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > > @@ -422,6 +422,34 @@ 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) {
> > > +               /* Check for holes, note that we also update the addr below */
> > > +               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);
> > > +
> > > +       return ret;
> > > +}
> > > +
> > >  /*
> > >   * Creates a new mm object that wraps some normal memory from the process
> > >   * context - user memory.
> > > @@ -477,7 +505,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 +533,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..bbb7cac43eb4 100644
> > > --- a/drivers/gpu/drm/i915/i915_getparam.c
> > > +++ b/drivers/gpu/drm/i915/i915_getparam.c
> > > @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
> > >         case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
> > >         case I915_PARAM_HAS_EXEC_SUBMIT_FENCE:
> > >         case I915_PARAM_HAS_EXEC_TIMELINE_FENCES:
> > > +       case I915_PARAM_HAS_USERPTR_PROBE:
> > >                 /* For the time being all of these are always true;
> > >                  * if some supported hardware does not have one of these
> > >                  * features this value needs to be provided from
> > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > > index 975087553ea0..0d290535a6e5 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 {
> > > @@ -2222,12 +2225,29 @@ 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, and also doesn't
> > > +        * guarantee that the object will remain valid when the object is
> > > +        * eventually used.
> > > +        *
> > > +        * 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.
> > > --
> > > 2.26.3
> > >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation
  2021-07-23 17:47 ` [Intel-gfx] [PATCH] " Jason Ekstrand
  2021-07-23 17:49   ` Jason Ekstrand
@ 2021-07-26  8:06   ` Matthew Auld
  2021-07-26 15:12     ` Jason Ekstrand
  1 sibling, 1 reply; 16+ messages in thread
From: Matthew Auld @ 2021-07-26  8:06 UTC (permalink / raw)
  To: Jason Ekstrand
  Cc: Thomas Hellström, Daniel Vetter, Intel GFX,
	Maling list - DRI developers, Chris Wilson, Kenneth Graunke,
	Matthew Auld

On Fri, 23 Jul 2021 at 18:48, Jason Ekstrand <jason@jlekstrand.net> wrote:
>
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/12044

Cool, is that ready to go? i.e can we start merging the kernel + IGT side.

>
> On Fri, Jul 23, 2021 at 6:35 AM Matthew Auld <matthew.auld@intel.com> wrote:
> >
> > From: Chris Wilson <chris@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 we 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 PROBE flag, so userspace can easily
> >   check if the kernel supports it(Jason).
> > - use mmap_read_{lock, unlock}.
> > - add some kernel-doc.
> > v3:
> > - In the docs also mention that PROBE doesn't guarantee that the pages
> >   will remain valid by the time they are actually used(Tvrtko).
> > - Add a small comment for the hole finding logic(Jason).
> > - Move the param next to all the other params which just return true.
> >
> > Testcase: igt/gem_userptr_blits/probe
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Kenneth Graunke <kenneth@whitecape.org>
> > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Ramalingam C <ramalingam.c@intel.com>
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Acked-by: Kenneth Graunke <kenneth@whitecape.org>
> > Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++-
> >  drivers/gpu/drm/i915/i915_getparam.c        |  1 +
> >  include/uapi/drm/i915_drm.h                 | 20 ++++++++++
> >  3 files changed, 61 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..468a7a617fbf 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > @@ -422,6 +422,34 @@ 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) {
> > +               /* Check for holes, note that we also update the addr below */
> > +               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);
> > +
> > +       return ret;
> > +}
> > +
> >  /*
> >   * Creates a new mm object that wraps some normal memory from the process
> >   * context - user memory.
> > @@ -477,7 +505,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 +533,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..bbb7cac43eb4 100644
> > --- a/drivers/gpu/drm/i915/i915_getparam.c
> > +++ b/drivers/gpu/drm/i915/i915_getparam.c
> > @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
> >         case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
> >         case I915_PARAM_HAS_EXEC_SUBMIT_FENCE:
> >         case I915_PARAM_HAS_EXEC_TIMELINE_FENCES:
> > +       case I915_PARAM_HAS_USERPTR_PROBE:
> >                 /* For the time being all of these are always true;
> >                  * if some supported hardware does not have one of these
> >                  * features this value needs to be provided from
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index 975087553ea0..0d290535a6e5 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 {
> > @@ -2222,12 +2225,29 @@ 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, and also doesn't
> > +        * guarantee that the object will remain valid when the object is
> > +        * eventually used.
> > +        *
> > +        * 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.
> > --
> > 2.26.3
> >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation
  2021-07-23 11:34 [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation Matthew Auld
                   ` (2 preceding siblings ...)
  2021-07-24  1:38 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork
@ 2021-07-26  8:31 ` Maarten Lankhorst
  2021-07-26 15:14   ` Jason Ekstrand
  3 siblings, 1 reply; 16+ messages in thread
From: Maarten Lankhorst @ 2021-07-26  8:31 UTC (permalink / raw)
  To: Matthew Auld, intel-gfx
  Cc: Thomas Hellström, dri-devel, Chris Wilson, Kenneth Graunke,
	Daniel Vetter

Op 23-07-2021 om 13:34 schreef Matthew Auld:
> From: Chris Wilson <chris@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 we 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 PROBE flag, so userspace can easily
>   check if the kernel supports it(Jason).
> - use mmap_read_{lock, unlock}.
> - add some kernel-doc.
> v3:
> - In the docs also mention that PROBE doesn't guarantee that the pages
>   will remain valid by the time they are actually used(Tvrtko).
> - Add a small comment for the hole finding logic(Jason).
> - Move the param next to all the other params which just return true.
>
> Testcase: igt/gem_userptr_blits/probe
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Kenneth Graunke <kenneth@whitecape.org>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ramalingam C <ramalingam.c@intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Acked-by: Kenneth Graunke <kenneth@whitecape.org>
> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_getparam.c        |  1 +
>  include/uapi/drm/i915_drm.h                 | 20 ++++++++++
>  3 files changed, 61 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..468a7a617fbf 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -422,6 +422,34 @@ 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) {
> +		/* Check for holes, note that we also update the addr below */
> +		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);
> +
> +	return ret;
> +}
> +
>  /*
>   * Creates a new mm object that wraps some normal memory from the process
>   * context - user memory.
> @@ -477,7 +505,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 +533,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..bbb7cac43eb4 100644
> --- a/drivers/gpu/drm/i915/i915_getparam.c
> +++ b/drivers/gpu/drm/i915/i915_getparam.c
> @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
>  	case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
>  	case I915_PARAM_HAS_EXEC_SUBMIT_FENCE:
>  	case I915_PARAM_HAS_EXEC_TIMELINE_FENCES:
> +	case I915_PARAM_HAS_USERPTR_PROBE:
>  		/* For the time being all of these are always true;
>  		 * if some supported hardware does not have one of these
>  		 * features this value needs to be provided from
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 975087553ea0..0d290535a6e5 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 {
> @@ -2222,12 +2225,29 @@ 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, and also doesn't
> +	 * guarantee that the object will remain valid when the object is
> +	 * eventually used.
> +	 *
> +	 * 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.

Could we use _VALIDATE instead of probe? Or at least pin the pages as well, so we don't have to do it later?

We already have i915_gem_object_userptr_validate, no need to dupe it.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation
  2021-07-26  8:06   ` Matthew Auld
@ 2021-07-26 15:12     ` Jason Ekstrand
  0 siblings, 0 replies; 16+ messages in thread
From: Jason Ekstrand @ 2021-07-26 15:12 UTC (permalink / raw)
  To: Matthew Auld
  Cc: Thomas Hellström, Daniel Vetter, Intel GFX,
	Maling list - DRI developers, Chris Wilson, Kenneth Graunke,
	Matthew Auld

On Mon, Jul 26, 2021 at 3:06 AM Matthew Auld
<matthew.william.auld@gmail.com> wrote:
>
> On Fri, 23 Jul 2021 at 18:48, Jason Ekstrand <jason@jlekstrand.net> wrote:
> >
> > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/12044
>
> Cool, is that ready to go? i.e can we start merging the kernel + IGT side.

Yes, it's all reviewed.  Though, it sounds like Maarten had a comment
so we should settle on that before landing.

> >
> > On Fri, Jul 23, 2021 at 6:35 AM Matthew Auld <matthew.auld@intel.com> wrote:
> > >
> > > From: Chris Wilson <chris@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 we 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 PROBE flag, so userspace can easily
> > >   check if the kernel supports it(Jason).
> > > - use mmap_read_{lock, unlock}.
> > > - add some kernel-doc.
> > > v3:
> > > - In the docs also mention that PROBE doesn't guarantee that the pages
> > >   will remain valid by the time they are actually used(Tvrtko).
> > > - Add a small comment for the hole finding logic(Jason).
> > > - Move the param next to all the other params which just return true.
> > >
> > > Testcase: igt/gem_userptr_blits/probe
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > > Cc: Kenneth Graunke <kenneth@whitecape.org>
> > > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Ramalingam C <ramalingam.c@intel.com>
> > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Acked-by: Kenneth Graunke <kenneth@whitecape.org>
> > > Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
> > > ---
> > >  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++-
> > >  drivers/gpu/drm/i915/i915_getparam.c        |  1 +
> > >  include/uapi/drm/i915_drm.h                 | 20 ++++++++++
> > >  3 files changed, 61 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..468a7a617fbf 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > > @@ -422,6 +422,34 @@ 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) {
> > > +               /* Check for holes, note that we also update the addr below */
> > > +               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);
> > > +
> > > +       return ret;
> > > +}
> > > +
> > >  /*
> > >   * Creates a new mm object that wraps some normal memory from the process
> > >   * context - user memory.
> > > @@ -477,7 +505,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 +533,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..bbb7cac43eb4 100644
> > > --- a/drivers/gpu/drm/i915/i915_getparam.c
> > > +++ b/drivers/gpu/drm/i915/i915_getparam.c
> > > @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
> > >         case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
> > >         case I915_PARAM_HAS_EXEC_SUBMIT_FENCE:
> > >         case I915_PARAM_HAS_EXEC_TIMELINE_FENCES:
> > > +       case I915_PARAM_HAS_USERPTR_PROBE:
> > >                 /* For the time being all of these are always true;
> > >                  * if some supported hardware does not have one of these
> > >                  * features this value needs to be provided from
> > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > > index 975087553ea0..0d290535a6e5 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 {
> > > @@ -2222,12 +2225,29 @@ 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, and also doesn't
> > > +        * guarantee that the object will remain valid when the object is
> > > +        * eventually used.
> > > +        *
> > > +        * 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.
> > > --
> > > 2.26.3
> > >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation
  2021-07-26  8:31 ` [Intel-gfx] [PATCH] " Maarten Lankhorst
@ 2021-07-26 15:14   ` Jason Ekstrand
  2021-07-26 16:10     ` Tvrtko Ursulin
  0 siblings, 1 reply; 16+ messages in thread
From: Jason Ekstrand @ 2021-07-26 15:14 UTC (permalink / raw)
  To: Maarten Lankhorst
  Cc: Thomas Hellström, Intel GFX, Maling list - DRI developers,
	Chris Wilson, Kenneth Graunke, Matthew Auld, Daniel Vetter

On Mon, Jul 26, 2021 at 3:31 AM Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
>
> Op 23-07-2021 om 13:34 schreef Matthew Auld:
> > From: Chris Wilson <chris@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 we 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 PROBE flag, so userspace can easily
> >   check if the kernel supports it(Jason).
> > - use mmap_read_{lock, unlock}.
> > - add some kernel-doc.
> > v3:
> > - In the docs also mention that PROBE doesn't guarantee that the pages
> >   will remain valid by the time they are actually used(Tvrtko).
> > - Add a small comment for the hole finding logic(Jason).
> > - Move the param next to all the other params which just return true.
> >
> > Testcase: igt/gem_userptr_blits/probe
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Kenneth Graunke <kenneth@whitecape.org>
> > Cc: Jason Ekstrand <jason@jlekstrand.net>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Ramalingam C <ramalingam.c@intel.com>
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Acked-by: Kenneth Graunke <kenneth@whitecape.org>
> > Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++-
> >  drivers/gpu/drm/i915/i915_getparam.c        |  1 +
> >  include/uapi/drm/i915_drm.h                 | 20 ++++++++++
> >  3 files changed, 61 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..468a7a617fbf 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > @@ -422,6 +422,34 @@ 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) {
> > +             /* Check for holes, note that we also update the addr below */
> > +             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);
> > +
> > +     return ret;
> > +}
> > +
> >  /*
> >   * Creates a new mm object that wraps some normal memory from the process
> >   * context - user memory.
> > @@ -477,7 +505,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 +533,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..bbb7cac43eb4 100644
> > --- a/drivers/gpu/drm/i915/i915_getparam.c
> > +++ b/drivers/gpu/drm/i915/i915_getparam.c
> > @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
> >       case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
> >       case I915_PARAM_HAS_EXEC_SUBMIT_FENCE:
> >       case I915_PARAM_HAS_EXEC_TIMELINE_FENCES:
> > +     case I915_PARAM_HAS_USERPTR_PROBE:
> >               /* For the time being all of these are always true;
> >                * if some supported hardware does not have one of these
> >                * features this value needs to be provided from
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index 975087553ea0..0d290535a6e5 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 {
> > @@ -2222,12 +2225,29 @@ 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, and also doesn't
> > +      * guarantee that the object will remain valid when the object is
> > +      * eventually used.
> > +      *
> > +      * 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.
>
> Could we use _VALIDATE instead of probe? Or at least pin the pages as well, so we don't have to do it later?

I only care that the name matches what it does.  _VALIDATE sounds like
it does a full validation of everything such that, if the import
succeeds, execbuf will as well.  If we pin the pages at the same time,
maybe that's true?  _PROBE, on the other hand, sounds a lot more like
a one-time best-effort check which may race with other stuff and
doesn't guarantee future success.  That's in line with what the
current patch does.

> We already have i915_gem_object_userptr_validate, no need to dupe it.

I have no opinion on this.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation
  2021-07-26 15:14   ` Jason Ekstrand
@ 2021-07-26 16:10     ` Tvrtko Ursulin
  2021-07-28 14:22       ` Matthew Auld
  0 siblings, 1 reply; 16+ messages in thread
From: Tvrtko Ursulin @ 2021-07-26 16:10 UTC (permalink / raw)
  To: Jason Ekstrand, Maarten Lankhorst
  Cc: Thomas Hellström, Intel GFX, Maling list - DRI developers,
	Chris Wilson, Kenneth Graunke, Matthew Auld, Daniel Vetter


On 26/07/2021 16:14, Jason Ekstrand wrote:
> On Mon, Jul 26, 2021 at 3:31 AM Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>>
>> Op 23-07-2021 om 13:34 schreef Matthew Auld:
>>> From: Chris Wilson <chris@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 we 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 PROBE flag, so userspace can easily
>>>    check if the kernel supports it(Jason).
>>> - use mmap_read_{lock, unlock}.
>>> - add some kernel-doc.
>>> v3:
>>> - In the docs also mention that PROBE doesn't guarantee that the pages
>>>    will remain valid by the time they are actually used(Tvrtko).
>>> - Add a small comment for the hole finding logic(Jason).
>>> - Move the param next to all the other params which just return true.
>>>
>>> Testcase: igt/gem_userptr_blits/probe
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Kenneth Graunke <kenneth@whitecape.org>
>>> Cc: Jason Ekstrand <jason@jlekstrand.net>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Ramalingam C <ramalingam.c@intel.com>
>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Acked-by: Kenneth Graunke <kenneth@whitecape.org>
>>> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
>>> ---
>>>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++-
>>>   drivers/gpu/drm/i915/i915_getparam.c        |  1 +
>>>   include/uapi/drm/i915_drm.h                 | 20 ++++++++++
>>>   3 files changed, 61 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..468a7a617fbf 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>> @@ -422,6 +422,34 @@ 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) {
>>> +             /* Check for holes, note that we also update the addr below */
>>> +             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);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>>   /*
>>>    * Creates a new mm object that wraps some normal memory from the process
>>>    * context - user memory.
>>> @@ -477,7 +505,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 +533,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..bbb7cac43eb4 100644
>>> --- a/drivers/gpu/drm/i915/i915_getparam.c
>>> +++ b/drivers/gpu/drm/i915/i915_getparam.c
>>> @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
>>>        case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
>>>        case I915_PARAM_HAS_EXEC_SUBMIT_FENCE:
>>>        case I915_PARAM_HAS_EXEC_TIMELINE_FENCES:
>>> +     case I915_PARAM_HAS_USERPTR_PROBE:
>>>                /* For the time being all of these are always true;
>>>                 * if some supported hardware does not have one of these
>>>                 * features this value needs to be provided from
>>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>>> index 975087553ea0..0d290535a6e5 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 {
>>> @@ -2222,12 +2225,29 @@ 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, and also doesn't
>>> +      * guarantee that the object will remain valid when the object is
>>> +      * eventually used.
>>> +      *
>>> +      * 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.
>>
>> Could we use _VALIDATE instead of probe? Or at least pin the pages as well, so we don't have to do it later?
> 
> I only care that the name matches what it does.  _VALIDATE sounds like
> it does a full validation of everything such that, if the import
> succeeds, execbuf will as well.  If we pin the pages at the same time,
> maybe that's true?  _PROBE, on the other hand, sounds a lot more like

No it is not possible to guarantee backing store remains valid until 
execbuf.

> a one-time best-effort check which may race with other stuff and
> doesn't guarantee future success.  That's in line with what the
> current patch does.
> 
>> We already have i915_gem_object_userptr_validate, no need to dupe it.
> 
> I have no opinion on this.

I was actually suggesting the same as Maarten here - that we should add 
a "populate" flag. But opinion was that was not desired - please look 
for the older threads to see the reasoning there.

Regards,

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation
  2021-07-26 16:10     ` Tvrtko Ursulin
@ 2021-07-28 14:22       ` Matthew Auld
  2021-08-03 15:09         ` Daniel Vetter
  0 siblings, 1 reply; 16+ messages in thread
From: Matthew Auld @ 2021-07-28 14:22 UTC (permalink / raw)
  To: Tvrtko Ursulin
  Cc: Thomas Hellström, Daniel Vetter, Intel GFX,
	Maling list - DRI developers, Chris Wilson, Kenneth Graunke,
	Matthew Auld

On Mon, 26 Jul 2021 at 17:10, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 26/07/2021 16:14, Jason Ekstrand wrote:
> > On Mon, Jul 26, 2021 at 3:31 AM Maarten Lankhorst
> > <maarten.lankhorst@linux.intel.com> wrote:
> >>
> >> Op 23-07-2021 om 13:34 schreef Matthew Auld:
> >>> From: Chris Wilson <chris@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 we 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 PROBE flag, so userspace can easily
> >>>    check if the kernel supports it(Jason).
> >>> - use mmap_read_{lock, unlock}.
> >>> - add some kernel-doc.
> >>> v3:
> >>> - In the docs also mention that PROBE doesn't guarantee that the pages
> >>>    will remain valid by the time they are actually used(Tvrtko).
> >>> - Add a small comment for the hole finding logic(Jason).
> >>> - Move the param next to all the other params which just return true.
> >>>
> >>> Testcase: igt/gem_userptr_blits/probe
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> >>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> >>> Cc: Jordan Justen <jordan.l.justen@intel.com>
> >>> Cc: Kenneth Graunke <kenneth@whitecape.org>
> >>> Cc: Jason Ekstrand <jason@jlekstrand.net>
> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>> Cc: Ramalingam C <ramalingam.c@intel.com>
> >>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> Acked-by: Kenneth Graunke <kenneth@whitecape.org>
> >>> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
> >>> ---
> >>>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++-
> >>>   drivers/gpu/drm/i915/i915_getparam.c        |  1 +
> >>>   include/uapi/drm/i915_drm.h                 | 20 ++++++++++
> >>>   3 files changed, 61 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..468a7a617fbf 100644
> >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> >>> @@ -422,6 +422,34 @@ 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) {
> >>> +             /* Check for holes, note that we also update the addr below */
> >>> +             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);
> >>> +
> >>> +     return ret;
> >>> +}
> >>> +
> >>>   /*
> >>>    * Creates a new mm object that wraps some normal memory from the process
> >>>    * context - user memory.
> >>> @@ -477,7 +505,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 +533,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..bbb7cac43eb4 100644
> >>> --- a/drivers/gpu/drm/i915/i915_getparam.c
> >>> +++ b/drivers/gpu/drm/i915/i915_getparam.c
> >>> @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
> >>>        case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
> >>>        case I915_PARAM_HAS_EXEC_SUBMIT_FENCE:
> >>>        case I915_PARAM_HAS_EXEC_TIMELINE_FENCES:
> >>> +     case I915_PARAM_HAS_USERPTR_PROBE:
> >>>                /* For the time being all of these are always true;
> >>>                 * if some supported hardware does not have one of these
> >>>                 * features this value needs to be provided from
> >>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> >>> index 975087553ea0..0d290535a6e5 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 {
> >>> @@ -2222,12 +2225,29 @@ 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, and also doesn't
> >>> +      * guarantee that the object will remain valid when the object is
> >>> +      * eventually used.
> >>> +      *
> >>> +      * 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.
> >>
> >> Could we use _VALIDATE instead of probe? Or at least pin the pages as well, so we don't have to do it later?
> >
> > I only care that the name matches what it does.  _VALIDATE sounds like
> > it does a full validation of everything such that, if the import
> > succeeds, execbuf will as well.  If we pin the pages at the same time,
> > maybe that's true?  _PROBE, on the other hand, sounds a lot more like
>
> No it is not possible to guarantee backing store remains valid until
> execbuf.
>
> > a one-time best-effort check which may race with other stuff and
> > doesn't guarantee future success.  That's in line with what the
> > current patch does.
> >
> >> We already have i915_gem_object_userptr_validate, no need to dupe it.
> >
> > I have no opinion on this.
>
> I was actually suggesting the same as Maarten here - that we should add
> a "populate" flag. But opinion was that was not desired - please look
> for the older threads to see the reasoning there.

So how should we proceed here? Maarten?

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation
  2021-07-28 14:22       ` Matthew Auld
@ 2021-08-03 15:09         ` Daniel Vetter
  2021-08-03 15:45           ` Jason Ekstrand
  0 siblings, 1 reply; 16+ messages in thread
From: Daniel Vetter @ 2021-08-03 15:09 UTC (permalink / raw)
  To: Matthew Auld
  Cc: Tvrtko Ursulin, Jason Ekstrand, Maarten Lankhorst,
	Thomas Hellström, Intel GFX, Maling list - DRI developers,
	Chris Wilson, Kenneth Graunke, Matthew Auld

On Wed, Jul 28, 2021 at 4:22 PM Matthew Auld
<matthew.william.auld@gmail.com> wrote:
>
> On Mon, 26 Jul 2021 at 17:10, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
> >
> >
> > On 26/07/2021 16:14, Jason Ekstrand wrote:
> > > On Mon, Jul 26, 2021 at 3:31 AM Maarten Lankhorst
> > > <maarten.lankhorst@linux.intel.com> wrote:
> > >>
> > >> Op 23-07-2021 om 13:34 schreef Matthew Auld:
> > >>> From: Chris Wilson <chris@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 we 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 PROBE flag, so userspace can easily
> > >>>    check if the kernel supports it(Jason).
> > >>> - use mmap_read_{lock, unlock}.
> > >>> - add some kernel-doc.
> > >>> v3:
> > >>> - In the docs also mention that PROBE doesn't guarantee that the pages
> > >>>    will remain valid by the time they are actually used(Tvrtko).
> > >>> - Add a small comment for the hole finding logic(Jason).
> > >>> - Move the param next to all the other params which just return true.
> > >>>
> > >>> Testcase: igt/gem_userptr_blits/probe
> > >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > >>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > >>> Cc: Jordan Justen <jordan.l.justen@intel.com>
> > >>> Cc: Kenneth Graunke <kenneth@whitecape.org>
> > >>> Cc: Jason Ekstrand <jason@jlekstrand.net>
> > >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >>> Cc: Ramalingam C <ramalingam.c@intel.com>
> > >>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > >>> Acked-by: Kenneth Graunke <kenneth@whitecape.org>
> > >>> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
> > >>> ---
> > >>>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++-
> > >>>   drivers/gpu/drm/i915/i915_getparam.c        |  1 +
> > >>>   include/uapi/drm/i915_drm.h                 | 20 ++++++++++
> > >>>   3 files changed, 61 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..468a7a617fbf 100644
> > >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > >>> @@ -422,6 +422,34 @@ 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) {
> > >>> +             /* Check for holes, note that we also update the addr below */
> > >>> +             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);
> > >>> +
> > >>> +     return ret;
> > >>> +}
> > >>> +
> > >>>   /*
> > >>>    * Creates a new mm object that wraps some normal memory from the process
> > >>>    * context - user memory.
> > >>> @@ -477,7 +505,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 +533,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..bbb7cac43eb4 100644
> > >>> --- a/drivers/gpu/drm/i915/i915_getparam.c
> > >>> +++ b/drivers/gpu/drm/i915/i915_getparam.c
> > >>> @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
> > >>>        case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
> > >>>        case I915_PARAM_HAS_EXEC_SUBMIT_FENCE:
> > >>>        case I915_PARAM_HAS_EXEC_TIMELINE_FENCES:
> > >>> +     case I915_PARAM_HAS_USERPTR_PROBE:
> > >>>                /* For the time being all of these are always true;
> > >>>                 * if some supported hardware does not have one of these
> > >>>                 * features this value needs to be provided from
> > >>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > >>> index 975087553ea0..0d290535a6e5 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 {
> > >>> @@ -2222,12 +2225,29 @@ 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, and also doesn't
> > >>> +      * guarantee that the object will remain valid when the object is
> > >>> +      * eventually used.
> > >>> +      *
> > >>> +      * 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.
> > >>
> > >> Could we use _VALIDATE instead of probe? Or at least pin the pages as well, so we don't have to do it later?
> > >
> > > I only care that the name matches what it does.  _VALIDATE sounds like
> > > it does a full validation of everything such that, if the import
> > > succeeds, execbuf will as well.  If we pin the pages at the same time,
> > > maybe that's true?  _PROBE, on the other hand, sounds a lot more like
> >
> > No it is not possible to guarantee backing store remains valid until
> > execbuf.
> >
> > > a one-time best-effort check which may race with other stuff and
> > > doesn't guarantee future success.  That's in line with what the
> > > current patch does.
> > >
> > >> We already have i915_gem_object_userptr_validate, no need to dupe it.
> > >
> > > I have no opinion on this.
> >
> > I was actually suggesting the same as Maarten here - that we should add
> > a "populate" flag. But opinion was that was not desired - please look
> > for the older threads to see the reasoning there.
>
> So how should we proceed here? Maarten?

I honestly don't care, and I think the probe flag here is perfectly
fine. Reasons for that:
- we don't have an immediate allocation flag for buffer creation
either. So if there's a need we need a flag for this across the board,
not just userptr, and a clear userspace ask
- it's a fundamentally racy test anyway, userspace can munmap or map
something else and then it will fail. So we really don't gain anything
by pinning pages because by the time we go into execbuf they might be
invalidated already - checking the vmas for VM_SPECIAL is perfectly
good enough.
- we can always change the implementation later on too.

Hence why I think PROBE is the semantics we want/need here. Can we get
some acks/reviews here or is this really a significant enough bikeshed
that we need to hold up dg1 pciids for them?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation
  2021-08-03 15:09         ` Daniel Vetter
@ 2021-08-03 15:45           ` Jason Ekstrand
  2021-08-03 15:57             ` Maarten Lankhorst
  0 siblings, 1 reply; 16+ messages in thread
From: Jason Ekstrand @ 2021-08-03 15:45 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Matthew Auld, Tvrtko Ursulin, Maarten Lankhorst,
	Thomas Hellström, Intel GFX, Maling list - DRI developers,
	Chris Wilson, Kenneth Graunke, Matthew Auld

On Tue, Aug 3, 2021 at 10:09 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Wed, Jul 28, 2021 at 4:22 PM Matthew Auld
> <matthew.william.auld@gmail.com> wrote:
> >
> > On Mon, 26 Jul 2021 at 17:10, Tvrtko Ursulin
> > <tvrtko.ursulin@linux.intel.com> wrote:
> > >
> > >
> > > On 26/07/2021 16:14, Jason Ekstrand wrote:
> > > > On Mon, Jul 26, 2021 at 3:31 AM Maarten Lankhorst
> > > > <maarten.lankhorst@linux.intel.com> wrote:
> > > >>
> > > >> Op 23-07-2021 om 13:34 schreef Matthew Auld:
> > > >>> From: Chris Wilson <chris@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 we 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 PROBE flag, so userspace can easily
> > > >>>    check if the kernel supports it(Jason).
> > > >>> - use mmap_read_{lock, unlock}.
> > > >>> - add some kernel-doc.
> > > >>> v3:
> > > >>> - In the docs also mention that PROBE doesn't guarantee that the pages
> > > >>>    will remain valid by the time they are actually used(Tvrtko).
> > > >>> - Add a small comment for the hole finding logic(Jason).
> > > >>> - Move the param next to all the other params which just return true.
> > > >>>
> > > >>> Testcase: igt/gem_userptr_blits/probe
> > > >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > > >>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > > >>> Cc: Jordan Justen <jordan.l.justen@intel.com>
> > > >>> Cc: Kenneth Graunke <kenneth@whitecape.org>
> > > >>> Cc: Jason Ekstrand <jason@jlekstrand.net>
> > > >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > >>> Cc: Ramalingam C <ramalingam.c@intel.com>
> > > >>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > >>> Acked-by: Kenneth Graunke <kenneth@whitecape.org>
> > > >>> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
> > > >>> ---
> > > >>>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++-
> > > >>>   drivers/gpu/drm/i915/i915_getparam.c        |  1 +
> > > >>>   include/uapi/drm/i915_drm.h                 | 20 ++++++++++
> > > >>>   3 files changed, 61 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..468a7a617fbf 100644
> > > >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > > >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > > >>> @@ -422,6 +422,34 @@ 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) {
> > > >>> +             /* Check for holes, note that we also update the addr below */
> > > >>> +             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);
> > > >>> +
> > > >>> +     return ret;
> > > >>> +}
> > > >>> +
> > > >>>   /*
> > > >>>    * Creates a new mm object that wraps some normal memory from the process
> > > >>>    * context - user memory.
> > > >>> @@ -477,7 +505,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 +533,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..bbb7cac43eb4 100644
> > > >>> --- a/drivers/gpu/drm/i915/i915_getparam.c
> > > >>> +++ b/drivers/gpu/drm/i915/i915_getparam.c
> > > >>> @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
> > > >>>        case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
> > > >>>        case I915_PARAM_HAS_EXEC_SUBMIT_FENCE:
> > > >>>        case I915_PARAM_HAS_EXEC_TIMELINE_FENCES:
> > > >>> +     case I915_PARAM_HAS_USERPTR_PROBE:
> > > >>>                /* For the time being all of these are always true;
> > > >>>                 * if some supported hardware does not have one of these
> > > >>>                 * features this value needs to be provided from
> > > >>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > > >>> index 975087553ea0..0d290535a6e5 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 {
> > > >>> @@ -2222,12 +2225,29 @@ 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, and also doesn't
> > > >>> +      * guarantee that the object will remain valid when the object is
> > > >>> +      * eventually used.
> > > >>> +      *
> > > >>> +      * 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.
> > > >>
> > > >> Could we use _VALIDATE instead of probe? Or at least pin the pages as well, so we don't have to do it later?
> > > >
> > > > I only care that the name matches what it does.  _VALIDATE sounds like
> > > > it does a full validation of everything such that, if the import
> > > > succeeds, execbuf will as well.  If we pin the pages at the same time,
> > > > maybe that's true?  _PROBE, on the other hand, sounds a lot more like
> > >
> > > No it is not possible to guarantee backing store remains valid until
> > > execbuf.
> > >
> > > > a one-time best-effort check which may race with other stuff and
> > > > doesn't guarantee future success.  That's in line with what the
> > > > current patch does.
> > > >
> > > >> We already have i915_gem_object_userptr_validate, no need to dupe it.
> > > >
> > > > I have no opinion on this.
> > >
> > > I was actually suggesting the same as Maarten here - that we should add
> > > a "populate" flag. But opinion was that was not desired - please look
> > > for the older threads to see the reasoning there.
> >
> > So how should we proceed here? Maarten?
>
> I honestly don't care, and I think the probe flag here is perfectly
> fine. Reasons for that:
> - we don't have an immediate allocation flag for buffer creation
> either. So if there's a need we need a flag for this across the board,
> not just userptr, and a clear userspace ask

Both Mesa drivers would probably set that flag if we had it and it
demonstrated any perf benefits, FWIW.  However, I think it's fine if
that's a separate flag.  Also, I don't know that the perf benefits are
all that great.  We should get most of those benefits from VM_BIND
anyway.

> - it's a fundamentally racy test anyway, userspace can munmap or map
> something else and then it will fail. So we really don't gain anything
> by pinning pages because by the time we go into execbuf they might be
> invalidated already - checking the vmas for VM_SPECIAL is perfectly
> good enough.
> - we can always change the implementation later on too.
>
> Hence why I think PROBE is the semantics we want/need here. Can we get
> some acks/reviews here or is this really a significant enough bikeshed
> that we need to hold up dg1 pciids for them?

I don't care.  I've already reviewed the patch.

--Jason

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation
  2021-08-03 15:45           ` Jason Ekstrand
@ 2021-08-03 15:57             ` Maarten Lankhorst
  2021-08-05 10:14               ` Maarten Lankhorst
  0 siblings, 1 reply; 16+ messages in thread
From: Maarten Lankhorst @ 2021-08-03 15:57 UTC (permalink / raw)
  To: Jason Ekstrand, Daniel Vetter
  Cc: Matthew Auld, Tvrtko Ursulin, Thomas Hellström, Intel GFX,
	Maling list - DRI developers, Chris Wilson, Kenneth Graunke,
	Matthew Auld

Op 2021-08-03 om 17:45 schreef Jason Ekstrand:
> On Tue, Aug 3, 2021 at 10:09 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> On Wed, Jul 28, 2021 at 4:22 PM Matthew Auld
>> <matthew.william.auld@gmail.com> wrote:
>>> On Mon, 26 Jul 2021 at 17:10, Tvrtko Ursulin
>>> <tvrtko.ursulin@linux.intel.com> wrote:
>>>>
>>>> On 26/07/2021 16:14, Jason Ekstrand wrote:
>>>>> On Mon, Jul 26, 2021 at 3:31 AM Maarten Lankhorst
>>>>> <maarten.lankhorst@linux.intel.com> wrote:
>>>>>> Op 23-07-2021 om 13:34 schreef Matthew Auld:
>>>>>>> From: Chris Wilson <chris@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 we 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 PROBE flag, so userspace can easily
>>>>>>>    check if the kernel supports it(Jason).
>>>>>>> - use mmap_read_{lock, unlock}.
>>>>>>> - add some kernel-doc.
>>>>>>> v3:
>>>>>>> - In the docs also mention that PROBE doesn't guarantee that the pages
>>>>>>>    will remain valid by the time they are actually used(Tvrtko).
>>>>>>> - Add a small comment for the hole finding logic(Jason).
>>>>>>> - Move the param next to all the other params which just return true.
>>>>>>>
>>>>>>> Testcase: igt/gem_userptr_blits/probe
>>>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>>>>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>>>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>>>>> Cc: Kenneth Graunke <kenneth@whitecape.org>
>>>>>>> Cc: Jason Ekstrand <jason@jlekstrand.net>
>>>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>>> Cc: Ramalingam C <ramalingam.c@intel.com>
>>>>>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>> Acked-by: Kenneth Graunke <kenneth@whitecape.org>
>>>>>>> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++-
>>>>>>>   drivers/gpu/drm/i915/i915_getparam.c        |  1 +
>>>>>>>   include/uapi/drm/i915_drm.h                 | 20 ++++++++++
>>>>>>>   3 files changed, 61 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..468a7a617fbf 100644
>>>>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>>>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>>>>>> @@ -422,6 +422,34 @@ 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) {
>>>>>>> +             /* Check for holes, note that we also update the addr below */
>>>>>>> +             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);
>>>>>>> +
>>>>>>> +     return ret;
>>>>>>> +}
>>>>>>> +
>>>>>>>   /*
>>>>>>>    * Creates a new mm object that wraps some normal memory from the process
>>>>>>>    * context - user memory.
>>>>>>> @@ -477,7 +505,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 +533,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..bbb7cac43eb4 100644
>>>>>>> --- a/drivers/gpu/drm/i915/i915_getparam.c
>>>>>>> +++ b/drivers/gpu/drm/i915/i915_getparam.c
>>>>>>> @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
>>>>>>>        case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
>>>>>>>        case I915_PARAM_HAS_EXEC_SUBMIT_FENCE:
>>>>>>>        case I915_PARAM_HAS_EXEC_TIMELINE_FENCES:
>>>>>>> +     case I915_PARAM_HAS_USERPTR_PROBE:
>>>>>>>                /* For the time being all of these are always true;
>>>>>>>                 * if some supported hardware does not have one of these
>>>>>>>                 * features this value needs to be provided from
>>>>>>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>>>>>>> index 975087553ea0..0d290535a6e5 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 {
>>>>>>> @@ -2222,12 +2225,29 @@ 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, and also doesn't
>>>>>>> +      * guarantee that the object will remain valid when the object is
>>>>>>> +      * eventually used.
>>>>>>> +      *
>>>>>>> +      * 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.
>>>>>> Could we use _VALIDATE instead of probe? Or at least pin the pages as well, so we don't have to do it later?
>>>>> I only care that the name matches what it does.  _VALIDATE sounds like
>>>>> it does a full validation of everything such that, if the import
>>>>> succeeds, execbuf will as well.  If we pin the pages at the same time,
>>>>> maybe that's true?  _PROBE, on the other hand, sounds a lot more like
>>>> No it is not possible to guarantee backing store remains valid until
>>>> execbuf.
>>>>
>>>>> a one-time best-effort check which may race with other stuff and
>>>>> doesn't guarantee future success.  That's in line with what the
>>>>> current patch does.
>>>>>
>>>>>> We already have i915_gem_object_userptr_validate, no need to dupe it.
>>>>> I have no opinion on this.
>>>> I was actually suggesting the same as Maarten here - that we should add
>>>> a "populate" flag. But opinion was that was not desired - please look
>>>> for the older threads to see the reasoning there.
>>> So how should we proceed here? Maarten?
>> I honestly don't care, and I think the probe flag here is perfectly
>> fine. Reasons for that:
>> - we don't have an immediate allocation flag for buffer creation
>> either. So if there's a need we need a flag for this across the board,
>> not just userptr, and a clear userspace ask
> Both Mesa drivers would probably set that flag if we had it and it
> demonstrated any perf benefits, FWIW.  However, I think it's fine if
> that's a separate flag.  Also, I don't know that the perf benefits are
> all that great.  We should get most of those benefits from VM_BIND
> anyway.
>
>> - it's a fundamentally racy test anyway, userspace can munmap or map
>> something else and then it will fail. So we really don't gain anything
>> by pinning pages because by the time we go into execbuf they might be
>> invalidated already - checking the vmas for VM_SPECIAL is perfectly
>> good enough.
>> - we can always change the implementation later on too.
>>
>> Hence why I think PROBE is the semantics we want/need here. Can we get
>> some acks/reviews here or is this really a significant enough bikeshed
>> that we need to hold up dg1 pciids for them?
> I don't care.  I've already reviewed the patch.
>
> --Jason

I think we should still just put the validate() call in there, but I'm not going to hold up the implementation because of that.

Acked-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation
  2021-08-03 15:57             ` Maarten Lankhorst
@ 2021-08-05 10:14               ` Maarten Lankhorst
  0 siblings, 0 replies; 16+ messages in thread
From: Maarten Lankhorst @ 2021-08-05 10:14 UTC (permalink / raw)
  To: Jason Ekstrand, Daniel Vetter
  Cc: Matthew Auld, Tvrtko Ursulin, Thomas Hellström, Intel GFX,
	Maling list - DRI developers, Chris Wilson, Kenneth Graunke,
	Matthew Auld

Op 03-08-2021 om 17:57 schreef Maarten Lankhorst:
> Op 2021-08-03 om 17:45 schreef Jason Ekstrand:
>> On Tue, Aug 3, 2021 at 10:09 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>> On Wed, Jul 28, 2021 at 4:22 PM Matthew Auld
>>> <matthew.william.auld@gmail.com> wrote:
>>>> On Mon, 26 Jul 2021 at 17:10, Tvrtko Ursulin
>>>> <tvrtko.ursulin@linux.intel.com> wrote:
>>>>> On 26/07/2021 16:14, Jason Ekstrand wrote:
>>>>>> On Mon, Jul 26, 2021 at 3:31 AM Maarten Lankhorst
>>>>>> <maarten.lankhorst@linux.intel.com> wrote:
>>>>>>> Op 23-07-2021 om 13:34 schreef Matthew Auld:
>>>>>>>> From: Chris Wilson <chris@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 we 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 PROBE flag, so userspace can easily
>>>>>>>>    check if the kernel supports it(Jason).
>>>>>>>> - use mmap_read_{lock, unlock}.
>>>>>>>> - add some kernel-doc.
>>>>>>>> v3:
>>>>>>>> - In the docs also mention that PROBE doesn't guarantee that the pages
>>>>>>>>    will remain valid by the time they are actually used(Tvrtko).
>>>>>>>> - Add a small comment for the hole finding logic(Jason).
>>>>>>>> - Move the param next to all the other params which just return true.
>>>>>>>>
>>>>>>>> Testcase: igt/gem_userptr_blits/probe
>>>>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>>>>>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>>>>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>>>>>> Cc: Kenneth Graunke <kenneth@whitecape.org>
>>>>>>>> Cc: Jason Ekstrand <jason@jlekstrand.net>
>>>>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>>>> Cc: Ramalingam C <ramalingam.c@intel.com>
>>>>>>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>>> Acked-by: Kenneth Graunke <kenneth@whitecape.org>
>>>>>>>> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
>>>>>>>> ---
>>>>>>>>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++-
>>>>>>>>   drivers/gpu/drm/i915/i915_getparam.c        |  1 +
>>>>>>>>   include/uapi/drm/i915_drm.h                 | 20 ++++++++++
>>>>>>>>   3 files changed, 61 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..468a7a617fbf 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>>>>>>> @@ -422,6 +422,34 @@ 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) {
>>>>>>>> +             /* Check for holes, note that we also update the addr below */
>>>>>>>> +             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);
>>>>>>>> +
>>>>>>>> +     return ret;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>   /*
>>>>>>>>    * Creates a new mm object that wraps some normal memory from the process
>>>>>>>>    * context - user memory.
>>>>>>>> @@ -477,7 +505,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 +533,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..bbb7cac43eb4 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/i915_getparam.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/i915_getparam.c
>>>>>>>> @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
>>>>>>>>        case I915_PARAM_HAS_EXEC_FENCE_ARRAY:
>>>>>>>>        case I915_PARAM_HAS_EXEC_SUBMIT_FENCE:
>>>>>>>>        case I915_PARAM_HAS_EXEC_TIMELINE_FENCES:
>>>>>>>> +     case I915_PARAM_HAS_USERPTR_PROBE:
>>>>>>>>                /* For the time being all of these are always true;
>>>>>>>>                 * if some supported hardware does not have one of these
>>>>>>>>                 * features this value needs to be provided from
>>>>>>>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>>>>>>>> index 975087553ea0..0d290535a6e5 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 {
>>>>>>>> @@ -2222,12 +2225,29 @@ 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, and also doesn't
>>>>>>>> +      * guarantee that the object will remain valid when the object is
>>>>>>>> +      * eventually used.
>>>>>>>> +      *
>>>>>>>> +      * 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.
>>>>>>> Could we use _VALIDATE instead of probe? Or at least pin the pages as well, so we don't have to do it later?
>>>>>> I only care that the name matches what it does.  _VALIDATE sounds like
>>>>>> it does a full validation of everything such that, if the import
>>>>>> succeeds, execbuf will as well.  If we pin the pages at the same time,
>>>>>> maybe that's true?  _PROBE, on the other hand, sounds a lot more like
>>>>> No it is not possible to guarantee backing store remains valid until
>>>>> execbuf.
>>>>>
>>>>>> a one-time best-effort check which may race with other stuff and
>>>>>> doesn't guarantee future success.  That's in line with what the
>>>>>> current patch does.
>>>>>>
>>>>>>> We already have i915_gem_object_userptr_validate, no need to dupe it.
>>>>>> I have no opinion on this.
>>>>> I was actually suggesting the same as Maarten here - that we should add
>>>>> a "populate" flag. But opinion was that was not desired - please look
>>>>> for the older threads to see the reasoning there.
>>>> So how should we proceed here? Maarten?
>>> I honestly don't care, and I think the probe flag here is perfectly
>>> fine. Reasons for that:
>>> - we don't have an immediate allocation flag for buffer creation
>>> either. So if there's a need we need a flag for this across the board,
>>> not just userptr, and a clear userspace ask
>> Both Mesa drivers would probably set that flag if we had it and it
>> demonstrated any perf benefits, FWIW.  However, I think it's fine if
>> that's a separate flag.  Also, I don't know that the perf benefits are
>> all that great.  We should get most of those benefits from VM_BIND
>> anyway.
>>
>>> - it's a fundamentally racy test anyway, userspace can munmap or map
>>> something else and then it will fail. So we really don't gain anything
>>> by pinning pages because by the time we go into execbuf they might be
>>> invalidated already - checking the vmas for VM_SPECIAL is perfectly
>>> good enough.
>>> - we can always change the implementation later on too.
>>>
>>> Hence why I think PROBE is the semantics we want/need here. Can we get
>>> some acks/reviews here or is this really a significant enough bikeshed
>>> that we need to hold up dg1 pciids for them?
>> I don't care.  I've already reviewed the patch.
>>
>> --Jason
> I think we should still just put the validate() call in there, but I'm not going to hold up the implementation because of that.
>
> Acked-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
And pushed together with the IGT. :)


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2021-08-05 10:14 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23 11:34 [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation Matthew Auld
2021-07-23 16:50 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
2021-07-23 17:47 ` [Intel-gfx] [PATCH] " Jason Ekstrand
2021-07-23 17:49   ` Jason Ekstrand
2021-07-26  8:03     ` Matthew Auld
2021-07-26  8:06   ` Matthew Auld
2021-07-26 15:12     ` Jason Ekstrand
2021-07-24  1:38 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork
2021-07-26  8:31 ` [Intel-gfx] [PATCH] " Maarten Lankhorst
2021-07-26 15:14   ` Jason Ekstrand
2021-07-26 16:10     ` Tvrtko Ursulin
2021-07-28 14:22       ` Matthew Auld
2021-08-03 15:09         ` Daniel Vetter
2021-08-03 15:45           ` Jason Ekstrand
2021-08-03 15:57             ` Maarten Lankhorst
2021-08-05 10:14               ` Maarten Lankhorst

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).