| Message ID | 1443699365-11058-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 01, 2015 at 12:36:05PM +0100, Chris Wilson wrote: > Once userptr becomes part of client API, it is almost a certainty that > eventually someone will try to create a new object from a mapping of > another client object, e.g. > > new = vaImport(vaMap(old, &size), size); > > (using a hypothethical API, not meaning to pick on anyone!) > > Since this is actually fairly safe to implement and to allow for a GTT > mapping (since it is within a single process space and the memory access > passes the standard permissions test) let us not limit the Client > possibilities. > > v2: sfelling pixes > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Gwenole Beauchesne <gwenole.beauchesne@intel.com> > Cc: Micha? Winiarski <michal.winiarski@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> This feels like a really big can of worms, since all the apis I've seen thus far have been rather explicit that you can only import malloc'ed memory. Also tiling and stuff like this ... -Daniel > --- > drivers/gpu/drm/i915/i915_gem_userptr.c | 48 ++++++++++++++++++++++++++++++--- > 1 file changed, 45 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c > index 1b3b451b6658..afd4c2c4cc04 100644 > --- a/drivers/gpu/drm/i915/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c > @@ -795,6 +795,37 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { > .release = i915_gem_userptr_release, > }; > > +static struct drm_i915_gem_object * > +find_object_from_vma(struct drm_device *dev, > + struct drm_i915_gem_userptr *args) > +{ > + struct drm_i915_gem_object *obj = NULL; > + struct vm_area_struct *vma; > + > + down_read(¤t->mm->mmap_sem); > + vma = find_vma(current->mm, args->user_ptr); > + if (vma == NULL) { > + obj = ERR_PTR(-EFAULT); > + goto out; > + } > + > + if (vma->vm_ops != dev->driver->gem_vm_ops) > + goto out; > + > + if (vma->vm_start != args->user_ptr || > + vma->vm_end != args->user_ptr + args->user_size) { > + obj = ERR_PTR(-EINVAL); > + goto out; > + } > + > + obj = to_intel_bo(vma->vm_private_data); > + drm_gem_object_reference(&obj->base); > + > +out: > + up_read(¤t->mm->mmap_sem); > + return obj; > +} > + > /** > * Creates a new mm object that wraps some normal memory from the process > * context - user memory. > @@ -802,8 +833,11 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { > * We impose several restrictions upon the memory being mapped > * into the GPU. > * 1. It must be page aligned (both start/end addresses, i.e ptr and size). > - * 2. It must be normal system memory, not a pointer into another map of IO > - * space (e.g. it must not be a GTT mmapping of another object). > + * 2. It must either be: > + * a) normal system memory, not a pointer into another map of IO > + * space (e.g. it must not be part of a GTT mmapping of another object). > + * b) a pointer to the complete GTT mmap of another object in your > + * address space. > * 3. We only allow a bo as large as we could in theory map into the GTT, > * that is we limit the size to the total size of the GTT. > * 4. The bo is marked as being snoopable. The backing pages are left > @@ -853,6 +887,14 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void *data, struct drm_file *file > return -ENODEV; > } > > + obj = find_object_from_vma(dev, args); > + if (obj) { > + if (!IS_ERR(obj)) > + goto out; > + > + return PTR_ERR(obj); > + } > + > obj = i915_gem_object_alloc(dev); > if (obj == NULL) > return -ENOMEM; > @@ -874,7 +916,7 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void *data, struct drm_file *file > if (ret == 0) > ret = i915_gem_userptr_init__mmu_notifier(obj, args->flags); > if (ret == 0) > - ret = drm_gem_handle_create(file, &obj->base, &handle); > +out: ret = drm_gem_handle_create(file, &obj->base, &handle); > > /* drop reference from allocate - handle holds it now */ > drm_gem_object_unreference_unlocked(&obj->base); > -- > 2.6.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Oct 06, 2015 at 02:12:31PM +0200, Daniel Vetter wrote: > On Thu, Oct 01, 2015 at 12:36:05PM +0100, Chris Wilson wrote: > > Once userptr becomes part of client API, it is almost a certainty that > > eventually someone will try to create a new object from a mapping of > > another client object, e.g. > > > > new = vaImport(vaMap(old, &size), size); > > > > (using a hypothethical API, not meaning to pick on anyone!) > > > > Since this is actually fairly safe to implement and to allow for a GTT > > mapping (since it is within a single process space and the memory access > > passes the standard permissions test) let us not limit the Client > > possibilities. > > > > v2: sfelling pixes > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Gwenole Beauchesne <gwenole.beauchesne@intel.com> > > Cc: Micha? Winiarski <michal.winiarski@intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > This feels like a really big can of worms, since all the apis I've seen > thus far have been rather explicit that you can only import malloc'ed > memory. Weirdly no one even brought it up as an issue for AMD_pinned_memory. Nothing in the spec says you can't glBufferData(GL_EXTERNAL_VIRTUAL_MEMORY_BUFFER_AMD, ..., glMapBuffer(GL_PIXEL_UNPACK_BUFFER, ...)); Not that I would expect it to always work. > Also tiling and stuff like this ... Indeed, it is the only way to get tiling right... The only question is whether every userptr mapping should be a unique bo (this also has the effect of return a new handle for an existing userptr mapping to the same exact address range). Note that the memory address is still controlled by the earlier bo, so it will always remain linear, and with the second handle the user still has to object all the normal userptr restrictions. As you probably guess Gwenole raise the question of whether this behaviour would be possible to implement. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c index 1b3b451b6658..afd4c2c4cc04 100644 --- a/drivers/gpu/drm/i915/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c @@ -795,6 +795,37 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { .release = i915_gem_userptr_release, }; +static struct drm_i915_gem_object * +find_object_from_vma(struct drm_device *dev, + struct drm_i915_gem_userptr *args) +{ + struct drm_i915_gem_object *obj = NULL; + struct vm_area_struct *vma; + + down_read(¤t->mm->mmap_sem); + vma = find_vma(current->mm, args->user_ptr); + if (vma == NULL) { + obj = ERR_PTR(-EFAULT); + goto out; + } + + if (vma->vm_ops != dev->driver->gem_vm_ops) + goto out; + + if (vma->vm_start != args->user_ptr || + vma->vm_end != args->user_ptr + args->user_size) { + obj = ERR_PTR(-EINVAL); + goto out; + } + + obj = to_intel_bo(vma->vm_private_data); + drm_gem_object_reference(&obj->base); + +out: + up_read(¤t->mm->mmap_sem); + return obj; +} + /** * Creates a new mm object that wraps some normal memory from the process * context - user memory. @@ -802,8 +833,11 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { * We impose several restrictions upon the memory being mapped * into the GPU. * 1. It must be page aligned (both start/end addresses, i.e ptr and size). - * 2. It must be normal system memory, not a pointer into another map of IO - * space (e.g. it must not be a GTT mmapping of another object). + * 2. It must either be: + * a) normal system memory, not a pointer into another map of IO + * space (e.g. it must not be part of a GTT mmapping of another object). + * b) a pointer to the complete GTT mmap of another object in your + * address space. * 3. We only allow a bo as large as we could in theory map into the GTT, * that is we limit the size to the total size of the GTT. * 4. The bo is marked as being snoopable. The backing pages are left @@ -853,6 +887,14 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void *data, struct drm_file *file return -ENODEV; } + obj = find_object_from_vma(dev, args); + if (obj) { + if (!IS_ERR(obj)) + goto out; + + return PTR_ERR(obj); + } + obj = i915_gem_object_alloc(dev); if (obj == NULL) return -ENOMEM; @@ -874,7 +916,7 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void *data, struct drm_file *file if (ret == 0) ret = i915_gem_userptr_init__mmu_notifier(obj, args->flags); if (ret == 0) - ret = drm_gem_handle_create(file, &obj->base, &handle); +out: ret = drm_gem_handle_create(file, &obj->base, &handle); /* drop reference from allocate - handle holds it now */ drm_gem_object_unreference_unlocked(&obj->base);
Once userptr becomes part of client API, it is almost a certainty that eventually someone will try to create a new object from a mapping of another client object, e.g. new = vaImport(vaMap(old, &size), size); (using a hypothethical API, not meaning to pick on anyone!) Since this is actually fairly safe to implement and to allow for a GTT mapping (since it is within a single process space and the memory access passes the standard permissions test) let us not limit the Client possibilities. v2: sfelling pixes Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Gwenole Beauchesne <gwenole.beauchesne@intel.com> Cc: Micha? Winiarski <michal.winiarski@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_gem_userptr.c | 48 ++++++++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 3 deletions(-)