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

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

[Chris Wilson]

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 <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
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

Re: [Intel-gfx] [PATCH] drm/i915: Deny wrapping an userptr into a framebuffer

[Tvrtko Ursulin]

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 <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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

[Tvrtko Ursulin]

On 13/10/15 15:00, Tvrtko Ursulin wrote:
>
> 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,

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

Regards,

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

Re: [Intel-gfx] [PATCH] drm/i915: Deny wrapping an userptr into a framebuffer

[Jani Nikula]

On Tue, 13 Oct 2015, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> 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 <tvrtko.ursulin@intel.com>

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

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

[Daniel Vetter]

On Tue, Oct 13, 2015 at 03:00:18PM +0100, Tvrtko Ursulin wrote:
> 
> 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 <tvrtko.ursulin@intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Deny wrapping an userptr into a framebuffer

[Kristian Høgsberg]

On Tue, Oct 13, 2015 at 6:22 AM, Chris Wilson <chris@chris-wilson.co.uk> 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

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 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
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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 <chris@chris-wilson.co.uk> 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?

I'd like to not add more complexity to a root-only feature.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Deny wrapping an userptr into a framebuffer

[Chris Wilson]

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:
> > On Tue, Oct 13, 2015 at 6:22 AM, Chris Wilson <chris@chris-wilson.co.uk> 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?
> 
> 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

Re: [Intel-gfx] [PATCH] drm/i915: Deny wrapping an userptr into a framebuffer

[Kristian Høgsberg]

On Fri, Oct 23, 2015 at 2:28 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 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:
>> > On Tue, Oct 13, 2015 at 6:22 AM, Chris Wilson <chris@chris-wilson.co.uk> 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?
>>
>> 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