[PATCH] drm/i915: Deny wrapping an userptr into a framebuffer

Pinning a userptr onto the hardware raises interesting questions about
the lifetime of such a surface as the framebuffer extends that life
beyond the client's address space. That is the hardware will need to
keep scanning out from the backing storage even after the client wants
to remap its address space. As the hardware pins the backing storage,
the userptr becomes invalid and this raises a WARN when the clients
tries to unmap its address space. The situation can be even more
complicated when the buffer is passed between processes, between a
client and display server, where the lifetime and hardware access is
even more confusing. Deny it.

Signed-off-by: Chris Wilson <redacted>
Cc: Daniel Vetter <redacted>
Cc: Tvrtko Ursulin <redacted>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 5 ++++-
 drivers/gpu/drm/i915/intel_display.c    | 5 +++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 2dd911ab3019..3ce1b557f7c4 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c

@@ -974,7 +974,10 @@ out:
  * Also note, that the object created here is not currently a "first class"
  * object, in that several ioctls are banned. These are the CPU access
  * ioctls: mmap(), pwrite and pread. In practice, you are expected to use
- * direct access via your pointer rather than use those ioctls.
+ * direct access via your pointer rather than use those ioctls. Another
+ * restriction is that we do not allow userptr surfaces to be pinned to the
+ * hardware and so we reject any attempt to create a framebuffer out of a
+ * userptr.
  *
  * If you think this is a good interface to use to pass GPU memory between
  * drivers, please use dma-buf instead. In fact, wherever possible use

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b89131654a0e..d1deaedcc4ce 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c

@@ -14116,6 +14116,11 @@ static int intel_user_framebuffer_create_handle(struct drm_framebuffer *fb,
 	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
 	struct drm_i915_gem_object *obj = intel_fb->obj;
 
+	if (obj->userptr.mm) {
+		DRM_DEBUG("attempting to use a userptr for a framebuffer, denied\n");
+		return -EINVAL;
+	}
+
 	return drm_gem_handle_create(file, &obj->base, handle);
 }
 

-- 
2.6.1

On 13/10/15 14:22, Chris Wilson wrote:

Pinning a userptr onto the hardware raises interesting questions about
the lifetime of such a surface as the framebuffer extends that life
beyond the client's address space. That is the hardware will need to
keep scanning out from the backing storage even after the client wants
to remap its address space. As the hardware pins the backing storage,
the userptr becomes invalid and this raises a WARN when the clients
tries to unmap its address space. The situation can be even more
complicated when the buffer is passed between processes, between a
client and display server, where the lifetime and hardware access is
even more confusing. Deny it.

Reviewed-by: Tvrtko Ursulin <redacted>

Regards,

Tvrtko

On Tue, 13 Oct 2015, Tvrtko Ursulin [off-list ref] wrote:

On 13/10/15 14:22, Chris Wilson wrote:

quoted

Pinning a userptr onto the hardware raises interesting questions about
the lifetime of such a surface as the framebuffer extends that life
beyond the client's address space. That is the hardware will need to
keep scanning out from the backing storage even after the client wants
to remap its address space. As the hardware pins the backing storage,
the userptr becomes invalid and this raises a WARN when the clients
tries to unmap its address space. The situation can be even more
complicated when the buffer is passed between processes, between a
client and display server, where the lifetime and hardware access is
even more confusing. Deny it.

Reviewed-by: Tvrtko Ursulin <redacted>

Pushed to drm-intel-fixes, thanks for the patch and review.

BR,
Jani.

Regards,

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

-- 
Jani Nikula, Intel Open Source Technology Center

On 13/10/15 15:00, Tvrtko Ursulin wrote:

On 13/10/15 14:22, Chris Wilson wrote:

quoted

Pinning a userptr onto the hardware raises interesting questions about
the lifetime of such a surface as the framebuffer extends that life
beyond the client's address space. That is the hardware will need to
keep scanning out from the backing storage even after the client wants
to remap its address space. As the hardware pins the backing storage,

P.S.
Or even after the client exits in the new world order!

Regards,

Tvrtko

On Tue, Oct 13, 2015 at 03:00:18PM +0100, Tvrtko Ursulin wrote:

On 13/10/15 14:22, Chris Wilson wrote:

quoted

Pinning a userptr onto the hardware raises interesting questions about
the lifetime of such a surface as the framebuffer extends that life
beyond the client's address space. That is the hardware will need to
keep scanning out from the backing storage even after the client wants
to remap its address space. As the hardware pins the backing storage,
the userptr becomes invalid and this raises a WARN when the clients
tries to unmap its address space. The situation can be even more
complicated when the buffer is passed between processes, between a
client and display server, where the lifetime and hardware access is
even more confusing. Deny it.

Reviewed-by: Tvrtko Ursulin <redacted>

Reviewed-by: Daniel Vetter <redacted>
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On Tue, Oct 13, 2015 at 6:22 AM, Chris Wilson [off-list ref] wrote:

Pinning a userptr onto the hardware raises interesting questions about
the lifetime of such a surface as the framebuffer extends that life
beyond the client's address space. That is the hardware will need to
keep scanning out from the backing storage even after the client wants
to remap its address space. As the hardware pins the backing storage,
the userptr becomes invalid and this raises a WARN when the clients
tries to unmap its address space. The situation can be even more
complicated when the buffer is passed between processes, between a
client and display server, where the lifetime and hardware access is
even more confusing. Deny it.

Can we allow this for unsynchronized userptrs?

Kristian

On Thu, Oct 22, 2015 at 04:23:09PM -0700, Kristian H�gsberg wrote:

On Tue, Oct 13, 2015 at 6:22 AM, Chris Wilson [off-list ref] wrote:

quoted

Pinning a userptr onto the hardware raises interesting questions about
the lifetime of such a surface as the framebuffer extends that life
beyond the client's address space. That is the hardware will need to
keep scanning out from the backing storage even after the client wants
to remap its address space. As the hardware pins the backing storage,
the userptr becomes invalid and this raises a WARN when the clients
tries to unmap its address space. The situation can be even more
complicated when the buffer is passed between processes, between a
client and display server, where the lifetime and hardware access is
even more confusing. Deny it.

Can we allow this for unsynchronized userptrs?

I'd like to not add more complexity to a root-only feature.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On Fri, Oct 23, 2015 at 10:04:24AM +0200, Daniel Vetter wrote:

On Thu, Oct 22, 2015 at 04:23:09PM -0700, Kristian H�gsberg wrote:

quoted

On Tue, Oct 13, 2015 at 6:22 AM, Chris Wilson [off-list ref] wrote:

quoted

Pinning a userptr onto the hardware raises interesting questions about
the lifetime of such a surface as the framebuffer extends that life
beyond the client's address space. That is the hardware will need to
keep scanning out from the backing storage even after the client wants
to remap its address space. As the hardware pins the backing storage,
the userptr becomes invalid and this raises a WARN when the clients
tries to unmap its address space. The situation can be even more
complicated when the buffer is passed between processes, between a
client and display server, where the lifetime and hardware access is
even more confusing. Deny it.

Can we allow this for unsynchronized userptrs?

I'd like to not add more complexity to a root-only feature.

I've considered dropping the root-only restriction. As we've spent more
time analysing what exactly happens if we miss the mmu-notification and
we've decided that it can't grant access to other pages, it just causes
the information on the GPU and on the CPU to become unsynchronized. In
some situations that can be problematic (such as when the surface is
pinned by the hardware and we cannot keep the contract of maintaining
sync with the client address range), but normally the error is just
consistent with failing to the SET_DOMAIN api correctly. On that scale
of things, it is not as large a shotgun as I first feared and we could
ease the restriction and allow it for all. (I still would say that
unsync should only be used for objects being allocated and under full
control by the driver, importing client memory should be extremely
cautious).

We still have the requirement that surfaces exported between processes
use mmu-notifiers in order to revoke the exported surface when the
original mm is torndown, so it is not as simple to just allow fb on some
userptr and not others. (As we may still end up in the situation where
we need to revoke the pinned fb and fail miserably.) But that may just
be overthinking the issue, and letting the pages from one mm be pinned
onto the hw by another process and persist after the first is freed is
not an issue either.

So yes, following the same chain of logic, we could allow unsync fb, but
first we need to relax a few other restrictions en route and then we can
just reject creating fb on userptr with mmu-notifiers attached.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

On Fri, Oct 23, 2015 at 2:28 AM, Chris Wilson [off-list ref] wrote:

On Fri, Oct 23, 2015 at 10:04:24AM +0200, Daniel Vetter wrote:

quoted

On Thu, Oct 22, 2015 at 04:23:09PM -0700, Kristian Høgsberg wrote:

quoted

On Tue, Oct 13, 2015 at 6:22 AM, Chris Wilson [off-list ref] wrote:

quoted

Pinning a userptr onto the hardware raises interesting questions about
the lifetime of such a surface as the framebuffer extends that life
beyond the client's address space. That is the hardware will need to
keep scanning out from the backing storage even after the client wants
to remap its address space. As the hardware pins the backing storage,
the userptr becomes invalid and this raises a WARN when the clients
tries to unmap its address space. The situation can be even more
complicated when the buffer is passed between processes, between a
client and display server, where the lifetime and hardware access is
even more confusing. Deny it.

Can we allow this for unsynchronized userptrs?

I'd like to not add more complexity to a root-only feature.

I've considered dropping the root-only restriction. As we've spent more
time analysing what exactly happens if we miss the mmu-notification and
we've decided that it can't grant access to other pages, it just causes
the information on the GPU and on the CPU to become unsynchronized. In
some situations that can be problematic (such as when the surface is
pinned by the hardware and we cannot keep the contract of maintaining
sync with the client address range), but normally the error is just
consistent with failing to the SET_DOMAIN api correctly. On that scale
of things, it is not as large a shotgun as I first feared and we could
ease the restriction and allow it for all. (I still would say that
unsync should only be used for objects being allocated and under full
control by the driver, importing client memory should be extremely
cautious).

We still have the requirement that surfaces exported between processes
use mmu-notifiers in order to revoke the exported surface when the
original mm is torndown, so it is not as simple to just allow fb on some
userptr and not others. (As we may still end up in the situation where
we need to revoke the pinned fb and fail miserably.) But that may just
be overthinking the issue, and letting the pages from one mm be pinned
onto the hw by another process and persist after the first is freed is
not an issue either.

So yes, following the same chain of logic, we could allow unsync fb, but
first we need to relax a few other restrictions en route and then we can
just reject creating fb on userptr with mmu-notifiers attached.
-Chris

Thanks,  that all sounds good.

Kristian

--
Chris Wilson, Intel Open Source Technology Centre