| Message ID | 20210715101536.2606307-4-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some DG1 uAPI cleanup | expand |
On 15/07/2021 11:15, Matthew Auld wrote: > From: Chris Wilson <chris@chris-wilson.co.uk> > > Jason Ekstrand requested a more efficient method than userptr+set-domain > to determine if the userptr object was backed by a complete set of pages > upon creation. To be more efficient than simply populating the userptr > using get_user_pages() (as done by the call to set-domain or execbuf), > we can walk the tree of vm_area_struct and check for gaps or vma not > backed by struct page (VM_PFNMAP). The question is how to handle > VM_MIXEDMAP which may be either struct page or pfn backed... > > With discrete are going to drop support for set_domain(), so offering a > way to probe the pages, without having to resort to dummy batches has > been requested. > > v2: > - add new query param for the PROPBE flag, so userspace can easily > check if the kernel supports it(Jason). > - use mmap_read_{lock, unlock}. > - add some kernel-doc. 1) I think probing is too weak to be offered as part of the uapi. What probes successfully at create time might not be there anymore at usage time. So if the pointer is not trusted at one point, why should it be at a later stage? Only thing which works for me is populate (so get_pages) at create time. But again with no guarantees they are still there at use time clearly documented. 2) I am also not a fan of getparam for individual ioctl flags since I don't think it scales nicely. How about add a param which returns all supported flags like I915_PARAM_USERPTR_SUPPORTED_FLAGS? Downside is it only works for 32-bit flag fields with getparam. Or it could be a query to solve that as well. Regards, Tvrtko > Testcase: igt/gem_userptr_blits/probe > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Kenneth Graunke <kenneth@whitecape.org> > Cc: Jason Ekstrand <jason@jlekstrand.net> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Ramalingam C <ramalingam.c@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++- > drivers/gpu/drm/i915/i915_getparam.c | 3 ++ > include/uapi/drm/i915_drm.h | 18 ++++++++++ > 3 files changed, 60 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > index 56edfeff8c02..fd6880328596 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { > > #endif > > +static int > +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len) > +{ > + const unsigned long end = addr + len; > + struct vm_area_struct *vma; > + int ret = -EFAULT; > + > + mmap_read_lock(mm); > + for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) { > + if (vma->vm_start > addr) > + break; > + > + if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP)) > + break; > + > + if (vma->vm_end >= end) { > + ret = 0; > + break; > + } > + > + addr = vma->vm_end; > + } > + mmap_read_unlock(mm); > + > + return ret; > +} > + > /* > * Creates a new mm object that wraps some normal memory from the process > * context - user memory. > @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > } > > if (args->flags & ~(I915_USERPTR_READ_ONLY | > - I915_USERPTR_UNSYNCHRONIZED)) > + I915_USERPTR_UNSYNCHRONIZED | > + I915_USERPTR_PROBE)) > return -EINVAL; > > if (i915_gem_object_size_2big(args->user_size)) > @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > return -ENODEV; > } > > + if (args->flags & I915_USERPTR_PROBE) { > + /* > + * Check that the range pointed to represents real struct > + * pages and not iomappings (at this moment in time!) > + */ > + ret = probe_range(current->mm, args->user_ptr, args->user_size); > + if (ret) > + return ret; > + } > + > #ifdef CONFIG_MMU_NOTIFIER > obj = i915_gem_object_alloc(); > if (obj == NULL) > diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c > index 24e18219eb50..d6d2e1a10d14 100644 > --- a/drivers/gpu/drm/i915/i915_getparam.c > +++ b/drivers/gpu/drm/i915/i915_getparam.c > @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > case I915_PARAM_PERF_REVISION: > value = i915_perf_ioctl_version(); > break; > + case I915_PARAM_HAS_USERPTR_PROBE: > + value = true; > + break; > default: > DRM_DEBUG("Unknown parameter %d\n", param->param); > return -EINVAL; > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index e20eeeca7a1c..2e4112bf4d38 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait { > */ > #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55 > > +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */ > +#define I915_PARAM_HAS_USERPTR_PROBE 56 > + > /* Must be kept compact -- no holes and well documented */ > > typedef struct drm_i915_getparam { > @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr { > * through the GTT. If the HW can't support readonly access, an error is > * returned. > * > + * I915_USERPTR_PROBE: > + * > + * Probe the provided @user_ptr range and validate that the @user_ptr is > + * indeed pointing to normal memory and that the range is also valid. > + * For example if some garbage address is given to the kernel, then this > + * should complain. > + * > + * Returns -EFAULT if the probe failed. > + * > + * Note that this doesn't populate the backing pages. > + * > + * The kernel supports this feature if I915_PARAM_HAS_USERPTR_PROBE > + * returns a non-zero value. > + * > * I915_USERPTR_UNSYNCHRONIZED: > * > * NOT USED. Setting this flag will result in an error. > */ > __u32 flags; > #define I915_USERPTR_READ_ONLY 0x1 > +#define I915_USERPTR_PROBE 0x2 > #define I915_USERPTR_UNSYNCHRONIZED 0x80000000 > /** > * @handle: Returned handle for the object. >
On Thu, Jul 15, 2021 at 11:33:10AM +0100, Tvrtko Ursulin wrote: > > On 15/07/2021 11:15, Matthew Auld wrote: > > From: Chris Wilson <chris@chris-wilson.co.uk> > > > > Jason Ekstrand requested a more efficient method than userptr+set-domain > > to determine if the userptr object was backed by a complete set of pages > > upon creation. To be more efficient than simply populating the userptr > > using get_user_pages() (as done by the call to set-domain or execbuf), > > we can walk the tree of vm_area_struct and check for gaps or vma not > > backed by struct page (VM_PFNMAP). The question is how to handle > > VM_MIXEDMAP which may be either struct page or pfn backed... > > > > With discrete are going to drop support for set_domain(), so offering a > > way to probe the pages, without having to resort to dummy batches has > > been requested. > > > > v2: > > - add new query param for the PROPBE flag, so userspace can easily > > check if the kernel supports it(Jason). > > - use mmap_read_{lock, unlock}. > > - add some kernel-doc. > > 1) > > I think probing is too weak to be offered as part of the uapi. What probes > successfully at create time might not be there anymore at usage time. So if > the pointer is not trusted at one point, why should it be at a later stage? > > Only thing which works for me is populate (so get_pages) at create time. But > again with no guarantees they are still there at use time clearly > documented. Populate is exactly as racy as probe. We don't support pinned userptr anymore. > 2) > > I am also not a fan of getparam for individual ioctl flags since I don't > think it scales nicely. How about add a param which returns all supported > flags like I915_PARAM_USERPTR_SUPPORTED_FLAGS? > > Downside is it only works for 32-bit flag fields with getparam. Or it could > be a query to solve that as well. I expect the actual userspace code will simply try with the probe flag first, and then without + set_domain. So strictly speaking we might not even have a need for the getparam. Otoh, let's not overthink/engineer this, whatever works for userspace is ok imo. A new query that lists all flags is the kind of fake-generic stuff that will like mis-estimate where the future actually lands, e.g. how do you encode if we add extensions to userptr to this? Which is something we absolutely can, by extending the struct at the end, which doesn't even need a new flag. Let's solve the immediate problem at hand, and not try to guess what more problems we might have in the future. -Daniel > Regards, > > Tvrtko > > > Testcase: igt/gem_userptr_blits/probe > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > Cc: Jordan Justen <jordan.l.justen@intel.com> > > Cc: Kenneth Graunke <kenneth@whitecape.org> > > Cc: Jason Ekstrand <jason@jlekstrand.net> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Ramalingam C <ramalingam.c@intel.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++- > > drivers/gpu/drm/i915/i915_getparam.c | 3 ++ > > include/uapi/drm/i915_drm.h | 18 ++++++++++ > > 3 files changed, 60 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > index 56edfeff8c02..fd6880328596 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { > > #endif > > +static int > > +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len) > > +{ > > + const unsigned long end = addr + len; > > + struct vm_area_struct *vma; > > + int ret = -EFAULT; > > + > > + mmap_read_lock(mm); > > + for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) { > > + if (vma->vm_start > addr) > > + break; > > + > > + if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP)) > > + break; > > + > > + if (vma->vm_end >= end) { > > + ret = 0; > > + break; > > + } > > + > > + addr = vma->vm_end; > > + } > > + mmap_read_unlock(mm); > > + > > + return ret; > > +} > > + > > /* > > * Creates a new mm object that wraps some normal memory from the process > > * context - user memory. > > @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > > } > > if (args->flags & ~(I915_USERPTR_READ_ONLY | > > - I915_USERPTR_UNSYNCHRONIZED)) > > + I915_USERPTR_UNSYNCHRONIZED | > > + I915_USERPTR_PROBE)) > > return -EINVAL; > > if (i915_gem_object_size_2big(args->user_size)) > > @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > > return -ENODEV; > > } > > + if (args->flags & I915_USERPTR_PROBE) { > > + /* > > + * Check that the range pointed to represents real struct > > + * pages and not iomappings (at this moment in time!) > > + */ > > + ret = probe_range(current->mm, args->user_ptr, args->user_size); > > + if (ret) > > + return ret; > > + } > > + > > #ifdef CONFIG_MMU_NOTIFIER > > obj = i915_gem_object_alloc(); > > if (obj == NULL) > > diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c > > index 24e18219eb50..d6d2e1a10d14 100644 > > --- a/drivers/gpu/drm/i915/i915_getparam.c > > +++ b/drivers/gpu/drm/i915/i915_getparam.c > > @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > > case I915_PARAM_PERF_REVISION: > > value = i915_perf_ioctl_version(); > > break; > > + case I915_PARAM_HAS_USERPTR_PROBE: > > + value = true; > > + break; > > default: > > DRM_DEBUG("Unknown parameter %d\n", param->param); > > return -EINVAL; > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > index e20eeeca7a1c..2e4112bf4d38 100644 > > --- a/include/uapi/drm/i915_drm.h > > +++ b/include/uapi/drm/i915_drm.h > > @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait { > > */ > > #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55 > > +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */ > > +#define I915_PARAM_HAS_USERPTR_PROBE 56 > > + > > /* Must be kept compact -- no holes and well documented */ > > typedef struct drm_i915_getparam { > > @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr { > > * through the GTT. If the HW can't support readonly access, an error is > > * returned. > > * > > + * I915_USERPTR_PROBE: > > + * > > + * Probe the provided @user_ptr range and validate that the @user_ptr is > > + * indeed pointing to normal memory and that the range is also valid. > > + * For example if some garbage address is given to the kernel, then this > > + * should complain. > > + * > > + * Returns -EFAULT if the probe failed. > > + * > > + * Note that this doesn't populate the backing pages. > > + * > > + * The kernel supports this feature if I915_PARAM_HAS_USERPTR_PROBE > > + * returns a non-zero value. > > + * > > * I915_USERPTR_UNSYNCHRONIZED: > > * > > * NOT USED. Setting this flag will result in an error. > > */ > > __u32 flags; > > #define I915_USERPTR_READ_ONLY 0x1 > > +#define I915_USERPTR_PROBE 0x2 > > #define I915_USERPTR_UNSYNCHRONIZED 0x80000000 > > /** > > * @handle: Returned handle for the object. > >
On Thu, 15 Jul 2021 at 11:33, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > > On 15/07/2021 11:15, Matthew Auld wrote: > > From: Chris Wilson <chris@chris-wilson.co.uk> > > > > Jason Ekstrand requested a more efficient method than userptr+set-domain > > to determine if the userptr object was backed by a complete set of pages > > upon creation. To be more efficient than simply populating the userptr > > using get_user_pages() (as done by the call to set-domain or execbuf), > > we can walk the tree of vm_area_struct and check for gaps or vma not > > backed by struct page (VM_PFNMAP). The question is how to handle > > VM_MIXEDMAP which may be either struct page or pfn backed... > > > > With discrete are going to drop support for set_domain(), so offering a > > way to probe the pages, without having to resort to dummy batches has > > been requested. > > > > v2: > > - add new query param for the PROPBE flag, so userspace can easily > > check if the kernel supports it(Jason). > > - use mmap_read_{lock, unlock}. > > - add some kernel-doc. > > 1) > > I think probing is too weak to be offered as part of the uapi. What > probes successfully at create time might not be there anymore at usage > time. So if the pointer is not trusted at one point, why should it be at > a later stage? > > Only thing which works for me is populate (so get_pages) at create time. > But again with no guarantees they are still there at use time clearly > documented. > > 2) > > I am also not a fan of getparam for individual ioctl flags since I don't > think it scales nicely. How about add a param which returns all > supported flags like I915_PARAM_USERPTR_SUPPORTED_FLAGS? > > Downside is it only works for 32-bit flag fields with getparam. Or it > could be a query to solve that as well. I guess. You don't think it's a little iffy though, since there were other flags which were added before this? So effectively userspace queries SUPPORTED_FLAGS and might get -EINVAL on older kernels, even though the flag is supported on that kernel(like READONLY)? Maybe a versioning scheme instead? I915_PARAM_USERPTR_VERSION? Seems quite common for other params. > > Regards, > > Tvrtko > > > Testcase: igt/gem_userptr_blits/probe > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > Cc: Jordan Justen <jordan.l.justen@intel.com> > > Cc: Kenneth Graunke <kenneth@whitecape.org> > > Cc: Jason Ekstrand <jason@jlekstrand.net> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Ramalingam C <ramalingam.c@intel.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++- > > drivers/gpu/drm/i915/i915_getparam.c | 3 ++ > > include/uapi/drm/i915_drm.h | 18 ++++++++++ > > 3 files changed, 60 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > index 56edfeff8c02..fd6880328596 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { > > > > #endif > > > > +static int > > +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len) > > +{ > > + const unsigned long end = addr + len; > > + struct vm_area_struct *vma; > > + int ret = -EFAULT; > > + > > + mmap_read_lock(mm); > > + for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) { > > + if (vma->vm_start > addr) > > + break; > > + > > + if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP)) > > + break; > > + > > + if (vma->vm_end >= end) { > > + ret = 0; > > + break; > > + } > > + > > + addr = vma->vm_end; > > + } > > + mmap_read_unlock(mm); > > + > > + return ret; > > +} > > + > > /* > > * Creates a new mm object that wraps some normal memory from the process > > * context - user memory. > > @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > > } > > > > if (args->flags & ~(I915_USERPTR_READ_ONLY | > > - I915_USERPTR_UNSYNCHRONIZED)) > > + I915_USERPTR_UNSYNCHRONIZED | > > + I915_USERPTR_PROBE)) > > return -EINVAL; > > > > if (i915_gem_object_size_2big(args->user_size)) > > @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > > return -ENODEV; > > } > > > > + if (args->flags & I915_USERPTR_PROBE) { > > + /* > > + * Check that the range pointed to represents real struct > > + * pages and not iomappings (at this moment in time!) > > + */ > > + ret = probe_range(current->mm, args->user_ptr, args->user_size); > > + if (ret) > > + return ret; > > + } > > + > > #ifdef CONFIG_MMU_NOTIFIER > > obj = i915_gem_object_alloc(); > > if (obj == NULL) > > diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c > > index 24e18219eb50..d6d2e1a10d14 100644 > > --- a/drivers/gpu/drm/i915/i915_getparam.c > > +++ b/drivers/gpu/drm/i915/i915_getparam.c > > @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > > case I915_PARAM_PERF_REVISION: > > value = i915_perf_ioctl_version(); > > break; > > + case I915_PARAM_HAS_USERPTR_PROBE: > > + value = true; > > + break; > > default: > > DRM_DEBUG("Unknown parameter %d\n", param->param); > > return -EINVAL; > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > index e20eeeca7a1c..2e4112bf4d38 100644 > > --- a/include/uapi/drm/i915_drm.h > > +++ b/include/uapi/drm/i915_drm.h > > @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait { > > */ > > #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55 > > > > +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */ > > +#define I915_PARAM_HAS_USERPTR_PROBE 56 > > + > > /* Must be kept compact -- no holes and well documented */ > > > > typedef struct drm_i915_getparam { > > @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr { > > * through the GTT. If the HW can't support readonly access, an error is > > * returned. > > * > > + * I915_USERPTR_PROBE: > > + * > > + * Probe the provided @user_ptr range and validate that the @user_ptr is > > + * indeed pointing to normal memory and that the range is also valid. > > + * For example if some garbage address is given to the kernel, then this > > + * should complain. > > + * > > + * Returns -EFAULT if the probe failed. > > + * > > + * Note that this doesn't populate the backing pages. > > + * > > + * The kernel supports this feature if I915_PARAM_HAS_USERPTR_PROBE > > + * returns a non-zero value. > > + * > > * I915_USERPTR_UNSYNCHRONIZED: > > * > > * NOT USED. Setting this flag will result in an error. > > */ > > __u32 flags; > > #define I915_USERPTR_READ_ONLY 0x1 > > +#define I915_USERPTR_PROBE 0x2 > > #define I915_USERPTR_UNSYNCHRONIZED 0x80000000 > > /** > > * @handle: Returned handle for the object. > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 15/07/2021 12:07, Daniel Vetter wrote: > On Thu, Jul 15, 2021 at 11:33:10AM +0100, Tvrtko Ursulin wrote: >> >> On 15/07/2021 11:15, Matthew Auld wrote: >>> From: Chris Wilson <chris@chris-wilson.co.uk> >>> >>> Jason Ekstrand requested a more efficient method than userptr+set-domain >>> to determine if the userptr object was backed by a complete set of pages >>> upon creation. To be more efficient than simply populating the userptr >>> using get_user_pages() (as done by the call to set-domain or execbuf), >>> we can walk the tree of vm_area_struct and check for gaps or vma not >>> backed by struct page (VM_PFNMAP). The question is how to handle >>> VM_MIXEDMAP which may be either struct page or pfn backed... >>> >>> With discrete are going to drop support for set_domain(), so offering a >>> way to probe the pages, without having to resort to dummy batches has >>> been requested. >>> >>> v2: >>> - add new query param for the PROPBE flag, so userspace can easily >>> check if the kernel supports it(Jason). >>> - use mmap_read_{lock, unlock}. >>> - add some kernel-doc. >> >> 1) >> >> I think probing is too weak to be offered as part of the uapi. What probes >> successfully at create time might not be there anymore at usage time. So if >> the pointer is not trusted at one point, why should it be at a later stage? >> >> Only thing which works for me is populate (so get_pages) at create time. But >> again with no guarantees they are still there at use time clearly >> documented. > > Populate is exactly as racy as probe. We don't support pinned userptr > anymore. Yes, wrote so myself - "..again with no guarantees they are still there at use time..". Perhaps I don't understand what problem is probe supposed to solve. It doesn't deal 1:1 with set_domain removal since that one actually did get_pages so that would be populate. But fact remains regardless that if userspace is given a pointer it doesn't trust, _and_ wants the check it for this reason or that, then probe solves nothing. Unless there is actually at minimum some protocol to reply to whoever sent the pointer like "not that pointer please". >> 2) >> >> I am also not a fan of getparam for individual ioctl flags since I don't >> think it scales nicely. How about add a param which returns all supported >> flags like I915_PARAM_USERPTR_SUPPORTED_FLAGS? >> >> Downside is it only works for 32-bit flag fields with getparam. Or it could >> be a query to solve that as well. > > I expect the actual userspace code will simply try with the probe flag > first, and then without + set_domain. So strictly speaking we might not > even have a need for the getparam. > > Otoh, let's not overthink/engineer this, whatever works for userspace is > ok imo. A new query that lists all flags is the kind of fake-generic stuff > that will like mis-estimate where the future actually lands, e.g. how do > you encode if we add extensions to userptr to this? Which is something we > absolutely can, by extending the struct at the end, which doesn't even > need a new flag. > > Let's solve the immediate problem at hand, and not try to guess what more > problems we might have in the future. Yeah I am guessing if anyone is using some of the other userptr flags they don't have a getparam for that either. At least that would be consolidated with I915_PARAM_USERPTR_SUPPORTED_FLAGS. There is no requirement that getparam needs to keep supporting future extensions to the struct itself, equally as I915_PARAM_HAS_USERPTR_PROBE is directly tied to the same flags field, only a subset of it. Regards, Tvrtko > -Daniel > >> Regards, >> >> Tvrtko >> >>> Testcase: igt/gem_userptr_blits/probe >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >>> Cc: Jordan Justen <jordan.l.justen@intel.com> >>> Cc: Kenneth Graunke <kenneth@whitecape.org> >>> Cc: Jason Ekstrand <jason@jlekstrand.net> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Cc: Ramalingam C <ramalingam.c@intel.com> >>> --- >>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++- >>> drivers/gpu/drm/i915/i915_getparam.c | 3 ++ >>> include/uapi/drm/i915_drm.h | 18 ++++++++++ >>> 3 files changed, 60 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>> index 56edfeff8c02..fd6880328596 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>> @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { >>> #endif >>> +static int >>> +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len) >>> +{ >>> + const unsigned long end = addr + len; >>> + struct vm_area_struct *vma; >>> + int ret = -EFAULT; >>> + >>> + mmap_read_lock(mm); >>> + for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) { >>> + if (vma->vm_start > addr) >>> + break; >>> + >>> + if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP)) >>> + break; >>> + >>> + if (vma->vm_end >= end) { >>> + ret = 0; >>> + break; >>> + } >>> + >>> + addr = vma->vm_end; >>> + } >>> + mmap_read_unlock(mm); >>> + >>> + return ret; >>> +} >>> + >>> /* >>> * Creates a new mm object that wraps some normal memory from the process >>> * context - user memory. >>> @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev, >>> } >>> if (args->flags & ~(I915_USERPTR_READ_ONLY | >>> - I915_USERPTR_UNSYNCHRONIZED)) >>> + I915_USERPTR_UNSYNCHRONIZED | >>> + I915_USERPTR_PROBE)) >>> return -EINVAL; >>> if (i915_gem_object_size_2big(args->user_size)) >>> @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev, >>> return -ENODEV; >>> } >>> + if (args->flags & I915_USERPTR_PROBE) { >>> + /* >>> + * Check that the range pointed to represents real struct >>> + * pages and not iomappings (at this moment in time!) >>> + */ >>> + ret = probe_range(current->mm, args->user_ptr, args->user_size); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> #ifdef CONFIG_MMU_NOTIFIER >>> obj = i915_gem_object_alloc(); >>> if (obj == NULL) >>> diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c >>> index 24e18219eb50..d6d2e1a10d14 100644 >>> --- a/drivers/gpu/drm/i915/i915_getparam.c >>> +++ b/drivers/gpu/drm/i915/i915_getparam.c >>> @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, >>> case I915_PARAM_PERF_REVISION: >>> value = i915_perf_ioctl_version(); >>> break; >>> + case I915_PARAM_HAS_USERPTR_PROBE: >>> + value = true; >>> + break; >>> default: >>> DRM_DEBUG("Unknown parameter %d\n", param->param); >>> return -EINVAL; >>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h >>> index e20eeeca7a1c..2e4112bf4d38 100644 >>> --- a/include/uapi/drm/i915_drm.h >>> +++ b/include/uapi/drm/i915_drm.h >>> @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait { >>> */ >>> #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55 >>> +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */ >>> +#define I915_PARAM_HAS_USERPTR_PROBE 56 >>> + >>> /* Must be kept compact -- no holes and well documented */ >>> typedef struct drm_i915_getparam { >>> @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr { >>> * through the GTT. If the HW can't support readonly access, an error is >>> * returned. >>> * >>> + * I915_USERPTR_PROBE: >>> + * >>> + * Probe the provided @user_ptr range and validate that the @user_ptr is >>> + * indeed pointing to normal memory and that the range is also valid. >>> + * For example if some garbage address is given to the kernel, then this >>> + * should complain. >>> + * >>> + * Returns -EFAULT if the probe failed. >>> + * >>> + * Note that this doesn't populate the backing pages. >>> + * >>> + * The kernel supports this feature if I915_PARAM_HAS_USERPTR_PROBE >>> + * returns a non-zero value. >>> + * >>> * I915_USERPTR_UNSYNCHRONIZED: >>> * >>> * NOT USED. Setting this flag will result in an error. >>> */ >>> __u32 flags; >>> #define I915_USERPTR_READ_ONLY 0x1 >>> +#define I915_USERPTR_PROBE 0x2 >>> #define I915_USERPTR_UNSYNCHRONIZED 0x80000000 >>> /** >>> * @handle: Returned handle for the object. >>> >
On 15/07/2021 12:09, Matthew Auld wrote: > On Thu, 15 Jul 2021 at 11:33, Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com> wrote: >> >> >> On 15/07/2021 11:15, Matthew Auld wrote: >>> From: Chris Wilson <chris@chris-wilson.co.uk> >>> >>> Jason Ekstrand requested a more efficient method than userptr+set-domain >>> to determine if the userptr object was backed by a complete set of pages >>> upon creation. To be more efficient than simply populating the userptr >>> using get_user_pages() (as done by the call to set-domain or execbuf), >>> we can walk the tree of vm_area_struct and check for gaps or vma not >>> backed by struct page (VM_PFNMAP). The question is how to handle >>> VM_MIXEDMAP which may be either struct page or pfn backed... >>> >>> With discrete are going to drop support for set_domain(), so offering a >>> way to probe the pages, without having to resort to dummy batches has >>> been requested. >>> >>> v2: >>> - add new query param for the PROPBE flag, so userspace can easily >>> check if the kernel supports it(Jason). >>> - use mmap_read_{lock, unlock}. >>> - add some kernel-doc. >> >> 1) >> >> I think probing is too weak to be offered as part of the uapi. What >> probes successfully at create time might not be there anymore at usage >> time. So if the pointer is not trusted at one point, why should it be at >> a later stage? >> >> Only thing which works for me is populate (so get_pages) at create time. >> But again with no guarantees they are still there at use time clearly >> documented. >> >> 2) >> >> I am also not a fan of getparam for individual ioctl flags since I don't >> think it scales nicely. How about add a param which returns all >> supported flags like I915_PARAM_USERPTR_SUPPORTED_FLAGS? >> >> Downside is it only works for 32-bit flag fields with getparam. Or it >> could be a query to solve that as well. > > I guess. You don't think it's a little iffy though, since there were > other flags which were added before this? So effectively userspace > queries SUPPORTED_FLAGS and might get -EINVAL on older kernels, even > though the flag is supported on that kernel(like READONLY)? For me it is probably passable since unsupported getparam fundamentally doesn't imply unsupported features predating that getparam. Same as for example unsupported engine query does not mean there are no engines. But anyway, seems question will be resolved by Daniel so don't pay attention to me. Regards, Tvrtko > > Maybe a versioning scheme instead? I915_PARAM_USERPTR_VERSION? Seems > quite common for other params. > >> >> Regards, >> >> Tvrtko >> >>> Testcase: igt/gem_userptr_blits/probe >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >>> Cc: Jordan Justen <jordan.l.justen@intel.com> >>> Cc: Kenneth Graunke <kenneth@whitecape.org> >>> Cc: Jason Ekstrand <jason@jlekstrand.net> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Cc: Ramalingam C <ramalingam.c@intel.com> >>> --- >>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++- >>> drivers/gpu/drm/i915/i915_getparam.c | 3 ++ >>> include/uapi/drm/i915_drm.h | 18 ++++++++++ >>> 3 files changed, 60 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>> index 56edfeff8c02..fd6880328596 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>> @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { >>> >>> #endif >>> >>> +static int >>> +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len) >>> +{ >>> + const unsigned long end = addr + len; >>> + struct vm_area_struct *vma; >>> + int ret = -EFAULT; >>> + >>> + mmap_read_lock(mm); >>> + for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) { >>> + if (vma->vm_start > addr) >>> + break; >>> + >>> + if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP)) >>> + break; >>> + >>> + if (vma->vm_end >= end) { >>> + ret = 0; >>> + break; >>> + } >>> + >>> + addr = vma->vm_end; >>> + } >>> + mmap_read_unlock(mm); >>> + >>> + return ret; >>> +} >>> + >>> /* >>> * Creates a new mm object that wraps some normal memory from the process >>> * context - user memory. >>> @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev, >>> } >>> >>> if (args->flags & ~(I915_USERPTR_READ_ONLY | >>> - I915_USERPTR_UNSYNCHRONIZED)) >>> + I915_USERPTR_UNSYNCHRONIZED | >>> + I915_USERPTR_PROBE)) >>> return -EINVAL; >>> >>> if (i915_gem_object_size_2big(args->user_size)) >>> @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev, >>> return -ENODEV; >>> } >>> >>> + if (args->flags & I915_USERPTR_PROBE) { >>> + /* >>> + * Check that the range pointed to represents real struct >>> + * pages and not iomappings (at this moment in time!) >>> + */ >>> + ret = probe_range(current->mm, args->user_ptr, args->user_size); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> #ifdef CONFIG_MMU_NOTIFIER >>> obj = i915_gem_object_alloc(); >>> if (obj == NULL) >>> diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c >>> index 24e18219eb50..d6d2e1a10d14 100644 >>> --- a/drivers/gpu/drm/i915/i915_getparam.c >>> +++ b/drivers/gpu/drm/i915/i915_getparam.c >>> @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, >>> case I915_PARAM_PERF_REVISION: >>> value = i915_perf_ioctl_version(); >>> break; >>> + case I915_PARAM_HAS_USERPTR_PROBE: >>> + value = true; >>> + break; >>> default: >>> DRM_DEBUG("Unknown parameter %d\n", param->param); >>> return -EINVAL; >>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h >>> index e20eeeca7a1c..2e4112bf4d38 100644 >>> --- a/include/uapi/drm/i915_drm.h >>> +++ b/include/uapi/drm/i915_drm.h >>> @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait { >>> */ >>> #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55 >>> >>> +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */ >>> +#define I915_PARAM_HAS_USERPTR_PROBE 56 >>> + >>> /* Must be kept compact -- no holes and well documented */ >>> >>> typedef struct drm_i915_getparam { >>> @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr { >>> * through the GTT. If the HW can't support readonly access, an error is >>> * returned. >>> * >>> + * I915_USERPTR_PROBE: >>> + * >>> + * Probe the provided @user_ptr range and validate that the @user_ptr is >>> + * indeed pointing to normal memory and that the range is also valid. >>> + * For example if some garbage address is given to the kernel, then this >>> + * should complain. >>> + * >>> + * Returns -EFAULT if the probe failed. >>> + * >>> + * Note that this doesn't populate the backing pages. >>> + * >>> + * The kernel supports this feature if I915_PARAM_HAS_USERPTR_PROBE >>> + * returns a non-zero value. >>> + * >>> * I915_USERPTR_UNSYNCHRONIZED: >>> * >>> * NOT USED. Setting this flag will result in an error. >>> */ >>> __u32 flags; >>> #define I915_USERPTR_READ_ONLY 0x1 >>> +#define I915_USERPTR_PROBE 0x2 >>> #define I915_USERPTR_UNSYNCHRONIZED 0x80000000 >>> /** >>> * @handle: Returned handle for the object. >>> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thursday, July 15, 2021 4:27:44 AM PDT Tvrtko Ursulin wrote: > > On 15/07/2021 12:07, Daniel Vetter wrote: > > On Thu, Jul 15, 2021 at 11:33:10AM +0100, Tvrtko Ursulin wrote: > >> > >> On 15/07/2021 11:15, Matthew Auld wrote: > >>> From: Chris Wilson <chris@chris-wilson.co.uk> > >>> > >>> Jason Ekstrand requested a more efficient method than userptr+set-domain > >>> to determine if the userptr object was backed by a complete set of pages > >>> upon creation. To be more efficient than simply populating the userptr > >>> using get_user_pages() (as done by the call to set-domain or execbuf), > >>> we can walk the tree of vm_area_struct and check for gaps or vma not > >>> backed by struct page (VM_PFNMAP). The question is how to handle > >>> VM_MIXEDMAP which may be either struct page or pfn backed... > >>> > >>> With discrete are going to drop support for set_domain(), so offering a > >>> way to probe the pages, without having to resort to dummy batches has > >>> been requested. > >>> > >>> v2: > >>> - add new query param for the PROPBE flag, so userspace can easily > >>> check if the kernel supports it(Jason). > >>> - use mmap_read_{lock, unlock}. > >>> - add some kernel-doc. > >> > >> 1) > >> > >> I think probing is too weak to be offered as part of the uapi. What probes > >> successfully at create time might not be there anymore at usage time. So if > >> the pointer is not trusted at one point, why should it be at a later stage? > >> > >> Only thing which works for me is populate (so get_pages) at create time. But > >> again with no guarantees they are still there at use time clearly > >> documented. > > > > Populate is exactly as racy as probe. We don't support pinned userptr > > anymore. > > Yes, wrote so myself - "..again with no guarantees they are still there > at use time..". > > Perhaps I don't understand what problem is probe supposed to solve. It > doesn't deal 1:1 with set_domain removal since that one actually did > get_pages so that would be populate. But fact remains regardless that if > userspace is given a pointer it doesn't trust, _and_ wants the check it > for this reason or that, then probe solves nothing. Unless there is > actually at minimum some protocol to reply to whoever sent the pointer > like "not that pointer please". That's exactly the point. GL_AMD_pinned_memory requires us the OpenGL implementation to return an error for "not that pointer, please", at the time when said pointer is supplied - not at first use. Sure, there can be reasons why it might seem fine up front, and not work later. But an early check of "just no, you're doing it totally wrong" at the right moment can be helpful for application developers. While it shouldn't really happen, if it ever did, it would be a lot more obvious to debug than "much later on, when something randomly flushed the GPU commands we were building, something went wrong, and we don't know why." --Ken
On Thu, Jul 15, 2021 at 1:21 PM Kenneth Graunke <kenneth@whitecape.org> wrote: > > On Thursday, July 15, 2021 4:27:44 AM PDT Tvrtko Ursulin wrote: > > > > On 15/07/2021 12:07, Daniel Vetter wrote: > > > On Thu, Jul 15, 2021 at 11:33:10AM +0100, Tvrtko Ursulin wrote: > > >> > > >> On 15/07/2021 11:15, Matthew Auld wrote: > > >>> From: Chris Wilson <chris@chris-wilson.co.uk> > > >>> > > >>> Jason Ekstrand requested a more efficient method than userptr+set-domain > > >>> to determine if the userptr object was backed by a complete set of pages > > >>> upon creation. To be more efficient than simply populating the userptr > > >>> using get_user_pages() (as done by the call to set-domain or execbuf), > > >>> we can walk the tree of vm_area_struct and check for gaps or vma not > > >>> backed by struct page (VM_PFNMAP). The question is how to handle > > >>> VM_MIXEDMAP which may be either struct page or pfn backed... > > >>> > > >>> With discrete are going to drop support for set_domain(), so offering a > > >>> way to probe the pages, without having to resort to dummy batches has > > >>> been requested. > > >>> > > >>> v2: > > >>> - add new query param for the PROPBE flag, so userspace can easily > > >>> check if the kernel supports it(Jason). > > >>> - use mmap_read_{lock, unlock}. > > >>> - add some kernel-doc. > > >> > > >> 1) > > >> > > >> I think probing is too weak to be offered as part of the uapi. What probes > > >> successfully at create time might not be there anymore at usage time. So if > > >> the pointer is not trusted at one point, why should it be at a later stage? > > >> > > >> Only thing which works for me is populate (so get_pages) at create time. But > > >> again with no guarantees they are still there at use time clearly > > >> documented. > > > > > > Populate is exactly as racy as probe. We don't support pinned userptr > > > anymore. > > > > Yes, wrote so myself - "..again with no guarantees they are still there > > at use time..". > > > > Perhaps I don't understand what problem is probe supposed to solve. It > > doesn't deal 1:1 with set_domain removal since that one actually did > > get_pages so that would be populate. But fact remains regardless that if > > userspace is given a pointer it doesn't trust, _and_ wants the check it > > for this reason or that, then probe solves nothing. Unless there is > > actually at minimum some protocol to reply to whoever sent the pointer > > like "not that pointer please". > > That's exactly the point. GL_AMD_pinned_memory requires us the OpenGL > implementation to return an error for "not that pointer, please", at the > time when said pointer is supplied - not at first use. > > Sure, there can be reasons why it might seem fine up front, and not work > later. But an early check of "just no, you're doing it totally wrong" > at the right moment can be helpful for application developers. While it > shouldn't really happen, if it ever did, it would be a lot more obvious > to debug than "much later on, when something randomly flushed the GPU > commands we were building, something went wrong, and we don't know why." And before someone chimes in saying that Vulkan doesn't need this because we can just VK_ERROR_DEVICE_LOST later, that's not true. We'd like to be able to return VK_ERROR_INVALID_EXTERNAL_HANDLE in the easily checkable cases like if they try to pass in a mmapped file. --Jason
On 15/07/2021 19:21, Kenneth Graunke wrote: > On Thursday, July 15, 2021 4:27:44 AM PDT Tvrtko Ursulin wrote: >> >> On 15/07/2021 12:07, Daniel Vetter wrote: >>> On Thu, Jul 15, 2021 at 11:33:10AM +0100, Tvrtko Ursulin wrote: >>>> >>>> On 15/07/2021 11:15, Matthew Auld wrote: >>>>> From: Chris Wilson <chris@chris-wilson.co.uk> >>>>> >>>>> Jason Ekstrand requested a more efficient method than userptr+set-domain >>>>> to determine if the userptr object was backed by a complete set of pages >>>>> upon creation. To be more efficient than simply populating the userptr >>>>> using get_user_pages() (as done by the call to set-domain or execbuf), >>>>> we can walk the tree of vm_area_struct and check for gaps or vma not >>>>> backed by struct page (VM_PFNMAP). The question is how to handle >>>>> VM_MIXEDMAP which may be either struct page or pfn backed... >>>>> >>>>> With discrete are going to drop support for set_domain(), so offering a >>>>> way to probe the pages, without having to resort to dummy batches has >>>>> been requested. >>>>> >>>>> v2: >>>>> - add new query param for the PROPBE flag, so userspace can easily >>>>> check if the kernel supports it(Jason). >>>>> - use mmap_read_{lock, unlock}. >>>>> - add some kernel-doc. >>>> >>>> 1) >>>> >>>> I think probing is too weak to be offered as part of the uapi. What probes >>>> successfully at create time might not be there anymore at usage time. So if >>>> the pointer is not trusted at one point, why should it be at a later stage? >>>> >>>> Only thing which works for me is populate (so get_pages) at create time. But >>>> again with no guarantees they are still there at use time clearly >>>> documented. >>> >>> Populate is exactly as racy as probe. We don't support pinned userptr >>> anymore. >> >> Yes, wrote so myself - "..again with no guarantees they are still there >> at use time..". >> >> Perhaps I don't understand what problem is probe supposed to solve. It >> doesn't deal 1:1 with set_domain removal since that one actually did >> get_pages so that would be populate. But fact remains regardless that if >> userspace is given a pointer it doesn't trust, _and_ wants the check it >> for this reason or that, then probe solves nothing. Unless there is >> actually at minimum some protocol to reply to whoever sent the pointer >> like "not that pointer please". > > That's exactly the point. GL_AMD_pinned_memory requires us the OpenGL > implementation to return an error for "not that pointer, please", at the > time when said pointer is supplied - not at first use. > > Sure, there can be reasons why it might seem fine up front, and not work > later. But an early check of "just no, you're doing it totally wrong" > at the right moment can be helpful for application developers. While it > shouldn't really happen, if it ever did, it would be a lot more obvious > to debug than "much later on, when something randomly flushed the GPU > commands we were building, something went wrong, and we don't know why." Ack, thanks for the clarification. For this limited use case I agree probe works. Regards, Tvrtko P.S. I made a mistake (?) of looking at the GL_AMD_pinned_memory spec: """ 3) Can the application free the memory? RESOLVED: YES. However, the underlying buffer object should have been deleted from the OpenGL to prevent any access by the GPU, or the result is undefined, including program or even system termination. """ Scary stuff that spec of userspace level API would allow such kernel bugs!
On 15/07/2021 11:15, Matthew Auld wrote: > From: Chris Wilson <chris@chris-wilson.co.uk> > > Jason Ekstrand requested a more efficient method than userptr+set-domain > to determine if the userptr object was backed by a complete set of pages > upon creation. To be more efficient than simply populating the userptr > using get_user_pages() (as done by the call to set-domain or execbuf), > we can walk the tree of vm_area_struct and check for gaps or vma not > backed by struct page (VM_PFNMAP). The question is how to handle > VM_MIXEDMAP which may be either struct page or pfn backed... > > With discrete are going to drop support for set_domain(), so offering a > way to probe the pages, without having to resort to dummy batches has > been requested. > > v2: > - add new query param for the PROPBE flag, so userspace can easily PROBE > check if the kernel supports it(Jason). > - use mmap_read_{lock, unlock}. > - add some kernel-doc. > > Testcase: igt/gem_userptr_blits/probe > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Kenneth Graunke <kenneth@whitecape.org> > Cc: Jason Ekstrand <jason@jlekstrand.net> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Ramalingam C <ramalingam.c@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++- > drivers/gpu/drm/i915/i915_getparam.c | 3 ++ > include/uapi/drm/i915_drm.h | 18 ++++++++++ > 3 files changed, 60 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > index 56edfeff8c02..fd6880328596 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { > > #endif > > +static int > +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len) > +{ > + const unsigned long end = addr + len; > + struct vm_area_struct *vma; > + int ret = -EFAULT; > + > + mmap_read_lock(mm); > + for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) { > + if (vma->vm_start > addr) > + break; > + > + if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP)) > + break; > + > + if (vma->vm_end >= end) { > + ret = 0; > + break; > + } > + > + addr = vma->vm_end; > + } > + mmap_read_unlock(mm); Logic here looks good to me. > + > + return ret; > +} > + > /* > * Creates a new mm object that wraps some normal memory from the process > * context - user memory. > @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > } > > if (args->flags & ~(I915_USERPTR_READ_ONLY | > - I915_USERPTR_UNSYNCHRONIZED)) > + I915_USERPTR_UNSYNCHRONIZED | > + I915_USERPTR_PROBE)) > return -EINVAL; > > if (i915_gem_object_size_2big(args->user_size)) > @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > return -ENODEV; > } > > + if (args->flags & I915_USERPTR_PROBE) { > + /* > + * Check that the range pointed to represents real struct > + * pages and not iomappings (at this moment in time!) > + */ > + ret = probe_range(current->mm, args->user_ptr, args->user_size); > + if (ret) > + return ret; > + } > + > #ifdef CONFIG_MMU_NOTIFIER > obj = i915_gem_object_alloc(); > if (obj == NULL) > diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c > index 24e18219eb50..d6d2e1a10d14 100644 > --- a/drivers/gpu/drm/i915/i915_getparam.c > +++ b/drivers/gpu/drm/i915/i915_getparam.c > @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > case I915_PARAM_PERF_REVISION: > value = i915_perf_ioctl_version(); > break; > + case I915_PARAM_HAS_USERPTR_PROBE: > + value = true; > + break; > default: > DRM_DEBUG("Unknown parameter %d\n", param->param); > return -EINVAL; > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index e20eeeca7a1c..2e4112bf4d38 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait { > */ > #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55 > > +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */ > +#define I915_PARAM_HAS_USERPTR_PROBE 56 > + > /* Must be kept compact -- no holes and well documented */ > > typedef struct drm_i915_getparam { > @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr { > * through the GTT. If the HW can't support readonly access, an error is > * returned. > * > + * I915_USERPTR_PROBE: > + * > + * Probe the provided @user_ptr range and validate that the @user_ptr is > + * indeed pointing to normal memory and that the range is also valid. > + * For example if some garbage address is given to the kernel, then this > + * should complain. > + * > + * Returns -EFAULT if the probe failed. > + * > + * Note that this doesn't populate the backing pages. I'd also add a note on how validation at create time may not mean object will still be valid at use time. With that added, and ignoring the question of whether to have setparam or not: Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko > + * > + * The kernel supports this feature if I915_PARAM_HAS_USERPTR_PROBE > + * returns a non-zero value. > + * > * I915_USERPTR_UNSYNCHRONIZED: > * > * NOT USED. Setting this flag will result in an error. > */ > __u32 flags; > #define I915_USERPTR_READ_ONLY 0x1 > +#define I915_USERPTR_PROBE 0x2 > #define I915_USERPTR_UNSYNCHRONIZED 0x80000000 > /** > * @handle: Returned handle for the object. >
On Thu, Jul 15, 2021 at 8:21 PM Kenneth Graunke <kenneth@whitecape.org> wrote: > > On Thursday, July 15, 2021 4:27:44 AM PDT Tvrtko Ursulin wrote: > > > > On 15/07/2021 12:07, Daniel Vetter wrote: > > > On Thu, Jul 15, 2021 at 11:33:10AM +0100, Tvrtko Ursulin wrote: > > >> > > >> On 15/07/2021 11:15, Matthew Auld wrote: > > >>> From: Chris Wilson <chris@chris-wilson.co.uk> > > >>> > > >>> Jason Ekstrand requested a more efficient method than userptr+set-domain > > >>> to determine if the userptr object was backed by a complete set of pages > > >>> upon creation. To be more efficient than simply populating the userptr > > >>> using get_user_pages() (as done by the call to set-domain or execbuf), > > >>> we can walk the tree of vm_area_struct and check for gaps or vma not > > >>> backed by struct page (VM_PFNMAP). The question is how to handle > > >>> VM_MIXEDMAP which may be either struct page or pfn backed... > > >>> > > >>> With discrete are going to drop support for set_domain(), so offering a > > >>> way to probe the pages, without having to resort to dummy batches has > > >>> been requested. > > >>> > > >>> v2: > > >>> - add new query param for the PROPBE flag, so userspace can easily > > >>> check if the kernel supports it(Jason). > > >>> - use mmap_read_{lock, unlock}. > > >>> - add some kernel-doc. > > >> > > >> 1) > > >> > > >> I think probing is too weak to be offered as part of the uapi. What probes > > >> successfully at create time might not be there anymore at usage time. So if > > >> the pointer is not trusted at one point, why should it be at a later stage? > > >> > > >> Only thing which works for me is populate (so get_pages) at create time. But > > >> again with no guarantees they are still there at use time clearly > > >> documented. > > > > > > Populate is exactly as racy as probe. We don't support pinned userptr > > > anymore. > > > > Yes, wrote so myself - "..again with no guarantees they are still there > > at use time..". > > > > Perhaps I don't understand what problem is probe supposed to solve. It > > doesn't deal 1:1 with set_domain removal since that one actually did > > get_pages so that would be populate. But fact remains regardless that if > > userspace is given a pointer it doesn't trust, _and_ wants the check it > > for this reason or that, then probe solves nothing. Unless there is > > actually at minimum some protocol to reply to whoever sent the pointer > > like "not that pointer please". > > That's exactly the point. GL_AMD_pinned_memory requires us the OpenGL > implementation to return an error for "not that pointer, please", at the > time when said pointer is supplied - not at first use. > > Sure, there can be reasons why it might seem fine up front, and not work > later. But an early check of "just no, you're doing it totally wrong" > at the right moment can be helpful for application developers. While it > shouldn't really happen, if it ever did, it would be a lot more obvious > to debug than "much later on, when something randomly flushed the GPU > commands we were building, something went wrong, and we don't know why." Also, that extension doesn't make guarantees about importing nasty userspace memory where some non-trusted entity could e.g. truncate() the file and render all pages invalid, even if you're holding a reference to it still. It's purely to check "hey I got this random pointer here from somewhere, I trust it, can you use it assuming I will not change anything with hit?". Which is exactly what probe solves, and pulling in the pages is kinda overkill. -Daniel
Thanks for this! Series is: Acked-by: Kenneth Graunke <kenneth@whitecape.org> https://gitlab.freedesktop.org/kwg/mesa/-/commits/iris-userptr-probe is an untested branch that uses the new probe API in Mesa. On Thursday, July 15, 2021 3:15:35 AM PDT Matthew Auld wrote: > From: Chris Wilson <chris@chris-wilson.co.uk> > > Jason Ekstrand requested a more efficient method than userptr+set-domain > to determine if the userptr object was backed by a complete set of pages > upon creation. To be more efficient than simply populating the userptr > using get_user_pages() (as done by the call to set-domain or execbuf), > we can walk the tree of vm_area_struct and check for gaps or vma not > backed by struct page (VM_PFNMAP). The question is how to handle > VM_MIXEDMAP which may be either struct page or pfn backed... > > With discrete are going to drop support for set_domain(), so offering a > way to probe the pages, without having to resort to dummy batches has > been requested. > > v2: > - add new query param for the PROPBE flag, so userspace can easily > check if the kernel supports it(Jason). > - use mmap_read_{lock, unlock}. > - add some kernel-doc. > > Testcase: igt/gem_userptr_blits/probe > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Kenneth Graunke <kenneth@whitecape.org> > Cc: Jason Ekstrand <jason@jlekstrand.net> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Ramalingam C <ramalingam.c@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++- > drivers/gpu/drm/i915/i915_getparam.c | 3 ++ > include/uapi/drm/i915_drm.h | 18 ++++++++++ > 3 files changed, 60 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > index 56edfeff8c02..fd6880328596 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { > > #endif > > +static int > +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len) > +{ > + const unsigned long end = addr + len; > + struct vm_area_struct *vma; > + int ret = -EFAULT; > + > + mmap_read_lock(mm); > + for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) { > + if (vma->vm_start > addr) > + break; > + > + if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP)) > + break; > + > + if (vma->vm_end >= end) { > + ret = 0; > + break; > + } > + > + addr = vma->vm_end; > + } > + mmap_read_unlock(mm); > + > + return ret; > +} > + > /* > * Creates a new mm object that wraps some normal memory from the process > * context - user memory. > @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > } > > if (args->flags & ~(I915_USERPTR_READ_ONLY | > - I915_USERPTR_UNSYNCHRONIZED)) > + I915_USERPTR_UNSYNCHRONIZED | > + I915_USERPTR_PROBE)) > return -EINVAL; > > if (i915_gem_object_size_2big(args->user_size)) > @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > return -ENODEV; > } > > + if (args->flags & I915_USERPTR_PROBE) { > + /* > + * Check that the range pointed to represents real struct > + * pages and not iomappings (at this moment in time!) > + */ > + ret = probe_range(current->mm, args->user_ptr, args->user_size); > + if (ret) > + return ret; > + } > + > #ifdef CONFIG_MMU_NOTIFIER > obj = i915_gem_object_alloc(); > if (obj == NULL) > diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c > index 24e18219eb50..d6d2e1a10d14 100644 > --- a/drivers/gpu/drm/i915/i915_getparam.c > +++ b/drivers/gpu/drm/i915/i915_getparam.c > @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > case I915_PARAM_PERF_REVISION: > value = i915_perf_ioctl_version(); > break; > + case I915_PARAM_HAS_USERPTR_PROBE: > + value = true; > + break; > default: > DRM_DEBUG("Unknown parameter %d\n", param->param); > return -EINVAL; > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index e20eeeca7a1c..2e4112bf4d38 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait { > */ > #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55 > > +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */ > +#define I915_PARAM_HAS_USERPTR_PROBE 56 > + > /* Must be kept compact -- no holes and well documented */ > > typedef struct drm_i915_getparam { > @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr { > * through the GTT. If the HW can't support readonly access, an error is > * returned. > * > + * I915_USERPTR_PROBE: > + * > + * Probe the provided @user_ptr range and validate that the @user_ptr is > + * indeed pointing to normal memory and that the range is also valid. > + * For example if some garbage address is given to the kernel, then this > + * should complain. > + * > + * Returns -EFAULT if the probe failed. > + * > + * Note that this doesn't populate the backing pages. > + * > + * The kernel supports this feature if I915_PARAM_HAS_USERPTR_PROBE > + * returns a non-zero value. > + * > * I915_USERPTR_UNSYNCHRONIZED: > * > * NOT USED. Setting this flag will result in an error. > */ > __u32 flags; > #define I915_USERPTR_READ_ONLY 0x1 > +#define I915_USERPTR_PROBE 0x2 > #define I915_USERPTR_UNSYNCHRONIZED 0x80000000 > /** > * @handle: Returned handle for the object. >
Thanks! Series is: Acked-by: Kenneth Graunke <kenneth@whitecape.org> https://gitlab.freedesktop.org/kwg/mesa/-/commits/iris-userptr-probe is an untested Mesa branch that makes use of the new probe uAPI. On Thursday, July 15, 2021 3:15:35 AM PDT Matthew Auld wrote: > From: Chris Wilson <chris@chris-wilson.co.uk> > > Jason Ekstrand requested a more efficient method than userptr+set-domain > to determine if the userptr object was backed by a complete set of pages > upon creation. To be more efficient than simply populating the userptr > using get_user_pages() (as done by the call to set-domain or execbuf), > we can walk the tree of vm_area_struct and check for gaps or vma not > backed by struct page (VM_PFNMAP). The question is how to handle > VM_MIXEDMAP which may be either struct page or pfn backed... > > With discrete are going to drop support for set_domain(), so offering a > way to probe the pages, without having to resort to dummy batches has > been requested. > > v2: > - add new query param for the PROPBE flag, so userspace can easily > check if the kernel supports it(Jason). > - use mmap_read_{lock, unlock}. > - add some kernel-doc. > > Testcase: igt/gem_userptr_blits/probe > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Kenneth Graunke <kenneth@whitecape.org> > Cc: Jason Ekstrand <jason@jlekstrand.net> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Ramalingam C <ramalingam.c@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++- > drivers/gpu/drm/i915/i915_getparam.c | 3 ++ > include/uapi/drm/i915_drm.h | 18 ++++++++++ > 3 files changed, 60 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > index 56edfeff8c02..fd6880328596 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { > > #endif > > +static int > +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len) > +{ > + const unsigned long end = addr + len; > + struct vm_area_struct *vma; > + int ret = -EFAULT; > + > + mmap_read_lock(mm); > + for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) { > + if (vma->vm_start > addr) > + break; > + > + if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP)) > + break; > + > + if (vma->vm_end >= end) { > + ret = 0; > + break; > + } > + > + addr = vma->vm_end; > + } > + mmap_read_unlock(mm); > + > + return ret; > +} > + > /* > * Creates a new mm object that wraps some normal memory from the process > * context - user memory. > @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > } > > if (args->flags & ~(I915_USERPTR_READ_ONLY | > - I915_USERPTR_UNSYNCHRONIZED)) > + I915_USERPTR_UNSYNCHRONIZED | > + I915_USERPTR_PROBE)) > return -EINVAL; > > if (i915_gem_object_size_2big(args->user_size)) > @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > return -ENODEV; > } > > + if (args->flags & I915_USERPTR_PROBE) { > + /* > + * Check that the range pointed to represents real struct > + * pages and not iomappings (at this moment in time!) > + */ > + ret = probe_range(current->mm, args->user_ptr, args->user_size); > + if (ret) > + return ret; > + } > + > #ifdef CONFIG_MMU_NOTIFIER > obj = i915_gem_object_alloc(); > if (obj == NULL) > diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c > index 24e18219eb50..d6d2e1a10d14 100644 > --- a/drivers/gpu/drm/i915/i915_getparam.c > +++ b/drivers/gpu/drm/i915/i915_getparam.c > @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > case I915_PARAM_PERF_REVISION: > value = i915_perf_ioctl_version(); > break; > + case I915_PARAM_HAS_USERPTR_PROBE: > + value = true; > + break; > default: > DRM_DEBUG("Unknown parameter %d\n", param->param); > return -EINVAL; > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index e20eeeca7a1c..2e4112bf4d38 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait { > */ > #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55 > > +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */ > +#define I915_PARAM_HAS_USERPTR_PROBE 56 > + > /* Must be kept compact -- no holes and well documented */ > > typedef struct drm_i915_getparam { > @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr { > * through the GTT. If the HW can't support readonly access, an error is > * returned. > * > + * I915_USERPTR_PROBE: > + * > + * Probe the provided @user_ptr range and validate that the @user_ptr is > + * indeed pointing to normal memory and that the range is also valid. > + * For example if some garbage address is given to the kernel, then this > + * should complain. > + * > + * Returns -EFAULT if the probe failed. > + * > + * Note that this doesn't populate the backing pages. > + * > + * The kernel supports this feature if I915_PARAM_HAS_USERPTR_PROBE > + * returns a non-zero value. > + * > * I915_USERPTR_UNSYNCHRONIZED: > * > * NOT USED. Setting this flag will result in an error. > */ > __u32 flags; > #define I915_USERPTR_READ_ONLY 0x1 > +#define I915_USERPTR_PROBE 0x2 > #define I915_USERPTR_UNSYNCHRONIZED 0x80000000 > /** > * @handle: Returned handle for the object. >
On Thu, Jul 15, 2021 at 5:16 AM Matthew Auld <matthew.auld@intel.com> wrote: > > From: Chris Wilson <chris@chris-wilson.co.uk> > > Jason Ekstrand requested a more efficient method than userptr+set-domain > to determine if the userptr object was backed by a complete set of pages > upon creation. To be more efficient than simply populating the userptr > using get_user_pages() (as done by the call to set-domain or execbuf), > we can walk the tree of vm_area_struct and check for gaps or vma not > backed by struct page (VM_PFNMAP). The question is how to handle > VM_MIXEDMAP which may be either struct page or pfn backed... > > With discrete are going to drop support for set_domain(), so offering a > way to probe the pages, without having to resort to dummy batches has > been requested. > > v2: > - add new query param for the PROPBE flag, so userspace can easily > check if the kernel supports it(Jason). > - use mmap_read_{lock, unlock}. > - add some kernel-doc. > > Testcase: igt/gem_userptr_blits/probe > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Kenneth Graunke <kenneth@whitecape.org> > Cc: Jason Ekstrand <jason@jlekstrand.net> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Ramalingam C <ramalingam.c@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++- > drivers/gpu/drm/i915/i915_getparam.c | 3 ++ > include/uapi/drm/i915_drm.h | 18 ++++++++++ > 3 files changed, 60 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > index 56edfeff8c02..fd6880328596 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { > > #endif > > +static int > +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len) > +{ > + const unsigned long end = addr + len; > + struct vm_area_struct *vma; > + int ret = -EFAULT; > + > + mmap_read_lock(mm); > + for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) { > + if (vma->vm_start > addr) Why isn't this > end? Are we somehow guaranteed that one vma covers the entire range? > + break; > + > + if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP)) > + break; > + > + if (vma->vm_end >= end) { > + ret = 0; > + break; > + } > + > + addr = vma->vm_end; > + } > + mmap_read_unlock(mm); > + > + return ret; > +} > + > /* > * Creates a new mm object that wraps some normal memory from the process > * context - user memory. > @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > } > > if (args->flags & ~(I915_USERPTR_READ_ONLY | > - I915_USERPTR_UNSYNCHRONIZED)) > + I915_USERPTR_UNSYNCHRONIZED | > + I915_USERPTR_PROBE)) > return -EINVAL; > > if (i915_gem_object_size_2big(args->user_size)) > @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > return -ENODEV; > } > > + if (args->flags & I915_USERPTR_PROBE) { > + /* > + * Check that the range pointed to represents real struct > + * pages and not iomappings (at this moment in time!) > + */ > + ret = probe_range(current->mm, args->user_ptr, args->user_size); > + if (ret) > + return ret; > + } > + > #ifdef CONFIG_MMU_NOTIFIER > obj = i915_gem_object_alloc(); > if (obj == NULL) > diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c > index 24e18219eb50..d6d2e1a10d14 100644 > --- a/drivers/gpu/drm/i915/i915_getparam.c > +++ b/drivers/gpu/drm/i915/i915_getparam.c > @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > case I915_PARAM_PERF_REVISION: > value = i915_perf_ioctl_version(); > break; > + case I915_PARAM_HAS_USERPTR_PROBE: > + value = true; > + break; > default: > DRM_DEBUG("Unknown parameter %d\n", param->param); > return -EINVAL; > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index e20eeeca7a1c..2e4112bf4d38 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait { > */ > #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55 > > +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */ > +#define I915_PARAM_HAS_USERPTR_PROBE 56 > + > /* Must be kept compact -- no holes and well documented */ > > typedef struct drm_i915_getparam { > @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr { > * through the GTT. If the HW can't support readonly access, an error is > * returned. > * > + * I915_USERPTR_PROBE: > + * > + * Probe the provided @user_ptr range and validate that the @user_ptr is > + * indeed pointing to normal memory and that the range is also valid. > + * For example if some garbage address is given to the kernel, then this > + * should complain. > + * > + * Returns -EFAULT if the probe failed. > + * > + * Note that this doesn't populate the backing pages. > + * > + * The kernel supports this feature if I915_PARAM_HAS_USERPTR_PROBE > + * returns a non-zero value. > + * > * I915_USERPTR_UNSYNCHRONIZED: > * > * NOT USED. Setting this flag will result in an error. > */ > __u32 flags; > #define I915_USERPTR_READ_ONLY 0x1 > +#define I915_USERPTR_PROBE 0x2 > #define I915_USERPTR_UNSYNCHRONIZED 0x80000000 > /** > * @handle: Returned handle for the object. > -- > 2.26.3 >
On Wed, 21 Jul 2021 at 21:28, Jason Ekstrand <jason@jlekstrand.net> wrote: > > On Thu, Jul 15, 2021 at 5:16 AM Matthew Auld <matthew.auld@intel.com> wrote: > > > > From: Chris Wilson <chris@chris-wilson.co.uk> > > > > Jason Ekstrand requested a more efficient method than userptr+set-domain > > to determine if the userptr object was backed by a complete set of pages > > upon creation. To be more efficient than simply populating the userptr > > using get_user_pages() (as done by the call to set-domain or execbuf), > > we can walk the tree of vm_area_struct and check for gaps or vma not > > backed by struct page (VM_PFNMAP). The question is how to handle > > VM_MIXEDMAP which may be either struct page or pfn backed... > > > > With discrete are going to drop support for set_domain(), so offering a > > way to probe the pages, without having to resort to dummy batches has > > been requested. > > > > v2: > > - add new query param for the PROPBE flag, so userspace can easily > > check if the kernel supports it(Jason). > > - use mmap_read_{lock, unlock}. > > - add some kernel-doc. > > > > Testcase: igt/gem_userptr_blits/probe > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > Cc: Jordan Justen <jordan.l.justen@intel.com> > > Cc: Kenneth Graunke <kenneth@whitecape.org> > > Cc: Jason Ekstrand <jason@jlekstrand.net> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Ramalingam C <ramalingam.c@intel.com> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++- > > drivers/gpu/drm/i915/i915_getparam.c | 3 ++ > > include/uapi/drm/i915_drm.h | 18 ++++++++++ > > 3 files changed, 60 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > index 56edfeff8c02..fd6880328596 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { > > > > #endif > > > > +static int > > +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len) > > +{ > > + const unsigned long end = addr + len; > > + struct vm_area_struct *vma; > > + int ret = -EFAULT; > > + > > + mmap_read_lock(mm); > > + for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) { > > + if (vma->vm_start > addr) > > Why isn't this > end? Are we somehow guaranteed that one vma covers > the entire range? AFAIK we are just making sure we don't have a hole(note that we also update addr below), for example the user might have done a partial munmap. There could be multiple vma's if the kernel was unable to merge them. If we reach the vm_end >= end, then we know we have a "valid" range. > > > + break; > > + > > + if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP)) > > + break; > > + > > + if (vma->vm_end >= end) { > > + ret = 0; > > + break; > > + } > > + > > + addr = vma->vm_end; > > + } > > + mmap_read_unlock(mm); > > + > > + return ret; > > +} > > + > > /* > > * Creates a new mm object that wraps some normal memory from the process > > * context - user memory. > > @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > > } > > > > if (args->flags & ~(I915_USERPTR_READ_ONLY | > > - I915_USERPTR_UNSYNCHRONIZED)) > > + I915_USERPTR_UNSYNCHRONIZED | > > + I915_USERPTR_PROBE)) > > return -EINVAL; > > > > if (i915_gem_object_size_2big(args->user_size)) > > @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > > return -ENODEV; > > } > > > > + if (args->flags & I915_USERPTR_PROBE) { > > + /* > > + * Check that the range pointed to represents real struct > > + * pages and not iomappings (at this moment in time!) > > + */ > > + ret = probe_range(current->mm, args->user_ptr, args->user_size); > > + if (ret) > > + return ret; > > + } > > + > > #ifdef CONFIG_MMU_NOTIFIER > > obj = i915_gem_object_alloc(); > > if (obj == NULL) > > diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c > > index 24e18219eb50..d6d2e1a10d14 100644 > > --- a/drivers/gpu/drm/i915/i915_getparam.c > > +++ b/drivers/gpu/drm/i915/i915_getparam.c > > @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > > case I915_PARAM_PERF_REVISION: > > value = i915_perf_ioctl_version(); > > break; > > + case I915_PARAM_HAS_USERPTR_PROBE: > > + value = true; > > + break; > > default: > > DRM_DEBUG("Unknown parameter %d\n", param->param); > > return -EINVAL; > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > index e20eeeca7a1c..2e4112bf4d38 100644 > > --- a/include/uapi/drm/i915_drm.h > > +++ b/include/uapi/drm/i915_drm.h > > @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait { > > */ > > #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55 > > > > +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */ > > +#define I915_PARAM_HAS_USERPTR_PROBE 56 > > + > > /* Must be kept compact -- no holes and well documented */ > > > > typedef struct drm_i915_getparam { > > @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr { > > * through the GTT. If the HW can't support readonly access, an error is > > * returned. > > * > > + * I915_USERPTR_PROBE: > > + * > > + * Probe the provided @user_ptr range and validate that the @user_ptr is > > + * indeed pointing to normal memory and that the range is also valid. > > + * For example if some garbage address is given to the kernel, then this > > + * should complain. > > + * > > + * Returns -EFAULT if the probe failed. > > + * > > + * Note that this doesn't populate the backing pages. > > + * > > + * The kernel supports this feature if I915_PARAM_HAS_USERPTR_PROBE > > + * returns a non-zero value. > > + * > > * I915_USERPTR_UNSYNCHRONIZED: > > * > > * NOT USED. Setting this flag will result in an error. > > */ > > __u32 flags; > > #define I915_USERPTR_READ_ONLY 0x1 > > +#define I915_USERPTR_PROBE 0x2 > > #define I915_USERPTR_UNSYNCHRONIZED 0x80000000 > > /** > > * @handle: Returned handle for the object. > > -- > > 2.26.3 > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jul 22, 2021 at 3:44 AM Matthew Auld <matthew.william.auld@gmail.com> wrote: > > On Wed, 21 Jul 2021 at 21:28, Jason Ekstrand <jason@jlekstrand.net> wrote: > > > > On Thu, Jul 15, 2021 at 5:16 AM Matthew Auld <matthew.auld@intel.com> wrote: > > > > > > From: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > Jason Ekstrand requested a more efficient method than userptr+set-domain > > > to determine if the userptr object was backed by a complete set of pages > > > upon creation. To be more efficient than simply populating the userptr > > > using get_user_pages() (as done by the call to set-domain or execbuf), > > > we can walk the tree of vm_area_struct and check for gaps or vma not > > > backed by struct page (VM_PFNMAP). The question is how to handle > > > VM_MIXEDMAP which may be either struct page or pfn backed... > > > > > > With discrete are going to drop support for set_domain(), so offering a > > > way to probe the pages, without having to resort to dummy batches has > > > been requested. > > > > > > v2: > > > - add new query param for the PROPBE flag, so userspace can easily > > > check if the kernel supports it(Jason). > > > - use mmap_read_{lock, unlock}. > > > - add some kernel-doc. > > > > > > Testcase: igt/gem_userptr_blits/probe > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > > Cc: Jordan Justen <jordan.l.justen@intel.com> > > > Cc: Kenneth Graunke <kenneth@whitecape.org> > > > Cc: Jason Ekstrand <jason@jlekstrand.net> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Cc: Ramalingam C <ramalingam.c@intel.com> > > > --- > > > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 40 ++++++++++++++++++++- > > > drivers/gpu/drm/i915/i915_getparam.c | 3 ++ > > > include/uapi/drm/i915_drm.h | 18 ++++++++++ > > > 3 files changed, 60 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > > index 56edfeff8c02..fd6880328596 100644 > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > > @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { > > > > > > #endif > > > > > > +static int > > > +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len) > > > +{ > > > + const unsigned long end = addr + len; > > > + struct vm_area_struct *vma; > > > + int ret = -EFAULT; > > > + > > > + mmap_read_lock(mm); > > > + for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) { > > > + if (vma->vm_start > addr) > > > > Why isn't this > end? Are we somehow guaranteed that one vma covers > > the entire range? > > AFAIK we are just making sure we don't have a hole(note that we also > update addr below), for example the user might have done a partial > munmap. There could be multiple vma's if the kernel was unable to > merge them. If we reach the vm_end >= end, then we know we have a > "valid" range. Ok. That wasn't obvious to me but I see the addr update now. Makes sense. Might be worth a one-line comment for the next guy. Either way, Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> Thanks for wiring this up! --Jason > > > > > + break; > > > + > > > + if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP)) > > > + break; > > > + > > > + if (vma->vm_end >= end) { > > > + ret = 0; > > > + break; > > > + } > > > + > > > + addr = vma->vm_end; > > > + } > > > + mmap_read_unlock(mm); > > > + > > > + return ret; > > > +} > > > + > > > /* > > > * Creates a new mm object that wraps some normal memory from the process > > > * context - user memory. > > > @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > > > } > > > > > > if (args->flags & ~(I915_USERPTR_READ_ONLY | > > > - I915_USERPTR_UNSYNCHRONIZED)) > > > + I915_USERPTR_UNSYNCHRONIZED | > > > + I915_USERPTR_PROBE)) > > > return -EINVAL; > > > > > > if (i915_gem_object_size_2big(args->user_size)) > > > @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > > > return -ENODEV; > > > } > > > > > > + if (args->flags & I915_USERPTR_PROBE) { > > > + /* > > > + * Check that the range pointed to represents real struct > > > + * pages and not iomappings (at this moment in time!) > > > + */ > > > + ret = probe_range(current->mm, args->user_ptr, args->user_size); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > #ifdef CONFIG_MMU_NOTIFIER > > > obj = i915_gem_object_alloc(); > > > if (obj == NULL) > > > diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c > > > index 24e18219eb50..d6d2e1a10d14 100644 > > > --- a/drivers/gpu/drm/i915/i915_getparam.c > > > +++ b/drivers/gpu/drm/i915/i915_getparam.c > > > @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > > > case I915_PARAM_PERF_REVISION: > > > value = i915_perf_ioctl_version(); > > > break; > > > + case I915_PARAM_HAS_USERPTR_PROBE: > > > + value = true; > > > + break; > > > default: > > > DRM_DEBUG("Unknown parameter %d\n", param->param); > > > return -EINVAL; > > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > > index e20eeeca7a1c..2e4112bf4d38 100644 > > > --- a/include/uapi/drm/i915_drm.h > > > +++ b/include/uapi/drm/i915_drm.h > > > @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait { > > > */ > > > #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55 > > > > > > +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */ > > > +#define I915_PARAM_HAS_USERPTR_PROBE 56 > > > + > > > /* Must be kept compact -- no holes and well documented */ > > > > > > typedef struct drm_i915_getparam { > > > @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr { > > > * through the GTT. If the HW can't support readonly access, an error is > > > * returned. > > > * > > > + * I915_USERPTR_PROBE: > > > + * > > > + * Probe the provided @user_ptr range and validate that the @user_ptr is > > > + * indeed pointing to normal memory and that the range is also valid. > > > + * For example if some garbage address is given to the kernel, then this > > > + * should complain. > > > + * > > > + * Returns -EFAULT if the probe failed. > > > + * > > > + * Note that this doesn't populate the backing pages. > > > + * > > > + * The kernel supports this feature if I915_PARAM_HAS_USERPTR_PROBE > > > + * returns a non-zero value. > > > + * > > > * I915_USERPTR_UNSYNCHRONIZED: > > > * > > > * NOT USED. Setting this flag will result in an error. > > > */ > > > __u32 flags; > > > #define I915_USERPTR_READ_ONLY 0x1 > > > +#define I915_USERPTR_PROBE 0x2 > > > #define I915_USERPTR_UNSYNCHRONIZED 0x80000000 > > > /** > > > * @handle: Returned handle for the object. > > > -- > > > 2.26.3 > > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c index 56edfeff8c02..fd6880328596 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c @@ -422,6 +422,33 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { #endif +static int +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long len) +{ + const unsigned long end = addr + len; + struct vm_area_struct *vma; + int ret = -EFAULT; + + mmap_read_lock(mm); + for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) { + if (vma->vm_start > addr) + break; + + if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP)) + break; + + if (vma->vm_end >= end) { + ret = 0; + break; + } + + addr = vma->vm_end; + } + mmap_read_unlock(mm); + + return ret; +} + /* * Creates a new mm object that wraps some normal memory from the process * context - user memory. @@ -477,7 +504,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev, } if (args->flags & ~(I915_USERPTR_READ_ONLY | - I915_USERPTR_UNSYNCHRONIZED)) + I915_USERPTR_UNSYNCHRONIZED | + I915_USERPTR_PROBE)) return -EINVAL; if (i915_gem_object_size_2big(args->user_size)) @@ -504,6 +532,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev, return -ENODEV; } + if (args->flags & I915_USERPTR_PROBE) { + /* + * Check that the range pointed to represents real struct + * pages and not iomappings (at this moment in time!) + */ + ret = probe_range(current->mm, args->user_ptr, args->user_size); + if (ret) + return ret; + } + #ifdef CONFIG_MMU_NOTIFIER obj = i915_gem_object_alloc(); if (obj == NULL) diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c index 24e18219eb50..d6d2e1a10d14 100644 --- a/drivers/gpu/drm/i915/i915_getparam.c +++ b/drivers/gpu/drm/i915/i915_getparam.c @@ -163,6 +163,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, case I915_PARAM_PERF_REVISION: value = i915_perf_ioctl_version(); break; + case I915_PARAM_HAS_USERPTR_PROBE: + value = true; + break; default: DRM_DEBUG("Unknown parameter %d\n", param->param); return -EINVAL; diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index e20eeeca7a1c..2e4112bf4d38 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait { */ #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55 +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */ +#define I915_PARAM_HAS_USERPTR_PROBE 56 + /* Must be kept compact -- no holes and well documented */ typedef struct drm_i915_getparam { @@ -2178,12 +2181,27 @@ struct drm_i915_gem_userptr { * through the GTT. If the HW can't support readonly access, an error is * returned. * + * I915_USERPTR_PROBE: + * + * Probe the provided @user_ptr range and validate that the @user_ptr is + * indeed pointing to normal memory and that the range is also valid. + * For example if some garbage address is given to the kernel, then this + * should complain. + * + * Returns -EFAULT if the probe failed. + * + * Note that this doesn't populate the backing pages. + * + * The kernel supports this feature if I915_PARAM_HAS_USERPTR_PROBE + * returns a non-zero value. + * * I915_USERPTR_UNSYNCHRONIZED: * * NOT USED. Setting this flag will result in an error. */ __u32 flags; #define I915_USERPTR_READ_ONLY 0x1 +#define I915_USERPTR_PROBE 0x2 #define I915_USERPTR_UNSYNCHRONIZED 0x80000000 /** * @handle: Returned handle for the object.