Re: [PATCH] drm/i915/userptr: detect un-GUP-able pages early

From: Jinoh Kang <jinoh.kang.kr@gmail.com>
To: "Chris Wilson" <chris@chris-wilson.co.uk>,
	"Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>,
	Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>,
	David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	Matthew Auld <matthew.auld@intel.com>,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/i915/userptr: detect un-GUP-able pages early
Date: Sat, 16 Jan 2021 05:11:16 +0000	[thread overview]
Message-ID: <e3ef8af7-f3fb-a6aa-9838-a90b6d5516ac@gmail.com> (raw)
In-Reply-To: <161073047349.18103.16410031184146072179@build.alporthouse.com>

On 1/15/21 5:07 PM, Chris Wilson wrote:
> Quoting Chris Wilson (2021-01-15 16:56:42)
>> Quoting Jinoh Kang (2021-01-15 16:23:31)
>>> If GUP-ineligible pages are passed to a GEM userptr object, -EFAULT is
>>> returned only when the object is actually bound.
>>>
>>> The xf86-video-intel userspace driver cannot differentiate this
>>> condition, and marks the GPU as wedged.
>>
>> The idea was to call gem_set_domain on the object to validate the pages
>> after creation. I only did that for read-only... I did however make mesa
>> use set-domain for validation.
> 
> Hmm, I remember a reason why we wanted to be lazy in populating the
> pages was that we would often be called to create a map that was never
> used. So the additional gup + bind was measurable, and we would rather
> just have a probe.
> -Chris
> 

try_upload__blt uses the map immediately, so I guess that would be an
appropriate place to patch.

> Basically XShmAttachFd, which is not exposed on libX11.

To clarify: privcmd pages cannot actually be passed by fd, since it's
tightly bound with current->mm.  There's some sysv shmop hooking hack
involved, which is injected in Xorg side.

---

Besides, is there an equivalent code path that lets you eagerly *unpin*
pages when closing an userptr object without waiting for the worker?

This is actually more of a problem in drivers/xen/grant-table.c side,
since it hangs the current task until all page refs are released, and
it didn't even use ZONE_DEVICE pages until recently (while still not
using dev_pagemap_ops::page_free, since the unpopulated-alloc pgmap
is not MEMORY_DEVICE_FS_DAX apparently).

(You could say that we should switch to DMA-BUF instead and that would
be a valid criticism.  I'm merely figuring out what the best workaround
for the current status quo would be.)

I'm using something like the following:
---
 drivers/gpu/drm/i915/gem/i915_gem_object.c       | 8 ++++++++
 drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 1 +
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c      | 3 ++-
 drivers/gpu/drm/i915/i915_params.c               | 3 +++
 drivers/gpu/drm/i915/i915_params.h               | 1 +
 5 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 00d24000b5e8..4352a5788fd8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -167,6 +167,14 @@ static void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file *f
 		i915_lut_handle_free(lut);
 		i915_gem_object_put(obj);
 	}
+
+	if (i915_modparams.gem_userptr_close_immediate &&
+	    i915_gem_object_type_has(obj, I915_GEM_OBJECT_IMM_RELEASE) &&
+	    i915_gem_object_is_shrinkable(obj) &&
+	    !atomic_read(&obj->mm.shrink_pin) &&
+	    i915_gem_object_unbind(obj, I915_GEM_OBJECT_UNBIND_ACTIVE |
+					I915_GEM_OBJECT_UNBIND_TEST) == 0)
+		__i915_gem_object_put_pages(obj);
 }
 
 static void __i915_gem_free_object_rcu(struct rcu_head *head)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index e2d9b7e1e152..0ac1dfed0b91 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -36,6 +36,7 @@ struct drm_i915_gem_object_ops {
 #define I915_GEM_OBJECT_IS_PROXY	BIT(3)
 #define I915_GEM_OBJECT_NO_MMAP		BIT(4)
 #define I915_GEM_OBJECT_ASYNC_CANCEL	BIT(5)
+#define I915_GEM_OBJECT_IMM_RELEASE	BIT(7)
 
 	/* Interface between the GEM object and its backing storage.
 	 * get_pages() is called once prior to the use of the associated set
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index f2eaed6aca3d..baa91daf43a1 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -705,7 +705,8 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
 	.flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE |
 		 I915_GEM_OBJECT_IS_SHRINKABLE |
 		 I915_GEM_OBJECT_NO_MMAP |
-		 I915_GEM_OBJECT_ASYNC_CANCEL,
+		 I915_GEM_OBJECT_ASYNC_CANCEL |
+		 I915_GEM_OBJECT_IMM_RELEASE,
 	.get_pages = i915_gem_userptr_get_pages,
 	.put_pages = i915_gem_userptr_put_pages,
 	.dmabuf_export = i915_gem_userptr_dmabuf_export,
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 7f139ea4a90b..4d056fd1b6e7 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -197,6 +197,9 @@ i915_param_named_unsafe(fake_lmem_start, ulong, 0400,
 	"Fake LMEM start offset (default: 0)");
 #endif
 
+i915_param_named(gem_userptr_close_immediate, bool, 0600,
+	"Immediately release pages when userptr GEM object is closed (default:false)");
+
 static __always_inline void _print_param(struct drm_printer *p,
 					 const char *name,
 					 const char *type,
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 330c03e2b4f7..a94367a0345b 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -79,6 +79,7 @@ struct drm_printer;
 	param(bool, disable_display, false, 0400) \
 	param(bool, verbose_state_checks, true, 0) \
 	param(bool, nuclear_pageflip, false, 0400) \
+	param(bool, gem_userptr_close_immediate, false, 0600) \
 	param(bool, enable_dp_mst, true, 0600) \
 	param(bool, enable_gvt, false, 0400)
 
-- 
Sincerely,
Jinoh Kang
