| Message ID | 1444742546-27401-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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>
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
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
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
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
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); }
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(-)