[Intel-gfx] [PATCH 0/4] Some DG1 uAPI cleanup

[Intel-gfx] [PATCH 0/4] Some DG1 uAPI cleanup

[Matthew Auld]

Test-with: 20210715100932.2605648-1-matthew.auld@intel.com

Chris Wilson (1):
  drm/i915/userptr: Probe existence of backing struct pages upon
    creation

Matthew Auld (3):
  drm/i915/uapi: reject caching ioctls for discrete
  drm/i915/uapi: convert drm_i915_gem_userptr to kernel doc
  drm/i915/uapi: reject set_domain for discrete

 drivers/gpu/drm/i915/gem/i915_gem_domain.c  |   9 ++
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c |  40 +++++++-
 drivers/gpu/drm/i915/i915_getparam.c        |   3 +
 include/uapi/drm/i915_drm.h                 | 106 +++++++++++++++++++-
 4 files changed, 156 insertions(+), 2 deletions(-)

-- 
2.26.3

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

[Intel-gfx] [PATCH 1/4] drm/i915/uapi: reject caching ioctls for discrete

[Matthew Auld]

It's a noop on DG1, and in the future when need to support other devices
which let us control the coherency, then it should be an immutable
creation time property for the BO. This will likely be controlled
through a new gem_create_ext extension.

v2: add some kernel doc for the discrete changes, and document the
    implicit rules

Suggested-by: Daniel Vetter <daniel@ffwll.ch>
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: Ramalingam C <ramalingam.c@intel.com>
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
---
 drivers/gpu/drm/i915/gem/i915_gem_domain.c |  6 +++++
 include/uapi/drm/i915_drm.h                | 29 ++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
index 7d1400b13429..43004bef55cb 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
@@ -268,6 +268,9 @@ int i915_gem_get_caching_ioctl(struct drm_device *dev, void *data,
 	struct drm_i915_gem_object *obj;
 	int err = 0;
 
+	if (IS_DGFX(to_i915(dev)))
+		return -ENODEV;
+
 	rcu_read_lock();
 	obj = i915_gem_object_lookup_rcu(file, args->handle);
 	if (!obj) {
@@ -303,6 +306,9 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
 	enum i915_cache_level level;
 	int ret = 0;
 
+	if (IS_DGFX(i915))
+		return -ENODEV;
+
 	switch (args->caching) {
 	case I915_CACHING_NONE:
 		level = I915_CACHE_NONE;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index e54f9efaead0..868c2ee7be60 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1395,6 +1395,35 @@ struct drm_i915_gem_busy {
  * ppGTT support, or if the object is used for scanout). Note that this might
  * require unbinding the object from the GTT first, if its current caching value
  * doesn't match.
+ *
+ * Note that this all changes on discrete platforms, starting from DG1, the
+ * set/get caching is no longer supported, and is now rejected.  Instead the CPU
+ * caching attributes(WB vs WC) will become an immutable creation time property
+ * for the object, along with the GTT caching level. For now we don't expose any
+ * new uAPI for this, instead on DG1 this is all implicit, although this largely
+ * shouldn't matter since DG1 is coherent by default(without any way of
+ * controlling it).
+ *
+ * Implicit caching rules, starting from DG1:
+ *
+ *     - If any of the object placements (see &drm_i915_gem_create_ext_memory_regions)
+ *       contain I915_MEMORY_CLASS_DEVICE then the object will be allocated and
+ *       mapped as write-combined only.
+ *
+ *     - Everything else is always allocated and mapped as write-back, with the
+ *       guarantee that everything is also coherent with the GPU.
+ *
+ * Note that this is likely to change in the future again, where we might need
+ * more flexibility on future devices, so making this all explicit as part of a
+ * new &drm_i915_gem_create_ext extension is probable.
+ *
+ * Side note: Part of the reason for this is that changing the at-allocation-time CPU
+ * caching attributes for the pages might be required(and is expensive) if we
+ * need to then CPU map the pages later with different caching attributes. This
+ * inconsistent caching behaviour, while supported on x86, is not universally
+ * supported on other architectures. So for simplicity we opt for setting
+ * everything at creation time, whilst also making it immutable, on discrete
+ * platforms.
  */
 struct drm_i915_gem_caching {
 	/**
-- 
2.26.3

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

[Intel-gfx] [PATCH 2/4] drm/i915/uapi: convert drm_i915_gem_userptr to kernel doc

[Matthew Auld]

Add the missing kernel-doc.

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>
---
 include/uapi/drm/i915_drm.h | 40 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 868c2ee7be60..e20eeeca7a1c 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -2141,14 +2141,52 @@ struct drm_i915_reset_stats {
 	__u32 pad;
 };
 
+/**
+ * struct drm_i915_gem_userptr - Create GEM object from user allocated memory.
+ *
+ * Userptr objects have several restrictions on what ioctls can be used with the
+ * object handle.
+ */
 struct drm_i915_gem_userptr {
+	/**
+	 * @user_ptr: The pointer to the allocated memory.
+	 *
+	 * Needs to be aligned to PAGE_SIZE.
+	 */
 	__u64 user_ptr;
+
+	/**
+	 * @user_size:
+	 *
+	 * The size in bytes for the allocated memory. This will also become the
+	 * object size.
+	 *
+	 * Needs to be aligned to PAGE_SIZE, and should be at least PAGE_SIZE,
+	 * or larger.
+	 */
 	__u64 user_size;
+
+	/**
+	 * @flags:
+	 *
+	 * Supported flags:
+	 *
+	 * I915_USERPTR_READ_ONLY:
+	 *
+	 * Mark the object as readonly, this also means GPU access can only be
+	 * readonly. This is only supported on HW which supports readonly access
+	 * through the GTT. If the HW can't support readonly access, an error is
+	 * returned.
+	 *
+	 * I915_USERPTR_UNSYNCHRONIZED:
+	 *
+	 * NOT USED. Setting this flag will result in an error.
+	 */
 	__u32 flags;
 #define I915_USERPTR_READ_ONLY 0x1
 #define I915_USERPTR_UNSYNCHRONIZED 0x80000000
 	/**
-	 * Returned handle for the object.
+	 * @handle: Returned handle for the object.
 	 *
 	 * Object handles are nonzero.
 	 */
-- 
2.26.3

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

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

[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 are going to drop support for set_domain(), so offering a
way to probe the pages, without having to resort to dummy batches has
been requested.

v2:
- add new query param for the PROPBE flag, so userspace can easily
  check if the kernel supports it(Jason).
- use mmap_read_{lock, unlock}.
- add some kernel-doc.

Testcase: igt/gem_userptr_blits/probe
Signed-off-by: Chris Wilson <chris@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>
---
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_getparam.c        |  3 ++
 include/uapi/drm/i915_drm.h                 | 18 ++++++++++
 3 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index 56edfeff8c02..fd6880328596 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
 
 #endif
 
+static int
+probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
+{
+	const unsigned long end = addr + len;
+	struct vm_area_struct *vma;
+	int ret = -EFAULT;
+
+	mmap_read_lock(mm);
+	for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
+		if (vma->vm_start > addr)
+			break;
+
+		if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
+			break;
+
+		if (vma->vm_end >= end) {
+			ret = 0;
+			break;
+		}
+
+		addr = vma->vm_end;
+	}
+	mmap_read_unlock(mm);
+
+	return ret;
+}
+
 /*
  * Creates a new mm object that wraps some normal memory from the process
  * context - user memory.
@@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
 	}
 
 	if (args->flags & ~(I915_USERPTR_READ_ONLY |
-			    I915_USERPTR_UNSYNCHRONIZED))
+			    I915_USERPTR_UNSYNCHRONIZED |
+			    I915_USERPTR_PROBE))
 		return -EINVAL;
 
 	if (i915_gem_object_size_2big(args->user_size))
@@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
 			return -ENODEV;
 	}
 
+	if (args->flags & I915_USERPTR_PROBE) {
+		/*
+		 * Check that the range pointed to represents real struct
+		 * pages and not iomappings (at this moment in time!)
+		 */
+		ret = probe_range(current->mm, args->user_ptr, args->user_size);
+		if (ret)
+			return ret;
+	}
+
 #ifdef CONFIG_MMU_NOTIFIER
 	obj = i915_gem_object_alloc();
 	if (obj == NULL)
diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c
index 24e18219eb50..d6d2e1a10d14 100644
--- a/drivers/gpu/drm/i915/i915_getparam.c
+++ b/drivers/gpu/drm/i915/i915_getparam.c
@@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
 	case I915_PARAM_PERF_REVISION:
 		value = i915_perf_ioctl_version();
 		break;
+	case I915_PARAM_HAS_USERPTR_PROBE:
+		value = true;
+		break;
 	default:
 		DRM_DEBUG("Unknown parameter %d\n", param->param);
 		return -EINVAL;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index e20eeeca7a1c..2e4112bf4d38 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait {
  */
 #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55
 
+/* Query if the kernel supports the I915_USERPTR_PROBE flag. */
+#define I915_PARAM_HAS_USERPTR_PROBE 56
+
 /* Must be kept compact -- no holes and well documented */
 
 typedef struct drm_i915_getparam {
@@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr {
 	 * through the GTT. If the HW can't support readonly access, an error is
 	 * returned.
 	 *
+	 * I915_USERPTR_PROBE:
+	 *
+	 * Probe the provided @user_ptr range and validate that the @user_ptr is
+	 * indeed pointing to normal memory and that the range is also valid.
+	 * For example if some garbage address is given to the kernel, then this
+	 * should complain.
+	 *
+	 * Returns -EFAULT if the probe failed.
+	 *
+	 * Note that this doesn't populate the backing pages.
+	 *
+	 * 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] [PATCH 4/4] drm/i915/uapi: reject set_domain for discrete

[Matthew Auld]

The CPU domain should be static for discrete, and on DG1 we don't need
any flushing since everything is already coherent, so really all this
does is an object wait, for which we have an ioctl. Longer term the
desired caching should be an immutable creation time property for the
BO, which can be set with something like gem_create_ext.

One other user is iris + userptr, which uses the set_domain to probe all
the pages to check if the GUP succeeds, however we now have a PROBE
flag for this purpose.

v2: add some more kernel doc, also add the implicit rules with caching

Suggested-by: Daniel Vetter <daniel@ffwll.ch>
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: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_domain.c |  3 +++
 include/uapi/drm/i915_drm.h                | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
index 43004bef55cb..b684a62bf3b0 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
@@ -490,6 +490,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 	u32 write_domain = args->write_domain;
 	int err;
 
+	if (IS_DGFX(to_i915(dev)))
+		return -ENODEV;
+
 	/* Only handle setting domains to types used by the CPU. */
 	if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS)
 		return -EINVAL;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 2e4112bf4d38..04ce310e7ee6 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -901,6 +901,25 @@ struct drm_i915_gem_mmap_offset {
  *	- I915_GEM_DOMAIN_GTT: Mappable aperture domain
  *
  * All other domains are rejected.
+ *
+ * Note that for discrete, starting from DG1, this is no longer supported, and
+ * is instead rejected. On such platforms the CPU domain is effectively static,
+ * where we also only support a single &drm_i915_gem_mmap_offset cache mode,
+ * which can't be set explicitly and instead depends on the object placements,
+ * as per the below.
+ *
+ * Implicit caching rules, starting from DG1:
+ *
+ *	- If any of the object placements (see &drm_i915_gem_create_ext_memory_regions)
+ *	  contain I915_MEMORY_CLASS_DEVICE then the object will be allocated and
+ *	  mapped as write-combined only.
+ *
+ *	- Everything else is always allocated and mapped as write-back, with the
+ *	  guarantee that everything is also coherent with the GPU.
+ *
+ * Note that this is likely to change in the future again, where we might need
+ * more flexibility on future devices, so making this all explicit as part of a
+ * new &drm_i915_gem_create_ext extension is probable.
  */
 struct drm_i915_gem_set_domain {
 	/** @handle: Handle for the object. */
-- 
2.26.3

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

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

[Tvrtko Ursulin]

On 15/07/2021 11:15, Matthew Auld 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 are going to drop support for set_domain(), so offering a
> way to probe the pages, without having to resort to dummy batches has
> been requested.
> 
> v2:
> - add new query param for the PROPBE flag, so userspace can easily
>    check if the kernel supports it(Jason).
> - use mmap_read_{lock, unlock}.
> - add some kernel-doc.

1)

I think probing is too weak to be offered as part of the uapi. What 
probes successfully at create time might not be there anymore at usage 
time. So if the pointer is not trusted at one point, why should it be at 
a later stage?

Only thing which works for me is populate (so get_pages) at create time. 
But again with no guarantees they are still there at use time clearly 
documented.

2)

I am also not a fan of getparam for individual ioctl flags since I don't 
think it scales nicely. How about add a param which returns all 
supported flags like I915_PARAM_USERPTR_SUPPORTED_FLAGS?

Downside is it only works for 32-bit flag fields with getparam. Or it 
could be a query to solve that as well.

Regards,

Tvrtko

> 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>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++-
>   drivers/gpu/drm/i915/i915_getparam.c        |  3 ++
>   include/uapi/drm/i915_drm.h                 | 18 ++++++++++
>   3 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index 56edfeff8c02..fd6880328596 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
>   
>   #endif
>   
> +static int
> +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
> +{
> +	const unsigned long end = addr + len;
> +	struct vm_area_struct *vma;
> +	int ret = -EFAULT;
> +
> +	mmap_read_lock(mm);
> +	for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
> +		if (vma->vm_start > addr)
> +			break;
> +
> +		if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
> +			break;
> +
> +		if (vma->vm_end >= end) {
> +			ret = 0;
> +			break;
> +		}
> +
> +		addr = vma->vm_end;
> +	}
> +	mmap_read_unlock(mm);
> +
> +	return ret;
> +}
> +
>   /*
>    * Creates a new mm object that wraps some normal memory from the process
>    * context - user memory.
> @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
>   	}
>   
>   	if (args->flags & ~(I915_USERPTR_READ_ONLY |
> -			    I915_USERPTR_UNSYNCHRONIZED))
> +			    I915_USERPTR_UNSYNCHRONIZED |
> +			    I915_USERPTR_PROBE))
>   		return -EINVAL;
>   
>   	if (i915_gem_object_size_2big(args->user_size))
> @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
>   			return -ENODEV;
>   	}
>   
> +	if (args->flags & I915_USERPTR_PROBE) {
> +		/*
> +		 * Check that the range pointed to represents real struct
> +		 * pages and not iomappings (at this moment in time!)
> +		 */
> +		ret = probe_range(current->mm, args->user_ptr, args->user_size);
> +		if (ret)
> +			return ret;
> +	}
> +
>   #ifdef CONFIG_MMU_NOTIFIER
>   	obj = i915_gem_object_alloc();
>   	if (obj == NULL)
> diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c
> index 24e18219eb50..d6d2e1a10d14 100644
> --- a/drivers/gpu/drm/i915/i915_getparam.c
> +++ b/drivers/gpu/drm/i915/i915_getparam.c
> @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
>   	case I915_PARAM_PERF_REVISION:
>   		value = i915_perf_ioctl_version();
>   		break;
> +	case I915_PARAM_HAS_USERPTR_PROBE:
> +		value = true;
> +		break;
>   	default:
>   		DRM_DEBUG("Unknown parameter %d\n", param->param);
>   		return -EINVAL;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index e20eeeca7a1c..2e4112bf4d38 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait {
>    */
>   #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55
>   
> +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */
> +#define I915_PARAM_HAS_USERPTR_PROBE 56
> +
>   /* Must be kept compact -- no holes and well documented */
>   
>   typedef struct drm_i915_getparam {
> @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr {
>   	 * through the GTT. If the HW can't support readonly access, an error is
>   	 * returned.
>   	 *
> +	 * I915_USERPTR_PROBE:
> +	 *
> +	 * Probe the provided @user_ptr range and validate that the @user_ptr is
> +	 * indeed pointing to normal memory and that the range is also valid.
> +	 * For example if some garbage address is given to the kernel, then this
> +	 * should complain.
> +	 *
> +	 * Returns -EFAULT if the probe failed.
> +	 *
> +	 * Note that this doesn't populate the backing pages.
> +	 *
> +	 * 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.
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Thu, Jul 15, 2021 at 11:33:10AM +0100, Tvrtko Ursulin wrote:
> 
> On 15/07/2021 11:15, Matthew Auld 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 are going to drop support for set_domain(), so offering a
> > way to probe the pages, without having to resort to dummy batches has
> > been requested.
> > 
> > v2:
> > - add new query param for the PROPBE flag, so userspace can easily
> >    check if the kernel supports it(Jason).
> > - use mmap_read_{lock, unlock}.
> > - add some kernel-doc.
> 
> 1)
> 
> I think probing is too weak to be offered as part of the uapi. What probes
> successfully at create time might not be there anymore at usage time. So if
> the pointer is not trusted at one point, why should it be at a later stage?
> 
> Only thing which works for me is populate (so get_pages) at create time. But
> again with no guarantees they are still there at use time clearly
> documented.

Populate is exactly as racy as probe. We don't support pinned userptr
anymore.

> 2)
> 
> I am also not a fan of getparam for individual ioctl flags since I don't
> think it scales nicely. How about add a param which returns all supported
> flags like I915_PARAM_USERPTR_SUPPORTED_FLAGS?
> 
> Downside is it only works for 32-bit flag fields with getparam. Or it could
> be a query to solve that as well.

I expect the actual userspace code will simply try with the probe flag
first, and then without + set_domain. So strictly speaking we might not
even have a need for the getparam.

Otoh, let's not overthink/engineer this, whatever works for userspace is
ok imo. A new query that lists all flags is the kind of fake-generic stuff
that will like mis-estimate where the future actually lands, e.g. how do
you encode if we add extensions to userptr to this? Which is something we
absolutely can, by extending the struct at the end, which doesn't even
need a new flag.

Let's solve the immediate problem at hand, and not try to guess what more
problems we might have in the future.
-Daniel

> Regards,
> 
> Tvrtko
> 
> > 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>
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++-
> >   drivers/gpu/drm/i915/i915_getparam.c        |  3 ++
> >   include/uapi/drm/i915_drm.h                 | 18 ++++++++++
> >   3 files changed, 60 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > index 56edfeff8c02..fd6880328596 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
> >   #endif
> > +static int
> > +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
> > +{
> > +	const unsigned long end = addr + len;
> > +	struct vm_area_struct *vma;
> > +	int ret = -EFAULT;
> > +
> > +	mmap_read_lock(mm);
> > +	for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
> > +		if (vma->vm_start > addr)
> > +			break;
> > +
> > +		if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
> > +			break;
> > +
> > +		if (vma->vm_end >= end) {
> > +			ret = 0;
> > +			break;
> > +		}
> > +
> > +		addr = vma->vm_end;
> > +	}
> > +	mmap_read_unlock(mm);
> > +
> > +	return ret;
> > +}
> > +
> >   /*
> >    * Creates a new mm object that wraps some normal memory from the process
> >    * context - user memory.
> > @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
> >   	}
> >   	if (args->flags & ~(I915_USERPTR_READ_ONLY |
> > -			    I915_USERPTR_UNSYNCHRONIZED))
> > +			    I915_USERPTR_UNSYNCHRONIZED |
> > +			    I915_USERPTR_PROBE))
> >   		return -EINVAL;
> >   	if (i915_gem_object_size_2big(args->user_size))
> > @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
> >   			return -ENODEV;
> >   	}
> > +	if (args->flags & I915_USERPTR_PROBE) {
> > +		/*
> > +		 * Check that the range pointed to represents real struct
> > +		 * pages and not iomappings (at this moment in time!)
> > +		 */
> > +		ret = probe_range(current->mm, args->user_ptr, args->user_size);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> >   #ifdef CONFIG_MMU_NOTIFIER
> >   	obj = i915_gem_object_alloc();
> >   	if (obj == NULL)
> > diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c
> > index 24e18219eb50..d6d2e1a10d14 100644
> > --- a/drivers/gpu/drm/i915/i915_getparam.c
> > +++ b/drivers/gpu/drm/i915/i915_getparam.c
> > @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
> >   	case I915_PARAM_PERF_REVISION:
> >   		value = i915_perf_ioctl_version();
> >   		break;
> > +	case I915_PARAM_HAS_USERPTR_PROBE:
> > +		value = true;
> > +		break;
> >   	default:
> >   		DRM_DEBUG("Unknown parameter %d\n", param->param);
> >   		return -EINVAL;
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index e20eeeca7a1c..2e4112bf4d38 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait {
> >    */
> >   #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55
> > +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */
> > +#define I915_PARAM_HAS_USERPTR_PROBE 56
> > +
> >   /* Must be kept compact -- no holes and well documented */
> >   typedef struct drm_i915_getparam {
> > @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr {
> >   	 * through the GTT. If the HW can't support readonly access, an error is
> >   	 * returned.
> >   	 *
> > +	 * I915_USERPTR_PROBE:
> > +	 *
> > +	 * Probe the provided @user_ptr range and validate that the @user_ptr is
> > +	 * indeed pointing to normal memory and that the range is also valid.
> > +	 * For example if some garbage address is given to the kernel, then this
> > +	 * should complain.
> > +	 *
> > +	 * Returns -EFAULT if the probe failed.
> > +	 *
> > +	 * Note that this doesn't populate the backing pages.
> > +	 *
> > +	 * 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.
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Matthew Auld]

On Thu, 15 Jul 2021 at 11:33, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 15/07/2021 11:15, Matthew Auld 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 are going to drop support for set_domain(), so offering a
> > way to probe the pages, without having to resort to dummy batches has
> > been requested.
> >
> > v2:
> > - add new query param for the PROPBE flag, so userspace can easily
> >    check if the kernel supports it(Jason).
> > - use mmap_read_{lock, unlock}.
> > - add some kernel-doc.
>
> 1)
>
> I think probing is too weak to be offered as part of the uapi. What
> probes successfully at create time might not be there anymore at usage
> time. So if the pointer is not trusted at one point, why should it be at
> a later stage?
>
> Only thing which works for me is populate (so get_pages) at create time.
> But again with no guarantees they are still there at use time clearly
> documented.
>
> 2)
>
> I am also not a fan of getparam for individual ioctl flags since I don't
> think it scales nicely. How about add a param which returns all
> supported flags like I915_PARAM_USERPTR_SUPPORTED_FLAGS?
>
> Downside is it only works for 32-bit flag fields with getparam. Or it
> could be a query to solve that as well.

I guess. You don't think it's a little iffy though, since there were
other flags which were added before this? So effectively userspace
queries SUPPORTED_FLAGS and might get -EINVAL on older kernels, even
though the flag is supported on that kernel(like READONLY)?

Maybe a versioning scheme instead? I915_PARAM_USERPTR_VERSION? Seems
quite common for other params.

>
> Regards,
>
> Tvrtko
>
> > 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>
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++-
> >   drivers/gpu/drm/i915/i915_getparam.c        |  3 ++
> >   include/uapi/drm/i915_drm.h                 | 18 ++++++++++
> >   3 files changed, 60 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > index 56edfeff8c02..fd6880328596 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
> >
> >   #endif
> >
> > +static int
> > +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
> > +{
> > +     const unsigned long end = addr + len;
> > +     struct vm_area_struct *vma;
> > +     int ret = -EFAULT;
> > +
> > +     mmap_read_lock(mm);
> > +     for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
> > +             if (vma->vm_start > addr)
> > +                     break;
> > +
> > +             if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
> > +                     break;
> > +
> > +             if (vma->vm_end >= end) {
> > +                     ret = 0;
> > +                     break;
> > +             }
> > +
> > +             addr = vma->vm_end;
> > +     }
> > +     mmap_read_unlock(mm);
> > +
> > +     return ret;
> > +}
> > +
> >   /*
> >    * Creates a new mm object that wraps some normal memory from the process
> >    * context - user memory.
> > @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
> >       }
> >
> >       if (args->flags & ~(I915_USERPTR_READ_ONLY |
> > -                         I915_USERPTR_UNSYNCHRONIZED))
> > +                         I915_USERPTR_UNSYNCHRONIZED |
> > +                         I915_USERPTR_PROBE))
> >               return -EINVAL;
> >
> >       if (i915_gem_object_size_2big(args->user_size))
> > @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
> >                       return -ENODEV;
> >       }
> >
> > +     if (args->flags & I915_USERPTR_PROBE) {
> > +             /*
> > +              * Check that the range pointed to represents real struct
> > +              * pages and not iomappings (at this moment in time!)
> > +              */
> > +             ret = probe_range(current->mm, args->user_ptr, args->user_size);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> >   #ifdef CONFIG_MMU_NOTIFIER
> >       obj = i915_gem_object_alloc();
> >       if (obj == NULL)
> > diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c
> > index 24e18219eb50..d6d2e1a10d14 100644
> > --- a/drivers/gpu/drm/i915/i915_getparam.c
> > +++ b/drivers/gpu/drm/i915/i915_getparam.c
> > @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
> >       case I915_PARAM_PERF_REVISION:
> >               value = i915_perf_ioctl_version();
> >               break;
> > +     case I915_PARAM_HAS_USERPTR_PROBE:
> > +             value = true;
> > +             break;
> >       default:
> >               DRM_DEBUG("Unknown parameter %d\n", param->param);
> >               return -EINVAL;
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index e20eeeca7a1c..2e4112bf4d38 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait {
> >    */
> >   #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55
> >
> > +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */
> > +#define I915_PARAM_HAS_USERPTR_PROBE 56
> > +
> >   /* Must be kept compact -- no holes and well documented */
> >
> >   typedef struct drm_i915_getparam {
> > @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr {
> >        * through the GTT. If the HW can't support readonly access, an error is
> >        * returned.
> >        *
> > +      * I915_USERPTR_PROBE:
> > +      *
> > +      * Probe the provided @user_ptr range and validate that the @user_ptr is
> > +      * indeed pointing to normal memory and that the range is also valid.
> > +      * For example if some garbage address is given to the kernel, then this
> > +      * should complain.
> > +      *
> > +      * Returns -EFAULT if the probe failed.
> > +      *
> > +      * Note that this doesn't populate the backing pages.
> > +      *
> > +      * 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.
> >
> _______________________________________________
> 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

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

[Tvrtko Ursulin]

On 15/07/2021 12:07, Daniel Vetter wrote:
> On Thu, Jul 15, 2021 at 11:33:10AM +0100, Tvrtko Ursulin wrote:
>>
>> On 15/07/2021 11:15, Matthew Auld 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 are going to drop support for set_domain(), so offering a
>>> way to probe the pages, without having to resort to dummy batches has
>>> been requested.
>>>
>>> v2:
>>> - add new query param for the PROPBE flag, so userspace can easily
>>>     check if the kernel supports it(Jason).
>>> - use mmap_read_{lock, unlock}.
>>> - add some kernel-doc.
>>
>> 1)
>>
>> I think probing is too weak to be offered as part of the uapi. What probes
>> successfully at create time might not be there anymore at usage time. So if
>> the pointer is not trusted at one point, why should it be at a later stage?
>>
>> Only thing which works for me is populate (so get_pages) at create time. But
>> again with no guarantees they are still there at use time clearly
>> documented.
> 
> Populate is exactly as racy as probe. We don't support pinned userptr
> anymore.

Yes, wrote so myself - "..again with no guarantees they are still there 
at use time..".

Perhaps I don't understand what problem is probe supposed to solve. It 
doesn't deal 1:1 with set_domain removal since that one actually did 
get_pages so that would be populate. But fact remains regardless that if 
userspace is given a pointer it doesn't trust, _and_ wants the check it 
for this reason or that, then probe solves nothing. Unless there is 
actually at minimum some protocol to reply to whoever sent the pointer 
like "not that pointer please".

>> 2)
>>
>> I am also not a fan of getparam for individual ioctl flags since I don't
>> think it scales nicely. How about add a param which returns all supported
>> flags like I915_PARAM_USERPTR_SUPPORTED_FLAGS?
>>
>> Downside is it only works for 32-bit flag fields with getparam. Or it could
>> be a query to solve that as well.
> 
> I expect the actual userspace code will simply try with the probe flag
> first, and then without + set_domain. So strictly speaking we might not
> even have a need for the getparam.
> 
> Otoh, let's not overthink/engineer this, whatever works for userspace is
> ok imo. A new query that lists all flags is the kind of fake-generic stuff
> that will like mis-estimate where the future actually lands, e.g. how do
> you encode if we add extensions to userptr to this? Which is something we
> absolutely can, by extending the struct at the end, which doesn't even
> need a new flag.
> 
> Let's solve the immediate problem at hand, and not try to guess what more
> problems we might have in the future.

Yeah I am guessing if anyone is using some of the other userptr flags 
they don't have a getparam for that either. At least that would be 
consolidated with I915_PARAM_USERPTR_SUPPORTED_FLAGS. There is no 
requirement that getparam needs to keep supporting future extensions to 
the struct itself, equally as I915_PARAM_HAS_USERPTR_PROBE is directly 
tied to the same flags field, only a subset of it.

Regards,

Tvrtko

> -Daniel
> 
>> Regards,
>>
>> Tvrtko
>>
>>> 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>
>>> ---
>>>    drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++-
>>>    drivers/gpu/drm/i915/i915_getparam.c        |  3 ++
>>>    include/uapi/drm/i915_drm.h                 | 18 ++++++++++
>>>    3 files changed, 60 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>> index 56edfeff8c02..fd6880328596 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>> @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
>>>    #endif
>>> +static int
>>> +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
>>> +{
>>> +	const unsigned long end = addr + len;
>>> +	struct vm_area_struct *vma;
>>> +	int ret = -EFAULT;
>>> +
>>> +	mmap_read_lock(mm);
>>> +	for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
>>> +		if (vma->vm_start > addr)
>>> +			break;
>>> +
>>> +		if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
>>> +			break;
>>> +
>>> +		if (vma->vm_end >= end) {
>>> +			ret = 0;
>>> +			break;
>>> +		}
>>> +
>>> +		addr = vma->vm_end;
>>> +	}
>>> +	mmap_read_unlock(mm);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>    /*
>>>     * Creates a new mm object that wraps some normal memory from the process
>>>     * context - user memory.
>>> @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
>>>    	}
>>>    	if (args->flags & ~(I915_USERPTR_READ_ONLY |
>>> -			    I915_USERPTR_UNSYNCHRONIZED))
>>> +			    I915_USERPTR_UNSYNCHRONIZED |
>>> +			    I915_USERPTR_PROBE))
>>>    		return -EINVAL;
>>>    	if (i915_gem_object_size_2big(args->user_size))
>>> @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
>>>    			return -ENODEV;
>>>    	}
>>> +	if (args->flags & I915_USERPTR_PROBE) {
>>> +		/*
>>> +		 * Check that the range pointed to represents real struct
>>> +		 * pages and not iomappings (at this moment in time!)
>>> +		 */
>>> +		ret = probe_range(current->mm, args->user_ptr, args->user_size);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>>    #ifdef CONFIG_MMU_NOTIFIER
>>>    	obj = i915_gem_object_alloc();
>>>    	if (obj == NULL)
>>> diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c
>>> index 24e18219eb50..d6d2e1a10d14 100644
>>> --- a/drivers/gpu/drm/i915/i915_getparam.c
>>> +++ b/drivers/gpu/drm/i915/i915_getparam.c
>>> @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
>>>    	case I915_PARAM_PERF_REVISION:
>>>    		value = i915_perf_ioctl_version();
>>>    		break;
>>> +	case I915_PARAM_HAS_USERPTR_PROBE:
>>> +		value = true;
>>> +		break;
>>>    	default:
>>>    		DRM_DEBUG("Unknown parameter %d\n", param->param);
>>>    		return -EINVAL;
>>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>>> index e20eeeca7a1c..2e4112bf4d38 100644
>>> --- a/include/uapi/drm/i915_drm.h
>>> +++ b/include/uapi/drm/i915_drm.h
>>> @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait {
>>>     */
>>>    #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55
>>> +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */
>>> +#define I915_PARAM_HAS_USERPTR_PROBE 56
>>> +
>>>    /* Must be kept compact -- no holes and well documented */
>>>    typedef struct drm_i915_getparam {
>>> @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr {
>>>    	 * through the GTT. If the HW can't support readonly access, an error is
>>>    	 * returned.
>>>    	 *
>>> +	 * I915_USERPTR_PROBE:
>>> +	 *
>>> +	 * Probe the provided @user_ptr range and validate that the @user_ptr is
>>> +	 * indeed pointing to normal memory and that the range is also valid.
>>> +	 * For example if some garbage address is given to the kernel, then this
>>> +	 * should complain.
>>> +	 *
>>> +	 * Returns -EFAULT if the probe failed.
>>> +	 *
>>> +	 * Note that this doesn't populate the backing pages.
>>> +	 *
>>> +	 * 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.
>>>
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 15/07/2021 12:09, Matthew Auld wrote:
> On Thu, 15 Jul 2021 at 11:33, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>
>> On 15/07/2021 11:15, Matthew Auld 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 are going to drop support for set_domain(), so offering a
>>> way to probe the pages, without having to resort to dummy batches has
>>> been requested.
>>>
>>> v2:
>>> - add new query param for the PROPBE flag, so userspace can easily
>>>     check if the kernel supports it(Jason).
>>> - use mmap_read_{lock, unlock}.
>>> - add some kernel-doc.
>>
>> 1)
>>
>> I think probing is too weak to be offered as part of the uapi. What
>> probes successfully at create time might not be there anymore at usage
>> time. So if the pointer is not trusted at one point, why should it be at
>> a later stage?
>>
>> Only thing which works for me is populate (so get_pages) at create time.
>> But again with no guarantees they are still there at use time clearly
>> documented.
>>
>> 2)
>>
>> I am also not a fan of getparam for individual ioctl flags since I don't
>> think it scales nicely. How about add a param which returns all
>> supported flags like I915_PARAM_USERPTR_SUPPORTED_FLAGS?
>>
>> Downside is it only works for 32-bit flag fields with getparam. Or it
>> could be a query to solve that as well.
> 
> I guess. You don't think it's a little iffy though, since there were
> other flags which were added before this? So effectively userspace
> queries SUPPORTED_FLAGS and might get -EINVAL on older kernels, even
> though the flag is supported on that kernel(like READONLY)?

For me it is probably passable since unsupported getparam fundamentally 
doesn't imply unsupported features predating that getparam. Same as for 
example unsupported engine query does not mean there are no engines. But 
anyway, seems question will be resolved by Daniel so don't pay attention 
to me.

Regards,

Tvrtko


> 
> Maybe a versioning scheme instead? I915_PARAM_USERPTR_VERSION? Seems
> quite common for other params.
> 
>>
>> Regards,
>>
>> Tvrtko
>>
>>> 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>
>>> ---
>>>    drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++-
>>>    drivers/gpu/drm/i915/i915_getparam.c        |  3 ++
>>>    include/uapi/drm/i915_drm.h                 | 18 ++++++++++
>>>    3 files changed, 60 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>> index 56edfeff8c02..fd6880328596 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
>>> @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
>>>
>>>    #endif
>>>
>>> +static int
>>> +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
>>> +{
>>> +     const unsigned long end = addr + len;
>>> +     struct vm_area_struct *vma;
>>> +     int ret = -EFAULT;
>>> +
>>> +     mmap_read_lock(mm);
>>> +     for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
>>> +             if (vma->vm_start > addr)
>>> +                     break;
>>> +
>>> +             if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
>>> +                     break;
>>> +
>>> +             if (vma->vm_end >= end) {
>>> +                     ret = 0;
>>> +                     break;
>>> +             }
>>> +
>>> +             addr = vma->vm_end;
>>> +     }
>>> +     mmap_read_unlock(mm);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>>    /*
>>>     * Creates a new mm object that wraps some normal memory from the process
>>>     * context - user memory.
>>> @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
>>>        }
>>>
>>>        if (args->flags & ~(I915_USERPTR_READ_ONLY |
>>> -                         I915_USERPTR_UNSYNCHRONIZED))
>>> +                         I915_USERPTR_UNSYNCHRONIZED |
>>> +                         I915_USERPTR_PROBE))
>>>                return -EINVAL;
>>>
>>>        if (i915_gem_object_size_2big(args->user_size))
>>> @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
>>>                        return -ENODEV;
>>>        }
>>>
>>> +     if (args->flags & I915_USERPTR_PROBE) {
>>> +             /*
>>> +              * Check that the range pointed to represents real struct
>>> +              * pages and not iomappings (at this moment in time!)
>>> +              */
>>> +             ret = probe_range(current->mm, args->user_ptr, args->user_size);
>>> +             if (ret)
>>> +                     return ret;
>>> +     }
>>> +
>>>    #ifdef CONFIG_MMU_NOTIFIER
>>>        obj = i915_gem_object_alloc();
>>>        if (obj == NULL)
>>> diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c
>>> index 24e18219eb50..d6d2e1a10d14 100644
>>> --- a/drivers/gpu/drm/i915/i915_getparam.c
>>> +++ b/drivers/gpu/drm/i915/i915_getparam.c
>>> @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
>>>        case I915_PARAM_PERF_REVISION:
>>>                value = i915_perf_ioctl_version();
>>>                break;
>>> +     case I915_PARAM_HAS_USERPTR_PROBE:
>>> +             value = true;
>>> +             break;
>>>        default:
>>>                DRM_DEBUG("Unknown parameter %d\n", param->param);
>>>                return -EINVAL;
>>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>>> index e20eeeca7a1c..2e4112bf4d38 100644
>>> --- a/include/uapi/drm/i915_drm.h
>>> +++ b/include/uapi/drm/i915_drm.h
>>> @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait {
>>>     */
>>>    #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55
>>>
>>> +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */
>>> +#define I915_PARAM_HAS_USERPTR_PROBE 56
>>> +
>>>    /* Must be kept compact -- no holes and well documented */
>>>
>>>    typedef struct drm_i915_getparam {
>>> @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr {
>>>         * through the GTT. If the HW can't support readonly access, an error is
>>>         * returned.
>>>         *
>>> +      * I915_USERPTR_PROBE:
>>> +      *
>>> +      * Probe the provided @user_ptr range and validate that the @user_ptr is
>>> +      * indeed pointing to normal memory and that the range is also valid.
>>> +      * For example if some garbage address is given to the kernel, then this
>>> +      * should complain.
>>> +      *
>>> +      * Returns -EFAULT if the probe failed.
>>> +      *
>>> +      * Note that this doesn't populate the backing pages.
>>> +      *
>>> +      * 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.
>>>
>> _______________________________________________
>> 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

Re: [Intel-gfx] [PATCH 2/4] drm/i915/uapi: convert drm_i915_gem_userptr to kernel doc

[Maarten Lankhorst]

Op 15-07-2021 om 12:15 schreef Matthew Auld:
> Add the missing kernel-doc.
>
> 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>
> ---
>  include/uapi/drm/i915_drm.h | 40 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 868c2ee7be60..e20eeeca7a1c 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -2141,14 +2141,52 @@ struct drm_i915_reset_stats {
>  	__u32 pad;
>  };
>  
> +/**
> + * struct drm_i915_gem_userptr - Create GEM object from user allocated memory.
> + *
> + * Userptr objects have several restrictions on what ioctls can be used with the
> + * object handle.
> + */
>  struct drm_i915_gem_userptr {
> +	/**
> +	 * @user_ptr: The pointer to the allocated memory.
> +	 *
> +	 * Needs to be aligned to PAGE_SIZE.
> +	 */
>  	__u64 user_ptr;
> +
> +	/**
> +	 * @user_size:
> +	 *
> +	 * The size in bytes for the allocated memory. This will also become the
> +	 * object size.
> +	 *
> +	 * Needs to be aligned to PAGE_SIZE, and should be at least PAGE_SIZE,
> +	 * or larger.
> +	 */
>  	__u64 user_size;
> +
> +	/**
> +	 * @flags:
> +	 *
> +	 * Supported flags:
> +	 *
> +	 * I915_USERPTR_READ_ONLY:
> +	 *
> +	 * Mark the object as readonly, this also means GPU access can only be
> +	 * readonly. This is only supported on HW which supports readonly access
> +	 * through the GTT. If the HW can't support readonly access, an error is
> +	 * returned.
> +	 *
> +	 * I915_USERPTR_UNSYNCHRONIZED:
> +	 *
> +	 * NOT USED. Setting this flag will result in an error.
> +	 */
>  	__u32 flags;
>  #define I915_USERPTR_READ_ONLY 0x1
>  #define I915_USERPTR_UNSYNCHRONIZED 0x80000000
>  	/**
> -	 * Returned handle for the object.
> +	 * @handle: Returned handle for the object.
>  	 *
>  	 * Object handles are nonzero.
>  	 */

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

Can you review

https://patchwork.freedesktop.org/patch/444147/?series=92359&rev=2

?

I noticed some parts of i915_drm.h missing.

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

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

[Kenneth Graunke]

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

On Thursday, July 15, 2021 4:27:44 AM PDT Tvrtko Ursulin wrote:
> 
> On 15/07/2021 12:07, Daniel Vetter wrote:
> > On Thu, Jul 15, 2021 at 11:33:10AM +0100, Tvrtko Ursulin wrote:
> >>
> >> On 15/07/2021 11:15, Matthew Auld 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 are going to drop support for set_domain(), so offering a
> >>> way to probe the pages, without having to resort to dummy batches has
> >>> been requested.
> >>>
> >>> v2:
> >>> - add new query param for the PROPBE flag, so userspace can easily
> >>>     check if the kernel supports it(Jason).
> >>> - use mmap_read_{lock, unlock}.
> >>> - add some kernel-doc.
> >>
> >> 1)
> >>
> >> I think probing is too weak to be offered as part of the uapi. What probes
> >> successfully at create time might not be there anymore at usage time. So if
> >> the pointer is not trusted at one point, why should it be at a later stage?
> >>
> >> Only thing which works for me is populate (so get_pages) at create time. But
> >> again with no guarantees they are still there at use time clearly
> >> documented.
> > 
> > Populate is exactly as racy as probe. We don't support pinned userptr
> > anymore.
> 
> Yes, wrote so myself - "..again with no guarantees they are still there 
> at use time..".
> 
> Perhaps I don't understand what problem is probe supposed to solve. It 
> doesn't deal 1:1 with set_domain removal since that one actually did 
> get_pages so that would be populate. But fact remains regardless that if 
> userspace is given a pointer it doesn't trust, _and_ wants the check it 
> for this reason or that, then probe solves nothing. Unless there is 
> actually at minimum some protocol to reply to whoever sent the pointer 
> like "not that pointer please".

That's exactly the point.  GL_AMD_pinned_memory requires us the OpenGL
implementation to return an error for "not that pointer, please", at the
time when said pointer is supplied - not at first use.

Sure, there can be reasons why it might seem fine up front, and not work
later.  But an early check of "just no, you're doing it totally wrong"
at the right moment can be helpful for application developers.  While it
shouldn't really happen, if it ever did, it would be a lot more obvious
to debug than "much later on, when something randomly flushed the GPU
commands we were building, something went wrong, and we don't know why."

--Ken

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 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

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

[Jason Ekstrand]

On Thu, Jul 15, 2021 at 1:21 PM Kenneth Graunke <kenneth@whitecape.org> wrote:
>
> On Thursday, July 15, 2021 4:27:44 AM PDT Tvrtko Ursulin wrote:
> >
> > On 15/07/2021 12:07, Daniel Vetter wrote:
> > > On Thu, Jul 15, 2021 at 11:33:10AM +0100, Tvrtko Ursulin wrote:
> > >>
> > >> On 15/07/2021 11:15, Matthew Auld 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 are going to drop support for set_domain(), so offering a
> > >>> way to probe the pages, without having to resort to dummy batches has
> > >>> been requested.
> > >>>
> > >>> v2:
> > >>> - add new query param for the PROPBE flag, so userspace can easily
> > >>>     check if the kernel supports it(Jason).
> > >>> - use mmap_read_{lock, unlock}.
> > >>> - add some kernel-doc.
> > >>
> > >> 1)
> > >>
> > >> I think probing is too weak to be offered as part of the uapi. What probes
> > >> successfully at create time might not be there anymore at usage time. So if
> > >> the pointer is not trusted at one point, why should it be at a later stage?
> > >>
> > >> Only thing which works for me is populate (so get_pages) at create time. But
> > >> again with no guarantees they are still there at use time clearly
> > >> documented.
> > >
> > > Populate is exactly as racy as probe. We don't support pinned userptr
> > > anymore.
> >
> > Yes, wrote so myself - "..again with no guarantees they are still there
> > at use time..".
> >
> > Perhaps I don't understand what problem is probe supposed to solve. It
> > doesn't deal 1:1 with set_domain removal since that one actually did
> > get_pages so that would be populate. But fact remains regardless that if
> > userspace is given a pointer it doesn't trust, _and_ wants the check it
> > for this reason or that, then probe solves nothing. Unless there is
> > actually at minimum some protocol to reply to whoever sent the pointer
> > like "not that pointer please".
>
> That's exactly the point.  GL_AMD_pinned_memory requires us the OpenGL
> implementation to return an error for "not that pointer, please", at the
> time when said pointer is supplied - not at first use.
>
> Sure, there can be reasons why it might seem fine up front, and not work
> later.  But an early check of "just no, you're doing it totally wrong"
> at the right moment can be helpful for application developers.  While it
> shouldn't really happen, if it ever did, it would be a lot more obvious
> to debug than "much later on, when something randomly flushed the GPU
> commands we were building, something went wrong, and we don't know why."

And before someone chimes in saying that Vulkan doesn't need this
because we can just VK_ERROR_DEVICE_LOST later, that's not true.  We'd
like to be able to return VK_ERROR_INVALID_EXTERNAL_HANDLE in the
easily checkable cases like if they try to pass in a mmapped file.

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

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

[Tvrtko Ursulin]

On 15/07/2021 19:21, Kenneth Graunke wrote:
> On Thursday, July 15, 2021 4:27:44 AM PDT Tvrtko Ursulin wrote:
>>
>> On 15/07/2021 12:07, Daniel Vetter wrote:
>>> On Thu, Jul 15, 2021 at 11:33:10AM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 15/07/2021 11:15, Matthew Auld 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 are going to drop support for set_domain(), so offering a
>>>>> way to probe the pages, without having to resort to dummy batches has
>>>>> been requested.
>>>>>
>>>>> v2:
>>>>> - add new query param for the PROPBE flag, so userspace can easily
>>>>>      check if the kernel supports it(Jason).
>>>>> - use mmap_read_{lock, unlock}.
>>>>> - add some kernel-doc.
>>>>
>>>> 1)
>>>>
>>>> I think probing is too weak to be offered as part of the uapi. What probes
>>>> successfully at create time might not be there anymore at usage time. So if
>>>> the pointer is not trusted at one point, why should it be at a later stage?
>>>>
>>>> Only thing which works for me is populate (so get_pages) at create time. But
>>>> again with no guarantees they are still there at use time clearly
>>>> documented.
>>>
>>> Populate is exactly as racy as probe. We don't support pinned userptr
>>> anymore.
>>
>> Yes, wrote so myself - "..again with no guarantees they are still there
>> at use time..".
>>
>> Perhaps I don't understand what problem is probe supposed to solve. It
>> doesn't deal 1:1 with set_domain removal since that one actually did
>> get_pages so that would be populate. But fact remains regardless that if
>> userspace is given a pointer it doesn't trust, _and_ wants the check it
>> for this reason or that, then probe solves nothing. Unless there is
>> actually at minimum some protocol to reply to whoever sent the pointer
>> like "not that pointer please".
> 
> That's exactly the point.  GL_AMD_pinned_memory requires us the OpenGL
> implementation to return an error for "not that pointer, please", at the
> time when said pointer is supplied - not at first use.
> 
> Sure, there can be reasons why it might seem fine up front, and not work
> later.  But an early check of "just no, you're doing it totally wrong"
> at the right moment can be helpful for application developers.  While it
> shouldn't really happen, if it ever did, it would be a lot more obvious
> to debug than "much later on, when something randomly flushed the GPU
> commands we were building, something went wrong, and we don't know why."

Ack, thanks for the clarification. For this limited use case I agree probe works.

Regards,

Tvrtko

P.S. I made a mistake (?) of looking at the GL_AMD_pinned_memory spec:

"""

     3) Can the application free the memory?

         RESOLVED: YES. However, the underlying buffer object should
         have been deleted from the OpenGL to prevent any access by
         the GPU, or the result is undefined, including program or even system
         termination.
"""

Scary stuff that spec of userspace level API would allow such kernel bugs!

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

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

[Tvrtko Ursulin]

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

PROBE

>    check if the kernel supports it(Jason).
> - use mmap_read_{lock, unlock}.
> - add some kernel-doc.
> 
> Testcase: igt/gem_userptr_blits/probe
> Signed-off-by: Chris Wilson <chris@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>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++-
>   drivers/gpu/drm/i915/i915_getparam.c        |  3 ++
>   include/uapi/drm/i915_drm.h                 | 18 ++++++++++
>   3 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index 56edfeff8c02..fd6880328596 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
>   
>   #endif
>   
> +static int
> +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
> +{
> +	const unsigned long end = addr + len;
> +	struct vm_area_struct *vma;
> +	int ret = -EFAULT;
> +
> +	mmap_read_lock(mm);
> +	for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
> +		if (vma->vm_start > addr)
> +			break;
> +
> +		if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
> +			break;
> +
> +		if (vma->vm_end >= end) {
> +			ret = 0;
> +			break;
> +		}
> +
> +		addr = vma->vm_end;
> +	}
> +	mmap_read_unlock(mm);

Logic here looks good to me.

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

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

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

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

Regards,

Tvrtko

> +	 *
> +	 * The kernel supports this feature if I915_PARAM_HAS_USERPTR_PROBE
> +	 * returns a non-zero value.
> +	 *
>   	 * I915_USERPTR_UNSYNCHRONIZED:
>   	 *
>   	 * NOT USED. Setting this flag will result in an error.
>   	 */
>   	__u32 flags;
>   #define I915_USERPTR_READ_ONLY 0x1
> +#define I915_USERPTR_PROBE 0x2
>   #define I915_USERPTR_UNSYNCHRONIZED 0x80000000
>   	/**
>   	 * @handle: Returned handle for the object.
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Thu, Jul 15, 2021 at 8:21 PM Kenneth Graunke <kenneth@whitecape.org> wrote:
>
> On Thursday, July 15, 2021 4:27:44 AM PDT Tvrtko Ursulin wrote:
> >
> > On 15/07/2021 12:07, Daniel Vetter wrote:
> > > On Thu, Jul 15, 2021 at 11:33:10AM +0100, Tvrtko Ursulin wrote:
> > >>
> > >> On 15/07/2021 11:15, Matthew Auld 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 are going to drop support for set_domain(), so offering a
> > >>> way to probe the pages, without having to resort to dummy batches has
> > >>> been requested.
> > >>>
> > >>> v2:
> > >>> - add new query param for the PROPBE flag, so userspace can easily
> > >>>     check if the kernel supports it(Jason).
> > >>> - use mmap_read_{lock, unlock}.
> > >>> - add some kernel-doc.
> > >>
> > >> 1)
> > >>
> > >> I think probing is too weak to be offered as part of the uapi. What probes
> > >> successfully at create time might not be there anymore at usage time. So if
> > >> the pointer is not trusted at one point, why should it be at a later stage?
> > >>
> > >> Only thing which works for me is populate (so get_pages) at create time. But
> > >> again with no guarantees they are still there at use time clearly
> > >> documented.
> > >
> > > Populate is exactly as racy as probe. We don't support pinned userptr
> > > anymore.
> >
> > Yes, wrote so myself - "..again with no guarantees they are still there
> > at use time..".
> >
> > Perhaps I don't understand what problem is probe supposed to solve. It
> > doesn't deal 1:1 with set_domain removal since that one actually did
> > get_pages so that would be populate. But fact remains regardless that if
> > userspace is given a pointer it doesn't trust, _and_ wants the check it
> > for this reason or that, then probe solves nothing. Unless there is
> > actually at minimum some protocol to reply to whoever sent the pointer
> > like "not that pointer please".
>
> That's exactly the point.  GL_AMD_pinned_memory requires us the OpenGL
> implementation to return an error for "not that pointer, please", at the
> time when said pointer is supplied - not at first use.
>
> Sure, there can be reasons why it might seem fine up front, and not work
> later.  But an early check of "just no, you're doing it totally wrong"
> at the right moment can be helpful for application developers.  While it
> shouldn't really happen, if it ever did, it would be a lot more obvious
> to debug than "much later on, when something randomly flushed the GPU
> commands we were building, something went wrong, and we don't know why."

Also, that extension doesn't make guarantees about importing nasty
userspace memory where some non-trusted entity could e.g. truncate()
the file and render all pages invalid, even if you're holding a
reference to it still.

It's purely to check "hey I got this random pointer here from
somewhere, I trust it, can you use it assuming I will not change
anything with hit?". Which is exactly what probe solves, and pulling
in the pages is kinda overkill.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/i915/uapi: reject set_domain for discrete

[Tvrtko Ursulin]

On 15/07/2021 11:15, Matthew Auld wrote:
> The CPU domain should be static for discrete, and on DG1 we don't need
> any flushing since everything is already coherent, so really all this
> does is an object wait, for which we have an ioctl. Longer term the
> desired caching should be an immutable creation time property for the
> BO, which can be set with something like gem_create_ext.
> 
> One other user is iris + userptr, which uses the set_domain to probe all
> the pages to check if the GUP succeeds, however we now have a PROBE
> flag for this purpose.
> 
> v2: add some more kernel doc, also add the implicit rules with caching
> 
> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> 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: Ramalingam C <ramalingam.c@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_domain.c |  3 +++
>   include/uapi/drm/i915_drm.h                | 19 +++++++++++++++++++
>   2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> index 43004bef55cb..b684a62bf3b0 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> @@ -490,6 +490,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
>   	u32 write_domain = args->write_domain;
>   	int err;
>   
> +	if (IS_DGFX(to_i915(dev)))
> +		return -ENODEV;
> +
>   	/* Only handle setting domains to types used by the CPU. */
>   	if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS)
>   		return -EINVAL;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 2e4112bf4d38..04ce310e7ee6 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -901,6 +901,25 @@ struct drm_i915_gem_mmap_offset {
>    *	- I915_GEM_DOMAIN_GTT: Mappable aperture domain
>    *
>    * All other domains are rejected.
> + *
> + * Note that for discrete, starting from DG1, this is no longer supported, and
> + * is instead rejected. On such platforms the CPU domain is effectively static,
> + * where we also only support a single &drm_i915_gem_mmap_offset cache mode,
> + * which can't be set explicitly and instead depends on the object placements,
> + * as per the below.
> + *
> + * Implicit caching rules, starting from DG1:
> + *
> + *	- If any of the object placements (see &drm_i915_gem_create_ext_memory_regions)
> + *	  contain I915_MEMORY_CLASS_DEVICE then the object will be allocated and
> + *	  mapped as write-combined only.

A note about write-combine buffer? I guess saying it is userspace 
responsibility to do it and how.

> + *
> + *	- Everything else is always allocated and mapped as write-back, with the
> + *	  guarantee that everything is also coherent with the GPU.

Haven't been following this so just a question on this one - it is not 
considered interesting to offer non-coherent modes, or even write 
combine, with system memory buffers, for a specific reason?

Regards,

Tvrtko

> + *
> + * Note that this is likely to change in the future again, where we might need
> + * more flexibility on future devices, so making this all explicit as part of a
> + * new &drm_i915_gem_create_ext extension is probable.
>    */
>   struct drm_i915_gem_set_domain {
>   	/** @handle: Handle for the object. */
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/i915/uapi: reject set_domain for discrete

[Jason Ekstrand]

On Fri, Jul 16, 2021 at 9:52 AM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 15/07/2021 11:15, Matthew Auld wrote:
> > The CPU domain should be static for discrete, and on DG1 we don't need
> > any flushing since everything is already coherent, so really all this
> > does is an object wait, for which we have an ioctl. Longer term the
> > desired caching should be an immutable creation time property for the
> > BO, which can be set with something like gem_create_ext.
> >
> > One other user is iris + userptr, which uses the set_domain to probe all
> > the pages to check if the GUP succeeds, however we now have a PROBE
> > flag for this purpose.
> >
> > v2: add some more kernel doc, also add the implicit rules with caching
> >
> > Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> > 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: Ramalingam C <ramalingam.c@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_domain.c |  3 +++
> >   include/uapi/drm/i915_drm.h                | 19 +++++++++++++++++++
> >   2 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > index 43004bef55cb..b684a62bf3b0 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > @@ -490,6 +490,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
> >       u32 write_domain = args->write_domain;
> >       int err;
> >
> > +     if (IS_DGFX(to_i915(dev)))
> > +             return -ENODEV;
> > +
> >       /* Only handle setting domains to types used by the CPU. */
> >       if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS)
> >               return -EINVAL;
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index 2e4112bf4d38..04ce310e7ee6 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -901,6 +901,25 @@ struct drm_i915_gem_mmap_offset {
> >    *  - I915_GEM_DOMAIN_GTT: Mappable aperture domain
> >    *
> >    * All other domains are rejected.
> > + *
> > + * Note that for discrete, starting from DG1, this is no longer supported, and
> > + * is instead rejected. On such platforms the CPU domain is effectively static,
> > + * where we also only support a single &drm_i915_gem_mmap_offset cache mode,
> > + * which can't be set explicitly and instead depends on the object placements,
> > + * as per the below.
> > + *
> > + * Implicit caching rules, starting from DG1:
> > + *
> > + *   - If any of the object placements (see &drm_i915_gem_create_ext_memory_regions)
> > + *     contain I915_MEMORY_CLASS_DEVICE then the object will be allocated and
> > + *     mapped as write-combined only.

Is this accurate?  I thought they got WB when living in SMEM and WC
when on the device.  But, since both are coherent, it's safe to lie to
userspace and say it's all WC.  Is that correct or am I missing
something?

> A note about write-combine buffer? I guess saying it is userspace
> responsibility to do it and how.

What exactly are you thinking is userspace's responsibility?

> > + *
> > + *   - Everything else is always allocated and mapped as write-back, with the
> > + *     guarantee that everything is also coherent with the GPU.
>
> Haven't been following this so just a question on this one - it is not
> considered interesting to offer non-coherent modes, or even write
> combine, with system memory buffers, for a specific reason?

We only care about non-coherent modes on integrated little-core.
There, we share memory between CPU and GPU but snooping from the GPU
is optional.  Depending on access patterns, we might want WB with GPU
snooping or we might want WC.  I don't think we care about WC for SMEM
allocations on discrete.  For that matter, I'm not sure you can
actually shut snooping off when going across a "real" PCIe bus.  At
least not with DG1.

--Jason

> Regards,
>
> Tvrtko
>
> > + *
> > + * Note that this is likely to change in the future again, where we might need
> > + * more flexibility on future devices, so making this all explicit as part of a
> > + * new &drm_i915_gem_create_ext extension is probable.
> >    */
> >   struct drm_i915_gem_set_domain {
> >       /** @handle: Handle for the object. */
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✓ Fi.CI.BAT: success for Some DG1 uAPI cleanup

[Patchwork]

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

== Series Details ==

Series: Some DG1 uAPI cleanup
URL   : https://patchwork.freedesktop.org/series/92581/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_10345 -> Patchwork_20628
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@semaphore:
    - fi-bdw-5557u:       NOTRUN -> [SKIP][1] ([fdo#109271]) +27 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/fi-bdw-5557u/igt@amdgpu/amd_basic@semaphore.html

  * igt@core_hotunplug@unbind-rebind:
    - fi-bdw-5557u:       NOTRUN -> [WARN][2] ([i915#3718])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/fi-bdw-5557u/igt@core_hotunplug@unbind-rebind.html

  * igt@kms_chamelium@dp-crc-fast:
    - fi-bdw-5557u:       NOTRUN -> [SKIP][3] ([fdo#109271] / [fdo#111827]) +8 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/fi-bdw-5557u/igt@kms_chamelium@dp-crc-fast.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3:
    - {fi-tgl-1115g4}:    [FAIL][4] ([i915#1888]) -> [PASS][5]
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/fi-tgl-1115g4/igt@gem_exec_suspend@basic-s3.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/fi-tgl-1115g4/igt@gem_exec_suspend@basic-s3.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7500u:       [DMESG-FAIL][6] ([i915#165]) -> [PASS][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/fi-kbl-7500u/igt@kms_chamelium@common-hpd-after-suspend.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/fi-kbl-7500u/igt@kms_chamelium@common-hpd-after-suspend.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#165]: https://gitlab.freedesktop.org/drm/intel/issues/165
  [i915#1888]: https://gitlab.freedesktop.org/drm/intel/issues/1888
  [i915#3717]: https://gitlab.freedesktop.org/drm/intel/issues/3717
  [i915#3718]: https://gitlab.freedesktop.org/drm/intel/issues/3718


Participating hosts (41 -> 36)
------------------------------

  Missing    (5): fi-ilk-m540 fi-hsw-4200u fi-bsw-cyan bat-jsl-1 fi-bdw-samus 


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

  * IGT: IGT_6142 -> IGTPW_6027
  * Linux: CI_DRM_10345 -> Patchwork_20628

  CI-20190529: 20190529
  CI_DRM_10345: 8c6a974b932fbaa798102b4713ceedf3b04227d9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_6027: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6027/index.html
  IGT_6142: 16e753fc5e1e51395e1df40865c569984a74c5ed @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_20628: ffb84e862c8e7a7fe4ae0cd7b7e13c254d9b8a9a @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

ffb84e862c8e drm/i915/uapi: reject set_domain for discrete
d31ae947a230 drm/i915/userptr: Probe existence of backing struct pages upon creation
c809985d14f7 drm/i915/uapi: convert drm_i915_gem_userptr to kernel doc
7dca45f37ec0 drm/i915/uapi: reject caching ioctls for discrete

== Logs ==

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

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

[Intel-gfx] ✗ Fi.CI.IGT: failure for Some DG1 uAPI cleanup

[Patchwork]

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

== Series Details ==

Series: Some DG1 uAPI cleanup
URL   : https://patchwork.freedesktop.org/series/92581/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_10345_full -> Patchwork_20628_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_20628_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_20628_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_20628_full:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_addfb_basic@invalid-smem-bo-on-discrete:
    - shard-iclb:         NOTRUN -> [SKIP][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb8/igt@kms_addfb_basic@invalid-smem-bo-on-discrete.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@kms_atomic@crtc-invalid-params-fence:
    - {shard-rkl}:        [SKIP][2] ([i915#1845]) -> [DMESG-WARN][3]
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-rkl-2/igt@kms_atomic@crtc-invalid-params-fence.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-rkl-6/igt@kms_atomic@crtc-invalid-params-fence.html

  
New tests
---------

  New tests have been introduced between CI_DRM_10345_full and Patchwork_20628_full:

### New IGT tests (1) ###

  * igt@gem_userptr_blits@probe:
    - Statuses : 6 pass(s)
    - Exec time: [0.17, 0.77] s

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@feature_discovery@display-2x:
    - shard-tglb:         NOTRUN -> [SKIP][4] ([i915#1839])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb1/igt@feature_discovery@display-2x.html

  * igt@feature_discovery@display-4x:
    - shard-iclb:         NOTRUN -> [SKIP][5] ([i915#1839]) +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb8/igt@feature_discovery@display-4x.html

  * igt@feature_discovery@psr2:
    - shard-iclb:         [PASS][6] -> [SKIP][7] ([i915#658])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-iclb2/igt@feature_discovery@psr2.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb4/igt@feature_discovery@psr2.html

  * igt@gem_ctx_persistence@legacy-engines-queued:
    - shard-snb:          NOTRUN -> [SKIP][8] ([fdo#109271] / [i915#1099]) +4 similar issues
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-snb7/igt@gem_ctx_persistence@legacy-engines-queued.html

  * igt@gem_ctx_sseu@mmap-args:
    - shard-tglb:         NOTRUN -> [SKIP][9] ([i915#280])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb1/igt@gem_ctx_sseu@mmap-args.html

  * igt@gem_eio@in-flight-suspend:
    - shard-apl:          [PASS][10] -> [DMESG-WARN][11] ([i915#180]) +1 similar issue
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-apl3/igt@gem_eio@in-flight-suspend.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-apl8/igt@gem_eio@in-flight-suspend.html

  * igt@gem_eio@unwedge-stress:
    - shard-snb:          NOTRUN -> [FAIL][12] ([i915#3354])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-snb5/igt@gem_eio@unwedge-stress.html

  * igt@gem_exec_fair@basic-deadline:
    - shard-kbl:          [PASS][13] -> [FAIL][14] ([i915#2846])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-kbl3/igt@gem_exec_fair@basic-deadline.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-kbl1/igt@gem_exec_fair@basic-deadline.html
    - shard-glk:          [PASS][15] -> [FAIL][16] ([i915#2846])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-glk8/igt@gem_exec_fair@basic-deadline.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-glk8/igt@gem_exec_fair@basic-deadline.html

  * igt@gem_exec_fair@basic-none@vcs0:
    - shard-glk:          [PASS][17] -> [FAIL][18] ([i915#2842]) +2 similar issues
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-glk4/igt@gem_exec_fair@basic-none@vcs0.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-glk1/igt@gem_exec_fair@basic-none@vcs0.html

  * igt@gem_exec_fair@basic-pace@vcs1:
    - shard-iclb:         NOTRUN -> [FAIL][19] ([i915#2842]) +1 similar issue
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb1/igt@gem_exec_fair@basic-pace@vcs1.html

  * igt@gem_exec_params@no-vebox:
    - shard-iclb:         NOTRUN -> [SKIP][20] ([fdo#109283]) +1 similar issue
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb3/igt@gem_exec_params@no-vebox.html
    - shard-tglb:         NOTRUN -> [SKIP][21] ([fdo#109283])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb1/igt@gem_exec_params@no-vebox.html

  * igt@gem_fenced_exec_thrash@2-spare-fences:
    - shard-snb:          [PASS][22] -> [INCOMPLETE][23] ([i915#2055])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-snb5/igt@gem_fenced_exec_thrash@2-spare-fences.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-snb5/igt@gem_fenced_exec_thrash@2-spare-fences.html

  * igt@gem_mmap_gtt@cpuset-big-copy:
    - shard-iclb:         NOTRUN -> [FAIL][24] ([i915#307])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb6/igt@gem_mmap_gtt@cpuset-big-copy.html

  * igt@gem_render_copy@y-tiled-to-vebox-x-tiled:
    - shard-glk:          NOTRUN -> [SKIP][25] ([fdo#109271]) +83 similar issues
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-glk8/igt@gem_render_copy@y-tiled-to-vebox-x-tiled.html

  * igt@gem_render_copy@yf-tiled-mc-ccs-to-vebox-y-tiled:
    - shard-iclb:         NOTRUN -> [SKIP][26] ([i915#768]) +1 similar issue
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb8/igt@gem_render_copy@yf-tiled-mc-ccs-to-vebox-y-tiled.html

  * igt@gem_userptr_blits@dmabuf-sync:
    - shard-kbl:          NOTRUN -> [SKIP][27] ([fdo#109271] / [i915#3323])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-kbl4/igt@gem_userptr_blits@dmabuf-sync.html

  * igt@gem_userptr_blits@input-checking:
    - shard-apl:          NOTRUN -> [DMESG-WARN][28] ([i915#3002]) +1 similar issue
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-apl7/igt@gem_userptr_blits@input-checking.html
    - shard-snb:          NOTRUN -> [DMESG-WARN][29] ([i915#3002])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-snb2/igt@gem_userptr_blits@input-checking.html

  * igt@gem_userptr_blits@process-exit-busy:
    - shard-skl:          [PASS][30] -> [DMESG-WARN][31] ([i915#1982] / [i915#3746])
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-skl2/igt@gem_userptr_blits@process-exit-busy.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-skl8/igt@gem_userptr_blits@process-exit-busy.html
    - shard-snb:          [PASS][32] -> [DMESG-WARN][33] ([i915#3746])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-snb7/igt@gem_userptr_blits@process-exit-busy.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-snb5/igt@gem_userptr_blits@process-exit-busy.html
    - shard-tglb:         [PASS][34] -> [DMESG-WARN][35] ([i915#3746])
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-tglb6/igt@gem_userptr_blits@process-exit-busy.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb2/igt@gem_userptr_blits@process-exit-busy.html
    - shard-glk:          [PASS][36] -> [DMESG-WARN][37] ([i915#3746])
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-glk7/igt@gem_userptr_blits@process-exit-busy.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-glk8/igt@gem_userptr_blits@process-exit-busy.html
    - shard-apl:          NOTRUN -> [DMESG-WARN][38] ([i915#3746])
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-apl2/igt@gem_userptr_blits@process-exit-busy.html
    - shard-iclb:         [PASS][39] -> [DMESG-WARN][40] ([i915#3746])
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-iclb4/igt@gem_userptr_blits@process-exit-busy.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb5/igt@gem_userptr_blits@process-exit-busy.html
    - shard-kbl:          [PASS][41] -> [DMESG-WARN][42] ([i915#3746])
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-kbl1/igt@gem_userptr_blits@process-exit-busy.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-kbl7/igt@gem_userptr_blits@process-exit-busy.html

  * igt@gem_userptr_blits@unsync-overlap:
    - shard-tglb:         NOTRUN -> [SKIP][43] ([i915#3297])
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb3/igt@gem_userptr_blits@unsync-overlap.html
    - shard-iclb:         NOTRUN -> [SKIP][44] ([i915#3297])
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb8/igt@gem_userptr_blits@unsync-overlap.html

  * igt@gem_userptr_blits@vma-merge:
    - shard-iclb:         NOTRUN -> [FAIL][45] ([i915#3318])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb8/igt@gem_userptr_blits@vma-merge.html
    - shard-tglb:         NOTRUN -> [FAIL][46] ([i915#3318])
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb6/igt@gem_userptr_blits@vma-merge.html
    - shard-skl:          NOTRUN -> [FAIL][47] ([i915#3318])
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-skl9/igt@gem_userptr_blits@vma-merge.html

  * igt@gen9_exec_parse@allowed-all:
    - shard-iclb:         NOTRUN -> [SKIP][48] ([fdo#112306]) +3 similar issues
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb3/igt@gen9_exec_parse@allowed-all.html
    - shard-tglb:         NOTRUN -> [SKIP][49] ([fdo#112306]) +3 similar issues
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb1/igt@gen9_exec_parse@allowed-all.html

  * igt@gen9_exec_parse@bb-large:
    - shard-apl:          NOTRUN -> [FAIL][50] ([i915#3296])
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-apl1/igt@gen9_exec_parse@bb-large.html
    - shard-kbl:          NOTRUN -> [FAIL][51] ([i915#3296])
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-kbl6/igt@gen9_exec_parse@bb-large.html

  * igt@i915_pm_dc@dc6-psr:
    - shard-iclb:         NOTRUN -> [DMESG-WARN][52] ([i915#3698])
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb8/igt@i915_pm_dc@dc6-psr.html
    - shard-tglb:         NOTRUN -> [FAIL][53] ([i915#454])
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb3/igt@i915_pm_dc@dc6-psr.html
    - shard-skl:          NOTRUN -> [FAIL][54] ([i915#454])
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-skl7/igt@i915_pm_dc@dc6-psr.html

  * igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-dp:
    - shard-apl:          NOTRUN -> [SKIP][55] ([fdo#109271] / [i915#1937])
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-apl8/igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-dp.html

  * igt@i915_suspend@fence-restore-tiled2untiled:
    - shard-apl:          NOTRUN -> [DMESG-WARN][56] ([i915#180])
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-apl8/igt@i915_suspend@fence-restore-tiled2untiled.html
    - shard-glk:          [PASS][57] -> [INCOMPLETE][58] ([i915#2199])
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-glk3/igt@i915_suspend@fence-restore-tiled2untiled.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-glk2/igt@i915_suspend@fence-restore-tiled2untiled.html

  * igt@kms_atomic_transition@plane-all-modeset-transition:
    - shard-iclb:         NOTRUN -> [SKIP][59] ([i915#1769])
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb7/igt@kms_atomic_transition@plane-all-modeset-transition.html

  * igt@kms_big_fb@linear-32bpp-rotate-180:
    - shard-glk:          [PASS][60] -> [DMESG-WARN][61] ([i915#118] / [i915#95])
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-glk5/igt@kms_big_fb@linear-32bpp-rotate-180.html
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-glk1/igt@kms_big_fb@linear-32bpp-rotate-180.html

  * igt@kms_big_fb@x-tiled-32bpp-rotate-180:
    - shard-tglb:         [PASS][62] -> [DMESG-WARN][63] ([i915#402])
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-tglb5/igt@kms_big_fb@x-tiled-32bpp-rotate-180.html
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb1/igt@kms_big_fb@x-tiled-32bpp-rotate-180.html

  * igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-180-hflip:
    - shard-skl:          NOTRUN -> [SKIP][64] ([fdo#109271] / [i915#3777]) +1 similar issue
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-skl2/igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-180-hflip.html

  * igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-180-async-flip:
    - shard-skl:          NOTRUN -> [FAIL][65] ([i915#3722]) +1 similar issue
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-skl9/igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-180-async-flip.html

  * igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-180-hflip:
    - shard-kbl:          NOTRUN -> [SKIP][66] ([fdo#109271] / [i915#3777]) +1 similar issue
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-kbl4/igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-180-hflip.html
    - shard-glk:          NOTRUN -> [SKIP][67] ([fdo#109271] / [i915#3777])
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-glk9/igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-180-hflip.html

  * igt@kms_big_fb@y-tiled-max-hw-stride-64bpp-rotate-180-async-flip:
    - shard-skl:          NOTRUN -> [FAIL][68] ([i915#3763])
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-skl6/igt@kms_big_fb@y-tiled-max-hw-stride-64bpp-rotate-180-async-flip.html

  * igt@kms_big_fb@yf-tiled-addfb:
    - shard-tglb:         NOTRUN -> [SKIP][69] ([fdo#111615]) +2 similar issues
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb5/igt@kms_big_fb@yf-tiled-addfb.html

  * igt@kms_big_fb@yf-tiled-max-hw-stride-64bpp-rotate-0:
    - shard-apl:          NOTRUN -> [SKIP][70] ([fdo#109271]) +194 similar issues
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-apl1/igt@kms_big_fb@yf-tiled-max-hw-stride-64bpp-rotate-0.html

  * igt@kms_big_fb@yf-tiled-max-hw-stride-64bpp-rotate-180:
    - shard-iclb:         NOTRUN -> [SKIP][71] ([fdo#110723])
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb8/igt@kms_big_fb@yf-tiled-max-hw-stride-64bpp-rotate-180.html

  * igt@kms_ccs@pipe-a-random-ccs-data-yf_tiled_ccs:
    - shard-tglb:         NOTRUN -> [SKIP][72] ([i915#3689]) +8 similar issues
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb1/igt@kms_ccs@pipe-a-random-ccs-data-yf_tiled_ccs.html

  * igt@kms_cdclk@plane-scaling:
    - shard-iclb:         NOTRUN -> [SKIP][73] ([i915#3742])
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb7/igt@kms_cdclk@plane-scaling.html
    - shard-tglb:         NOTRUN -> [SKIP][74] ([i915#3742])
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb5/igt@kms_cdclk@plane-scaling.html

  * igt@kms_chamelium@dp-crc-multiple:
    - shard-apl:          NOTRUN -> [SKIP][75] ([fdo#109271] / [fdo#111827]) +17 similar issues
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-apl6/igt@kms_chamelium@dp-crc-multiple.html

  * igt@kms_chamelium@dp-hpd-fast:
    - shard-kbl:          NOTRUN -> [SKIP][76] ([fdo#109271] / [fdo#111827]) +11 similar issues
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-kbl7/igt@kms_chamelium@dp-hpd-fast.html

  * igt@kms_chamelium@hdmi-hpd-enable-disable-mode:
    - shard-iclb:         NOTRUN -> [SKIP][77] ([fdo#109284] / [fdo#111827]) +12 similar issues
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb6/igt@kms_chamelium@hdmi-hpd-enable-disable-mode.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - shard-snb:          NOTRUN -> [SKIP][78] ([fdo#109271] / [fdo#111827]) +19 similar issues
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-snb7/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_color@pipe-c-ctm-0-5:
    - shard-skl:          [PASS][79] -> [DMESG-WARN][80] ([i915#1982]) +1 similar issue
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-skl8/igt@kms_color@pipe-c-ctm-0-5.html
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-skl7/igt@kms_color@pipe-c-ctm-0-5.html

  * igt@kms_color@pipe-d-ctm-max:
    - shard-iclb:         NOTRUN -> [SKIP][81] ([fdo#109278] / [i915#1149])
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb7/igt@kms_color@pipe-d-ctm-max.html

  * igt@kms_color_chamelium@pipe-b-ctm-max:
    - shard-skl:          NOTRUN -> [SKIP][82] ([fdo#109271] / [fdo#111827]) +9 similar issues
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-skl8/igt@kms_color_chamelium@pipe-b-ctm-max.html
    - shard-tglb:         NOTRUN -> [SKIP][83] ([fdo#109284] / [fdo#111827]) +8 similar issues
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb2/igt@kms_color_chamelium@pipe-b-ctm-max.html

  * igt@kms_color_chamelium@pipe-d-ctm-0-25:
    - shard-glk:          NOTRUN -> [SKIP][84] ([fdo#109271] / [fdo#111827]) +7 similar issues
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-glk3/igt@kms_color_chamelium@pipe-d-ctm-0-25.html
    - shard-iclb:         NOTRUN -> [SKIP][85] ([fdo#109278] / [fdo#109284] / [fdo#111827])
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb8/igt@kms_color_chamelium@pipe-d-ctm-0-25.html

  * igt@kms_content_protection@dp-mst-lic-type-1:
    - shard-iclb:         NOTRUN -> [SKIP][86] ([i915#3116])
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb5/igt@kms_content_protection@dp-mst-lic-type-1.html

  * igt@kms_content_protection@legacy:
    - shard-iclb:         NOTRUN -> [SKIP][87] ([fdo#109300] / [fdo#111066])
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb8/igt@kms_content_protection@legacy.html

  * igt@kms_content_protection@mei_interface:
    - shard-tglb:         NOTRUN -> [SKIP][88] ([fdo#111828])
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb2/igt@kms_content_protection@mei_interface.html

  * igt@kms_content_protection@srm:
    - shard-kbl:          NOTRUN -> [TIMEOUT][89] ([i915#1319])
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-kbl6/igt@kms_content_protection@srm.html
    - shard-apl:          NOTRUN -> [TIMEOUT][90] ([i915#1319])
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-apl6/igt@kms_content_protection@srm.html

  * igt@kms_cursor_crc@pipe-a-cursor-256x85-onscreen:
    - shard-glk:          [PASS][91] -> [FAIL][92] ([i915#3444])
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-glk8/igt@kms_cursor_crc@pipe-a-cursor-256x85-onscreen.html
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-glk2/igt@kms_cursor_crc@pipe-a-cursor-256x85-onscreen.html

  * igt@kms_cursor_crc@pipe-c-cursor-512x170-sliding:
    - shard-iclb:         NOTRUN -> [SKIP][93] ([fdo#109278] / [fdo#109279])
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb5/igt@kms_cursor_crc@pipe-c-cursor-512x170-sliding.html

  * igt@kms_cursor_crc@pipe-c-cursor-max-size-offscreen:
    - shard-tglb:         NOTRUN -> [SKIP][94] ([i915#3359]) +5 similar issues
   [94]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb2/igt@kms_cursor_crc@pipe-c-cursor-max-size-offscreen.html

  * igt@kms_cursor_crc@pipe-d-cursor-256x256-onscreen:
    - shard-kbl:          NOTRUN -> [SKIP][95] ([fdo#109271]) +135 similar issues
   [95]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-kbl1/igt@kms_cursor_crc@pipe-d-cursor-256x256-onscreen.html

  * igt@kms_cursor_crc@pipe-d-cursor-512x512-onscreen:
    - shard-tglb:         NOTRUN -> [SKIP][96] ([fdo#109279] / [i915#3359]) +2 similar issues
   [96]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb7/igt@kms_cursor_crc@pipe-d-cursor-512x512-onscreen.html

  * igt@kms_cursor_edge_walk@pipe-d-128x128-right-edge:
    - shard-snb:          NOTRUN -> [SKIP][97] ([fdo#109271]) +343 similar issues
   [97]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-snb2/igt@kms_cursor_edge_walk@pipe-d-128x128-right-edge.html

  * igt@kms_cursor_legacy@cursorb-vs-flipb-atomic-transitions-varying-size:
    - shard-iclb:         NOTRUN -> [SKIP][98] ([fdo#109274] / [fdo#109278]) +3 similar issues
   [98]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb3/igt@kms_cursor_legacy@cursorb-vs-flipb-atomic-transitions-varying-size.html

  * igt@kms_cursor_legacy@pipe-d-torture-move:
    - shard-skl:          NOTRUN -> [SKIP][99] ([fdo#109271]) +120 similar issues
   [99]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-skl9/igt@kms_cursor_legacy@pipe-d-torture-move.html

  * igt@kms_draw_crc@draw-method-rgb565-pwrite-untiled:
    - shard-skl:          [PASS][100] -> [SKIP][101] ([fdo#109271]) +2 similar issues
   [100]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-skl9/igt@kms_draw_crc@draw-method-rgb565-pwrite-untiled.html
   [101]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-skl3/igt@kms_draw_crc@draw-method-rgb565-pwrite-untiled.html

  * igt@kms_flip@2x-flip-vs-expired-vblank@bc-hdmi-a1-hdmi-a2:
    - shard-glk:          [PASS][102] -> [FAIL][103] ([i915#79])
   [102]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-glk5/igt@kms_flip@2x-flip-vs-expired-vblank@bc-hdmi-a1-hdmi-a2.html
   [103]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-glk3/igt@kms_flip@2x-flip-vs-expired-vblank@bc-hdmi-a1-hdmi-a2.html

  * igt@kms_flip@2x-flip-vs-wf_vblank-interruptible:
    - shard-iclb:         NOTRUN -> [SKIP][104] ([fdo#109274]) +5 similar issues
   [104]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb5/igt@kms_flip@2x-flip-vs-wf_vblank-interruptible.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible@c-edp1:
    - shard-skl:          [PASS][105] -> [FAIL][106] ([i915#79])
   [105]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-skl7/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-edp1.html
   [106]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-skl6/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-edp1.html

  * igt@kms_flip@flip-vs-suspend-interruptible@a-dp1:
    - shard-kbl:          [PASS][107] -> [DMESG-WARN][108] ([i915#180]) +2 similar issues
   [107]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10345/shard-kbl2/igt@kms_flip@flip-vs-suspend-interruptible@a-dp1.html
   [108]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-kbl3/igt@kms_flip@flip-vs-suspend-interruptible@a-dp1.html

  * igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-32bpp-ytilegen12rcccs:
    - shard-apl:          NOTRUN -> [SKIP][109] ([fdo#109271] / [i915#2672])
   [109]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-apl3/igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-32bpp-ytilegen12rcccs.html

  * igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilercccs:
    - shard-kbl:          NOTRUN -> [SKIP][110] ([fdo#109271] / [i915#2672])
   [110]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-kbl6/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilercccs.html

  * igt@kms_force_connector_basic@force-load-detect:
    - shard-iclb:         NOTRUN -> [SKIP][111] ([fdo#109285])
   [111]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb1/igt@kms_force_connector_basic@force-load-detect.html
    - shard-tglb:         NOTRUN -> [SKIP][112] ([fdo#109285])
   [112]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb5/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-onoff:
    - shard-tglb:         NOTRUN -> [SKIP][113] ([fdo#111825]) +27 similar issues
   [113]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb6/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-onoff.html

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-spr-indfb-draw-mmap-cpu:
    - shard-iclb:         NOTRUN -> [SKIP][114] ([fdo#109280]) +26 similar issues
   [114]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb1/igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-spr-indfb-draw-mmap-cpu.html

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-d:
    - shard-apl:          NOTRUN -> [SKIP][115] ([fdo#109271] / [i915#533]) +1 similar issue
   [115]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-apl2/igt@kms_pipe_crc_basic@hang-read-crc-pipe-d.html

  * igt@kms_pipe_crc_basic@read-crc-pipe-d-frame-sequence:
    - shard-iclb:         NOTRUN -> [SKIP][116] ([fdo#109278]) +37 similar issues
   [116]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb5/igt@kms_pipe_crc_basic@read-crc-pipe-d-frame-sequence.html

  * igt@kms_plane_alpha_blend@pipe-a-alpha-basic:
    - shard-apl:          NOTRUN -> [FAIL][117] ([fdo#108145] / [i915#265]) +2 similar issues
   [117]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-apl6/igt@kms_plane_alpha_blend@pipe-a-alpha-basic.html
    - shard-kbl:          NOTRUN -> [FAIL][118] ([fdo#108145] / [i915#265])
   [118]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-kbl6/igt@kms_plane_alpha_blend@pipe-a-alpha-basic.html

  * igt@kms_plane_alpha_blend@pipe-c-alpha-transparent-fb:
    - shard-glk:          NOTRUN -> [FAIL][119] ([i915#265])
   [119]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-glk9/igt@kms_plane_alpha_blend@pipe-c-alpha-transparent-fb.html
    - shard-kbl:          NOTRUN -> [FAIL][120] ([i915#265])
   [120]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-kbl4/igt@kms_plane_alpha_blend@pipe-c-alpha-transparent-fb.html
    - shard-skl:          NOTRUN -> [FAIL][121] ([i915#265])
   [121]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-skl8/igt@kms_plane_alpha_blend@pipe-c-alpha-transparent-fb.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-vs-premult-vs-constant:
    - shard-tglb:         NOTRUN -> [SKIP][122] ([i915#3794])
   [122]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb5/igt@kms_plane_alpha_blend@pipe-c-coverage-vs-premult-vs-constant.html

  * igt@kms_plane_lowres@pipe-b-tiling-yf:
    - shard-tglb:         NOTRUN -> [SKIP][123] ([fdo#112054])
   [123]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb6/igt@kms_plane_lowres@pipe-b-tiling-yf.html

  * igt@kms_plane_lowres@pipe-c-tiling-none:
    - shard-iclb:         NOTRUN -> [SKIP][124] ([i915#3536]) +1 similar issue
   [124]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb8/igt@kms_plane_lowres@pipe-c-tiling-none.html
    - shard-tglb:         NOTRUN -> [SKIP][125] ([i915#3536]) +1 similar issue
   [125]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb6/igt@kms_plane_lowres@pipe-c-tiling-none.html

  * igt@kms_plane_scaling@scaler-with-clipping-clamping@pipe-c-scaler-with-clipping-clamping:
    - shard-skl:          NOTRUN -> [SKIP][126] ([fdo#109271] / [i915#2733])
   [126]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-skl5/igt@kms_plane_scaling@scaler-with-clipping-clamping@pipe-c-scaler-with-clipping-clamping.html

  * igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area-4:
    - shard-glk:          NOTRUN -> [SKIP][127] ([fdo#109271] / [i915#658]) +1 similar issue
   [127]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-glk7/igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area-4.html
    - shard-iclb:         NOTRUN -> [SKIP][128] ([i915#658]) +2 similar issues
   [128]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-iclb6/igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area-4.html

  * igt@kms_psr2_sf@overlay-primary-update-sf-dmg-area-4:
    - shard-apl:          NOTRUN -> [SKIP][129] ([fdo#109271] / [i915#658]) +3 similar issues
   [129]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-apl6/igt@kms_psr2_sf@overlay-primary-update-sf-dmg-area-4.html

  * igt@kms_psr2_sf@plane-move-sf-dmg-area-1:
    - shard-skl:          NOTRUN -> [SKIP][130] ([fdo#109271] / [i915#658])
   [130]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-skl3/igt@kms_psr2_sf@plane-move-sf-dmg-area-1.html
    - shard-tglb:         NOTRUN -> [SKIP][131] ([i915#2920]) +1 similar issue
   [131]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20628/shard-tglb5/igt@kms_psr2_sf@plane-move-sf-dmg-area-1.html

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

  * igt@kms_p

== Logs ==

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

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

Re: [Intel-gfx] [PATCH 4/4] drm/i915/uapi: reject set_domain for discrete

[Tvrtko Ursulin]

On 16/07/2021 16:23, Jason Ekstrand wrote:
> On Fri, Jul 16, 2021 at 9:52 AM Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>
>> On 15/07/2021 11:15, Matthew Auld wrote:
>>> The CPU domain should be static for discrete, and on DG1 we don't need
>>> any flushing since everything is already coherent, so really all this
>>> does is an object wait, for which we have an ioctl. Longer term the
>>> desired caching should be an immutable creation time property for the
>>> BO, which can be set with something like gem_create_ext.
>>>
>>> One other user is iris + userptr, which uses the set_domain to probe all
>>> the pages to check if the GUP succeeds, however we now have a PROBE
>>> flag for this purpose.
>>>
>>> v2: add some more kernel doc, also add the implicit rules with caching
>>>
>>> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
>>> 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: Ramalingam C <ramalingam.c@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/gem/i915_gem_domain.c |  3 +++
>>>    include/uapi/drm/i915_drm.h                | 19 +++++++++++++++++++
>>>    2 files changed, 22 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
>>> index 43004bef55cb..b684a62bf3b0 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
>>> @@ -490,6 +490,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
>>>        u32 write_domain = args->write_domain;
>>>        int err;
>>>
>>> +     if (IS_DGFX(to_i915(dev)))
>>> +             return -ENODEV;
>>> +
>>>        /* Only handle setting domains to types used by the CPU. */
>>>        if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS)
>>>                return -EINVAL;
>>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>>> index 2e4112bf4d38..04ce310e7ee6 100644
>>> --- a/include/uapi/drm/i915_drm.h
>>> +++ b/include/uapi/drm/i915_drm.h
>>> @@ -901,6 +901,25 @@ struct drm_i915_gem_mmap_offset {
>>>     *  - I915_GEM_DOMAIN_GTT: Mappable aperture domain
>>>     *
>>>     * All other domains are rejected.
>>> + *
>>> + * Note that for discrete, starting from DG1, this is no longer supported, and
>>> + * is instead rejected. On such platforms the CPU domain is effectively static,
>>> + * where we also only support a single &drm_i915_gem_mmap_offset cache mode,
>>> + * which can't be set explicitly and instead depends on the object placements,
>>> + * as per the below.
>>> + *
>>> + * Implicit caching rules, starting from DG1:
>>> + *
>>> + *   - If any of the object placements (see &drm_i915_gem_create_ext_memory_regions)
>>> + *     contain I915_MEMORY_CLASS_DEVICE then the object will be allocated and
>>> + *     mapped as write-combined only.
> 
> Is this accurate?  I thought they got WB when living in SMEM and WC
> when on the device.  But, since both are coherent, it's safe to lie to
> userspace and say it's all WC.  Is that correct or am I missing
> something?
> 
>> A note about write-combine buffer? I guess saying it is userspace
>> responsibility to do it and how.
> 
> What exactly are you thinking is userspace's responsibility?

Flushing of the write combine buffer.

> 
>>> + *
>>> + *   - Everything else is always allocated and mapped as write-back, with the
>>> + *     guarantee that everything is also coherent with the GPU.
>>
>> Haven't been following this so just a question on this one - it is not
>> considered interesting to offer non-coherent modes, or even write
>> combine, with system memory buffers, for a specific reason?
> 
> We only care about non-coherent modes on integrated little-core.
> There, we share memory between CPU and GPU but snooping from the GPU
> is optional.  Depending on access patterns, we might want WB with GPU
> snooping or we might want WC.  I don't think we care about WC for SMEM
> allocations on discrete.  For that matter, I'm not sure you can
> actually shut snooping off when going across a "real" PCIe bus.  At
> least not with DG1.

But writes to system memory buffers aren't going over the PCIe bus?!

Anyways, I am not claiming it is an interesting use case, just wondering 
about the reasoning for making the modes fixed.

Regards,

Tvrtko

> 
> --Jason
> 
>> Regards,
>>
>> Tvrtko
>>
>>> + *
>>> + * Note that this is likely to change in the future again, where we might need
>>> + * more flexibility on future devices, so making this all explicit as part of a
>>> + * new &drm_i915_gem_create_ext extension is probable.
>>>     */
>>>    struct drm_i915_gem_set_domain {
>>>        /** @handle: Handle for the object. */
>>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/i915/uapi: reject set_domain for discrete

[Matthew Auld]

On Fri, 16 Jul 2021 at 16:23, Jason Ekstrand <jason@jlekstrand.net> wrote:
>
> On Fri, Jul 16, 2021 at 9:52 AM Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
> >
> >
> > On 15/07/2021 11:15, Matthew Auld wrote:
> > > The CPU domain should be static for discrete, and on DG1 we don't need
> > > any flushing since everything is already coherent, so really all this
> > > does is an object wait, for which we have an ioctl. Longer term the
> > > desired caching should be an immutable creation time property for the
> > > BO, which can be set with something like gem_create_ext.
> > >
> > > One other user is iris + userptr, which uses the set_domain to probe all
> > > the pages to check if the GUP succeeds, however we now have a PROBE
> > > flag for this purpose.
> > >
> > > v2: add some more kernel doc, also add the implicit rules with caching
> > >
> > > Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> > > 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: Ramalingam C <ramalingam.c@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/gem/i915_gem_domain.c |  3 +++
> > >   include/uapi/drm/i915_drm.h                | 19 +++++++++++++++++++
> > >   2 files changed, 22 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > index 43004bef55cb..b684a62bf3b0 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > @@ -490,6 +490,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
> > >       u32 write_domain = args->write_domain;
> > >       int err;
> > >
> > > +     if (IS_DGFX(to_i915(dev)))
> > > +             return -ENODEV;
> > > +
> > >       /* Only handle setting domains to types used by the CPU. */
> > >       if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS)
> > >               return -EINVAL;
> > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > > index 2e4112bf4d38..04ce310e7ee6 100644
> > > --- a/include/uapi/drm/i915_drm.h
> > > +++ b/include/uapi/drm/i915_drm.h
> > > @@ -901,6 +901,25 @@ struct drm_i915_gem_mmap_offset {
> > >    *  - I915_GEM_DOMAIN_GTT: Mappable aperture domain
> > >    *
> > >    * All other domains are rejected.
> > > + *
> > > + * Note that for discrete, starting from DG1, this is no longer supported, and
> > > + * is instead rejected. On such platforms the CPU domain is effectively static,
> > > + * where we also only support a single &drm_i915_gem_mmap_offset cache mode,
> > > + * which can't be set explicitly and instead depends on the object placements,
> > > + * as per the below.
> > > + *
> > > + * Implicit caching rules, starting from DG1:
> > > + *
> > > + *   - If any of the object placements (see &drm_i915_gem_create_ext_memory_regions)
> > > + *     contain I915_MEMORY_CLASS_DEVICE then the object will be allocated and
> > > + *     mapped as write-combined only.
>
> Is this accurate?  I thought they got WB when living in SMEM and WC
> when on the device.  But, since both are coherent, it's safe to lie to
> userspace and say it's all WC.  Is that correct or am I missing
> something?

Yes, it's accurate, it will be allocated and mapped as WC. I think we
can just make select_tt_caching always return cached if we want, and
it looks like ttm seems to be fine with having different caching
values for the tt vs io resource. Daniel, should we adjust this?

>
> > A note about write-combine buffer? I guess saying it is userspace
> > responsibility to do it and how.
>
> What exactly are you thinking is userspace's responsibility?
>
> > > + *
> > > + *   - Everything else is always allocated and mapped as write-back, with the
> > > + *     guarantee that everything is also coherent with the GPU.
> >
> > Haven't been following this so just a question on this one - it is not
> > considered interesting to offer non-coherent modes, or even write
> > combine, with system memory buffers, for a specific reason?
>
> We only care about non-coherent modes on integrated little-core.
> There, we share memory between CPU and GPU but snooping from the GPU
> is optional.  Depending on access patterns, we might want WB with GPU
> snooping or we might want WC.  I don't think we care about WC for SMEM
> allocations on discrete.  For that matter, I'm not sure you can
> actually shut snooping off when going across a "real" PCIe bus.  At
> least not with DG1.
>
> --Jason
>
> > Regards,
> >
> > Tvrtko
> >
> > > + *
> > > + * Note that this is likely to change in the future again, where we might need
> > > + * more flexibility on future devices, so making this all explicit as part of a
> > > + * new &drm_i915_gem_create_ext extension is probable.
> > >    */
> > >   struct drm_i915_gem_set_domain {
> > >       /** @handle: Handle for the object. */
> > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/i915/uapi: reject set_domain for discrete

[Jason Ekstrand]

On Mon, Jul 19, 2021 at 4:10 AM Matthew Auld
<matthew.william.auld@gmail.com> wrote:
>
> On Fri, 16 Jul 2021 at 16:23, Jason Ekstrand <jason@jlekstrand.net> wrote:
> >
> > On Fri, Jul 16, 2021 at 9:52 AM Tvrtko Ursulin
> > <tvrtko.ursulin@linux.intel.com> wrote:
> > >
> > >
> > > On 15/07/2021 11:15, Matthew Auld wrote:
> > > > The CPU domain should be static for discrete, and on DG1 we don't need
> > > > any flushing since everything is already coherent, so really all this
> > > > does is an object wait, for which we have an ioctl. Longer term the
> > > > desired caching should be an immutable creation time property for the
> > > > BO, which can be set with something like gem_create_ext.
> > > >
> > > > One other user is iris + userptr, which uses the set_domain to probe all
> > > > the pages to check if the GUP succeeds, however we now have a PROBE
> > > > flag for this purpose.
> > > >
> > > > v2: add some more kernel doc, also add the implicit rules with caching
> > > >
> > > > Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> > > > 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: Ramalingam C <ramalingam.c@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/gem/i915_gem_domain.c |  3 +++
> > > >   include/uapi/drm/i915_drm.h                | 19 +++++++++++++++++++
> > > >   2 files changed, 22 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > > index 43004bef55cb..b684a62bf3b0 100644
> > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> > > > @@ -490,6 +490,9 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
> > > >       u32 write_domain = args->write_domain;
> > > >       int err;
> > > >
> > > > +     if (IS_DGFX(to_i915(dev)))
> > > > +             return -ENODEV;
> > > > +
> > > >       /* Only handle setting domains to types used by the CPU. */
> > > >       if ((write_domain | read_domains) & I915_GEM_GPU_DOMAINS)
> > > >               return -EINVAL;
> > > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > > > index 2e4112bf4d38..04ce310e7ee6 100644
> > > > --- a/include/uapi/drm/i915_drm.h
> > > > +++ b/include/uapi/drm/i915_drm.h
> > > > @@ -901,6 +901,25 @@ struct drm_i915_gem_mmap_offset {
> > > >    *  - I915_GEM_DOMAIN_GTT: Mappable aperture domain
> > > >    *
> > > >    * All other domains are rejected.
> > > > + *
> > > > + * Note that for discrete, starting from DG1, this is no longer supported, and
> > > > + * is instead rejected. On such platforms the CPU domain is effectively static,
> > > > + * where we also only support a single &drm_i915_gem_mmap_offset cache mode,
> > > > + * which can't be set explicitly and instead depends on the object placements,
> > > > + * as per the below.
> > > > + *
> > > > + * Implicit caching rules, starting from DG1:
> > > > + *
> > > > + *   - If any of the object placements (see &drm_i915_gem_create_ext_memory_regions)
> > > > + *     contain I915_MEMORY_CLASS_DEVICE then the object will be allocated and
> > > > + *     mapped as write-combined only.
> >
> > Is this accurate?  I thought they got WB when living in SMEM and WC
> > when on the device.  But, since both are coherent, it's safe to lie to
> > userspace and say it's all WC.  Is that correct or am I missing
> > something?
>
> Yes, it's accurate, it will be allocated and mapped as WC. I think we
> can just make select_tt_caching always return cached if we want, and
> it looks like ttm seems to be fine with having different caching
> values for the tt vs io resource. Daniel, should we adjust this?

Mildly related, we had an issue some time back with i915+amdgpu where
we were choosing different caching settings for SMEM shared BOs and
the fallout was that we had all sorts of caching trouble when running
an integrated+discrete setup with them.  I don't remember how all that
shook out but we should think about it here.  Why is this important?
Because our mmap caching settings are going to be related to our
snooping settings for GPU access across the PCIe bar to SMEM.  If
we're WC then when can avoid snooping but if we're WB then we need
snooping enabled.  WC+snooping might work but I'm not sure off-hand.

--Jason

> >
> > > A note about write-combine buffer? I guess saying it is userspace
> > > responsibility to do it and how.
> >
> > What exactly are you thinking is userspace's responsibility?
> >
> > > > + *
> > > > + *   - Everything else is always allocated and mapped as write-back, with the
> > > > + *     guarantee that everything is also coherent with the GPU.
> > >
> > > Haven't been following this so just a question on this one - it is not
> > > considered interesting to offer non-coherent modes, or even write
> > > combine, with system memory buffers, for a specific reason?
> >
> > We only care about non-coherent modes on integrated little-core.
> > There, we share memory between CPU and GPU but snooping from the GPU
> > is optional.  Depending on access patterns, we might want WB with GPU
> > snooping or we might want WC.  I don't think we care about WC for SMEM
> > allocations on discrete.  For that matter, I'm not sure you can
> > actually shut snooping off when going across a "real" PCIe bus.  At
> > least not with DG1.
> >
> > --Jason
> >
> > > Regards,
> > >
> > > Tvrtko
> > >
> > > > + *
> > > > + * Note that this is likely to change in the future again, where we might need
> > > > + * more flexibility on future devices, so making this all explicit as part of a
> > > > + * new &drm_i915_gem_create_ext extension is probable.
> > > >    */
> > > >   struct drm_i915_gem_set_domain {
> > > >       /** @handle: Handle for the object. */
> > > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Kenneth Graunke]

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

Thanks for this!  Series is:

Acked-by: Kenneth Graunke <kenneth@whitecape.org>

https://gitlab.freedesktop.org/kwg/mesa/-/commits/iris-userptr-probe
is an untested branch that uses the new probe API in Mesa.

On Thursday, July 15, 2021 3:15:35 AM PDT Matthew Auld 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 are going to drop support for set_domain(), so offering a
> way to probe the pages, without having to resort to dummy batches has
> been requested.
> 
> v2:
> - add new query param for the PROPBE flag, so userspace can easily
>   check if the kernel supports it(Jason).
> - use mmap_read_{lock, unlock}.
> - add some kernel-doc.
> 
> Testcase: igt/gem_userptr_blits/probe
> Signed-off-by: Chris Wilson <chris@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>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_getparam.c        |  3 ++
>  include/uapi/drm/i915_drm.h                 | 18 ++++++++++
>  3 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index 56edfeff8c02..fd6880328596 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
>  
>  #endif
>  
> +static int
> +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
> +{
> +	const unsigned long end = addr + len;
> +	struct vm_area_struct *vma;
> +	int ret = -EFAULT;
> +
> +	mmap_read_lock(mm);
> +	for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
> +		if (vma->vm_start > addr)
> +			break;
> +
> +		if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
> +			break;
> +
> +		if (vma->vm_end >= end) {
> +			ret = 0;
> +			break;
> +		}
> +
> +		addr = vma->vm_end;
> +	}
> +	mmap_read_unlock(mm);
> +
> +	return ret;
> +}
> +
>  /*
>   * Creates a new mm object that wraps some normal memory from the process
>   * context - user memory.
> @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
>  	}
>  
>  	if (args->flags & ~(I915_USERPTR_READ_ONLY |
> -			    I915_USERPTR_UNSYNCHRONIZED))
> +			    I915_USERPTR_UNSYNCHRONIZED |
> +			    I915_USERPTR_PROBE))
>  		return -EINVAL;
>  
>  	if (i915_gem_object_size_2big(args->user_size))
> @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
>  			return -ENODEV;
>  	}
>  
> +	if (args->flags & I915_USERPTR_PROBE) {
> +		/*
> +		 * Check that the range pointed to represents real struct
> +		 * pages and not iomappings (at this moment in time!)
> +		 */
> +		ret = probe_range(current->mm, args->user_ptr, args->user_size);
> +		if (ret)
> +			return ret;
> +	}
> +
>  #ifdef CONFIG_MMU_NOTIFIER
>  	obj = i915_gem_object_alloc();
>  	if (obj == NULL)
> diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c
> index 24e18219eb50..d6d2e1a10d14 100644
> --- a/drivers/gpu/drm/i915/i915_getparam.c
> +++ b/drivers/gpu/drm/i915/i915_getparam.c
> @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
>  	case I915_PARAM_PERF_REVISION:
>  		value = i915_perf_ioctl_version();
>  		break;
> +	case I915_PARAM_HAS_USERPTR_PROBE:
> +		value = true;
> +		break;
>  	default:
>  		DRM_DEBUG("Unknown parameter %d\n", param->param);
>  		return -EINVAL;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index e20eeeca7a1c..2e4112bf4d38 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait {
>   */
>  #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55
>  
> +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */
> +#define I915_PARAM_HAS_USERPTR_PROBE 56
> +
>  /* Must be kept compact -- no holes and well documented */
>  
>  typedef struct drm_i915_getparam {
> @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr {
>  	 * through the GTT. If the HW can't support readonly access, an error is
>  	 * returned.
>  	 *
> +	 * I915_USERPTR_PROBE:
> +	 *
> +	 * Probe the provided @user_ptr range and validate that the @user_ptr is
> +	 * indeed pointing to normal memory and that the range is also valid.
> +	 * For example if some garbage address is given to the kernel, then this
> +	 * should complain.
> +	 *
> +	 * Returns -EFAULT if the probe failed.
> +	 *
> +	 * Note that this doesn't populate the backing pages.
> +	 *
> +	 * 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.
> 


[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 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

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

[Kenneth Graunke]

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

Thanks!  Series is:

Acked-by: Kenneth Graunke <kenneth@whitecape.org>

https://gitlab.freedesktop.org/kwg/mesa/-/commits/iris-userptr-probe
is an untested Mesa branch that makes use of the new probe uAPI.

On Thursday, July 15, 2021 3:15:35 AM PDT Matthew Auld 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 are going to drop support for set_domain(), so offering a
> way to probe the pages, without having to resort to dummy batches has
> been requested.
> 
> v2:
> - add new query param for the PROPBE flag, so userspace can easily
>   check if the kernel supports it(Jason).
> - use mmap_read_{lock, unlock}.
> - add some kernel-doc.
> 
> Testcase: igt/gem_userptr_blits/probe
> Signed-off-by: Chris Wilson <chris@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>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_getparam.c        |  3 ++
>  include/uapi/drm/i915_drm.h                 | 18 ++++++++++
>  3 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index 56edfeff8c02..fd6880328596 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
>  
>  #endif
>  
> +static int
> +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
> +{
> +	const unsigned long end = addr + len;
> +	struct vm_area_struct *vma;
> +	int ret = -EFAULT;
> +
> +	mmap_read_lock(mm);
> +	for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
> +		if (vma->vm_start > addr)
> +			break;
> +
> +		if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
> +			break;
> +
> +		if (vma->vm_end >= end) {
> +			ret = 0;
> +			break;
> +		}
> +
> +		addr = vma->vm_end;
> +	}
> +	mmap_read_unlock(mm);
> +
> +	return ret;
> +}
> +
>  /*
>   * Creates a new mm object that wraps some normal memory from the process
>   * context - user memory.
> @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
>  	}
>  
>  	if (args->flags & ~(I915_USERPTR_READ_ONLY |
> -			    I915_USERPTR_UNSYNCHRONIZED))
> +			    I915_USERPTR_UNSYNCHRONIZED |
> +			    I915_USERPTR_PROBE))
>  		return -EINVAL;
>  
>  	if (i915_gem_object_size_2big(args->user_size))
> @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
>  			return -ENODEV;
>  	}
>  
> +	if (args->flags & I915_USERPTR_PROBE) {
> +		/*
> +		 * Check that the range pointed to represents real struct
> +		 * pages and not iomappings (at this moment in time!)
> +		 */
> +		ret = probe_range(current->mm, args->user_ptr, args->user_size);
> +		if (ret)
> +			return ret;
> +	}
> +
>  #ifdef CONFIG_MMU_NOTIFIER
>  	obj = i915_gem_object_alloc();
>  	if (obj == NULL)
> diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c
> index 24e18219eb50..d6d2e1a10d14 100644
> --- a/drivers/gpu/drm/i915/i915_getparam.c
> +++ b/drivers/gpu/drm/i915/i915_getparam.c
> @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
>  	case I915_PARAM_PERF_REVISION:
>  		value = i915_perf_ioctl_version();
>  		break;
> +	case I915_PARAM_HAS_USERPTR_PROBE:
> +		value = true;
> +		break;
>  	default:
>  		DRM_DEBUG("Unknown parameter %d\n", param->param);
>  		return -EINVAL;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index e20eeeca7a1c..2e4112bf4d38 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait {
>   */
>  #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55
>  
> +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */
> +#define I915_PARAM_HAS_USERPTR_PROBE 56
> +
>  /* Must be kept compact -- no holes and well documented */
>  
>  typedef struct drm_i915_getparam {
> @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr {
>  	 * through the GTT. If the HW can't support readonly access, an error is
>  	 * returned.
>  	 *
> +	 * I915_USERPTR_PROBE:
> +	 *
> +	 * Probe the provided @user_ptr range and validate that the @user_ptr is
> +	 * indeed pointing to normal memory and that the range is also valid.
> +	 * For example if some garbage address is given to the kernel, then this
> +	 * should complain.
> +	 *
> +	 * Returns -EFAULT if the probe failed.
> +	 *
> +	 * Note that this doesn't populate the backing pages.
> +	 *
> +	 * 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.
> 


[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 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

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

[Jason Ekstrand]

On Thu, Jul 15, 2021 at 5:16 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 are going to drop support for set_domain(), so offering a
> way to probe the pages, without having to resort to dummy batches has
> been requested.
>
> v2:
> - add new query param for the PROPBE flag, so userspace can easily
>   check if the kernel supports it(Jason).
> - use mmap_read_{lock, unlock}.
> - add some kernel-doc.
>
> Testcase: igt/gem_userptr_blits/probe
> Signed-off-by: Chris Wilson <chris@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>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_getparam.c        |  3 ++
>  include/uapi/drm/i915_drm.h                 | 18 ++++++++++
>  3 files changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index 56edfeff8c02..fd6880328596 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
>
>  #endif
>
> +static int
> +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
> +{
> +       const unsigned long end = addr + len;
> +       struct vm_area_struct *vma;
> +       int ret = -EFAULT;
> +
> +       mmap_read_lock(mm);
> +       for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
> +               if (vma->vm_start > addr)

Why isn't this > end?  Are we somehow guaranteed that one vma covers
the entire range?

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

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

[Matthew Auld]

On Wed, 21 Jul 2021 at 21:28, Jason Ekstrand <jason@jlekstrand.net> wrote:
>
> On Thu, Jul 15, 2021 at 5:16 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 are going to drop support for set_domain(), so offering a
> > way to probe the pages, without having to resort to dummy batches has
> > been requested.
> >
> > v2:
> > - add new query param for the PROPBE flag, so userspace can easily
> >   check if the kernel supports it(Jason).
> > - use mmap_read_{lock, unlock}.
> > - add some kernel-doc.
> >
> > Testcase: igt/gem_userptr_blits/probe
> > Signed-off-by: Chris Wilson <chris@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>
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++-
> >  drivers/gpu/drm/i915/i915_getparam.c        |  3 ++
> >  include/uapi/drm/i915_drm.h                 | 18 ++++++++++
> >  3 files changed, 60 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > index 56edfeff8c02..fd6880328596 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
> >
> >  #endif
> >
> > +static int
> > +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
> > +{
> > +       const unsigned long end = addr + len;
> > +       struct vm_area_struct *vma;
> > +       int ret = -EFAULT;
> > +
> > +       mmap_read_lock(mm);
> > +       for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
> > +               if (vma->vm_start > addr)
>
> Why isn't this > end?  Are we somehow guaranteed that one vma covers
> the entire range?

AFAIK we are just making sure we don't have a hole(note that we also
update addr below), for example the user might have done a partial
munmap. There could be multiple vma's if the kernel was unable to
merge them. If we reach the vm_end >= end, then we know we have a
"valid" range.

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

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

[Jason Ekstrand]

On Thu, Jul 22, 2021 at 3:44 AM Matthew Auld
<matthew.william.auld@gmail.com> wrote:
>
> On Wed, 21 Jul 2021 at 21:28, Jason Ekstrand <jason@jlekstrand.net> wrote:
> >
> > On Thu, Jul 15, 2021 at 5:16 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 are going to drop support for set_domain(), so offering a
> > > way to probe the pages, without having to resort to dummy batches has
> > > been requested.
> > >
> > > v2:
> > > - add new query param for the PROPBE flag, so userspace can easily
> > >   check if the kernel supports it(Jason).
> > > - use mmap_read_{lock, unlock}.
> > > - add some kernel-doc.
> > >
> > > Testcase: igt/gem_userptr_blits/probe
> > > Signed-off-by: Chris Wilson <chris@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>
> > > ---
> > >  drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++-
> > >  drivers/gpu/drm/i915/i915_getparam.c        |  3 ++
> > >  include/uapi/drm/i915_drm.h                 | 18 ++++++++++
> > >  3 files changed, 60 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > > index 56edfeff8c02..fd6880328596 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> > > @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
> > >
> > >  #endif
> > >
> > > +static int
> > > +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len)
> > > +{
> > > +       const unsigned long end = addr + len;
> > > +       struct vm_area_struct *vma;
> > > +       int ret = -EFAULT;
> > > +
> > > +       mmap_read_lock(mm);
> > > +       for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
> > > +               if (vma->vm_start > addr)
> >
> > Why isn't this > end?  Are we somehow guaranteed that one vma covers
> > the entire range?
>
> AFAIK we are just making sure we don't have a hole(note that we also
> update addr below), for example the user might have done a partial
> munmap. There could be multiple vma's if the kernel was unable to
> merge them. If we reach the vm_end >= end, then we know we have a
> "valid" range.

Ok.  That wasn't obvious to me but I see the addr update now.  Makes
sense.  Might be worth a one-line comment for the next guy.  Either
way,

Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>

Thanks for wiring this up!

--Jason

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