* [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation @ 2021-07-23 11:34 Matthew Auld 2021-07-23 16:50 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork ` (3 more replies) 0 siblings, 4 replies; 16+ messages in thread From: Matthew Auld @ 2021-07-23 11:34 UTC (permalink / raw) To: intel-gfx Cc: Thomas Hellström, dri-devel, Chris Wilson, Kenneth Graunke, Daniel Vetter 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 we 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 PROBE flag, so userspace can easily check if the kernel supports it(Jason). - use mmap_read_{lock, unlock}. - add some kernel-doc. v3: - In the docs also mention that PROBE doesn't guarantee that the pages will remain valid by the time they are actually used(Tvrtko). - Add a small comment for the hole finding logic(Jason). - Move the param next to all the other params which just return true. 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> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Acked-by: Kenneth Graunke <kenneth@whitecape.org> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> --- drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++- drivers/gpu/drm/i915/i915_getparam.c | 1 + include/uapi/drm/i915_drm.h | 20 ++++++++++ 3 files changed, 61 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..468a7a617fbf 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c @@ -422,6 +422,34 @@ 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) { + /* Check for holes, note that we also update the addr below */ + 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 +505,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 +533,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..bbb7cac43eb4 100644 --- a/drivers/gpu/drm/i915/i915_getparam.c +++ b/drivers/gpu/drm/i915/i915_getparam.c @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, case I915_PARAM_HAS_EXEC_FENCE_ARRAY: case I915_PARAM_HAS_EXEC_SUBMIT_FENCE: case I915_PARAM_HAS_EXEC_TIMELINE_FENCES: + case I915_PARAM_HAS_USERPTR_PROBE: /* For the time being all of these are always true; * if some supported hardware does not have one of these * features this value needs to be provided from diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 975087553ea0..0d290535a6e5 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 { @@ -2222,12 +2225,29 @@ 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, and also doesn't + * guarantee that the object will remain valid when the object is + * eventually used. + * + * 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/userptr: Probe existence of backing struct pages upon creation 2021-07-23 11:34 [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation Matthew Auld @ 2021-07-23 16:50 ` Patchwork 2021-07-23 17:47 ` [Intel-gfx] [PATCH] " Jason Ekstrand ` (2 subsequent siblings) 3 siblings, 0 replies; 16+ messages in thread From: Patchwork @ 2021-07-23 16:50 UTC (permalink / raw) To: Matthew Auld; +Cc: intel-gfx [-- Attachment #1.1: Type: text/plain, Size: 2202 bytes --] == Series Details == Series: drm/i915/userptr: Probe existence of backing struct pages upon creation URL : https://patchwork.freedesktop.org/series/92948/ State : success == Summary == CI Bug Log - changes from CI_DRM_10379 -> Patchwork_20695 ==================================================== Summary ------- **SUCCESS** No regressions found. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/index.html Known issues ------------ Here are the changes found in Patchwork_20695 that come from known issues: ### IGT changes ### #### Issues hit #### * igt@gem_exec_suspend@basic-s0: - fi-tgl-u2: [PASS][1] -> [FAIL][2] ([i915#1888]) [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/fi-tgl-u2/igt@gem_exec_suspend@basic-s0.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/fi-tgl-u2/igt@gem_exec_suspend@basic-s0.html * igt@kms_chamelium@common-hpd-after-suspend: - fi-kbl-7500u: [PASS][3] -> [FAIL][4] ([i915#3449]) [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/fi-kbl-7500u/igt@kms_chamelium@common-hpd-after-suspend.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/fi-kbl-7500u/igt@kms_chamelium@common-hpd-after-suspend.html [i915#1888]: https://gitlab.freedesktop.org/drm/intel/issues/1888 [i915#3449]: https://gitlab.freedesktop.org/drm/intel/issues/3449 Participating hosts (43 -> 36) ------------------------------ Missing (7): fi-ilk-m540 fi-hsw-4200u fi-bsw-cyan bat-adls-4 fi-ctg-p8600 bat-adls-3 fi-bdw-samus Build changes ------------- * Linux: CI_DRM_10379 -> Patchwork_20695 CI-20190529: 20190529 CI_DRM_10379: 4f3e66252fc1d20871b7d187d719b82d7b3d102d @ git://anongit.freedesktop.org/gfx-ci/linux IGT_6149: 34ff2cf2bc352dce691593db803389fe0eb2be03 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git Patchwork_20695: 89a6ddd95d00e54dad61c31d34bab04f9e484fd1 @ git://anongit.freedesktop.org/gfx-ci/linux == Linux commits == 89a6ddd95d00 drm/i915/userptr: Probe existence of backing struct pages upon creation == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/index.html [-- Attachment #1.2: Type: text/html, Size: 2832 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation 2021-07-23 11:34 [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation Matthew Auld 2021-07-23 16:50 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork @ 2021-07-23 17:47 ` Jason Ekstrand 2021-07-23 17:49 ` Jason Ekstrand 2021-07-26 8:06 ` Matthew Auld 2021-07-24 1:38 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork 2021-07-26 8:31 ` [Intel-gfx] [PATCH] " Maarten Lankhorst 3 siblings, 2 replies; 16+ messages in thread From: Jason Ekstrand @ 2021-07-23 17:47 UTC (permalink / raw) To: Matthew Auld Cc: Thomas Hellström, Intel GFX, Maling list - DRI developers, Chris Wilson, Kenneth Graunke, Daniel Vetter https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/12044 On Fri, Jul 23, 2021 at 6:35 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 we 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 PROBE flag, so userspace can easily > check if the kernel supports it(Jason). > - use mmap_read_{lock, unlock}. > - add some kernel-doc. > v3: > - In the docs also mention that PROBE doesn't guarantee that the pages > will remain valid by the time they are actually used(Tvrtko). > - Add a small comment for the hole finding logic(Jason). > - Move the param next to all the other params which just return true. > > 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> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Acked-by: Kenneth Graunke <kenneth@whitecape.org> > Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> > --- > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++- > drivers/gpu/drm/i915/i915_getparam.c | 1 + > include/uapi/drm/i915_drm.h | 20 ++++++++++ > 3 files changed, 61 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..468a7a617fbf 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > @@ -422,6 +422,34 @@ 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) { > + /* Check for holes, note that we also update the addr below */ > + 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 +505,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 +533,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..bbb7cac43eb4 100644 > --- a/drivers/gpu/drm/i915/i915_getparam.c > +++ b/drivers/gpu/drm/i915/i915_getparam.c > @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > case I915_PARAM_HAS_EXEC_FENCE_ARRAY: > case I915_PARAM_HAS_EXEC_SUBMIT_FENCE: > case I915_PARAM_HAS_EXEC_TIMELINE_FENCES: > + case I915_PARAM_HAS_USERPTR_PROBE: > /* For the time being all of these are always true; > * if some supported hardware does not have one of these > * features this value needs to be provided from > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 975087553ea0..0d290535a6e5 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 { > @@ -2222,12 +2225,29 @@ 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, and also doesn't > + * guarantee that the object will remain valid when the object is > + * eventually used. > + * > + * 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation 2021-07-23 17:47 ` [Intel-gfx] [PATCH] " Jason Ekstrand @ 2021-07-23 17:49 ` Jason Ekstrand 2021-07-26 8:03 ` Matthew Auld 2021-07-26 8:06 ` Matthew Auld 1 sibling, 1 reply; 16+ messages in thread From: Jason Ekstrand @ 2021-07-23 17:49 UTC (permalink / raw) To: Matthew Auld Cc: Thomas Hellström, Intel GFX, Maling list - DRI developers, Chris Wilson, Kenneth Graunke, Daniel Vetter Are there IGTs for this anywhere? On Fri, Jul 23, 2021 at 12:47 PM Jason Ekstrand <jason@jlekstrand.net> wrote: > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/12044 > > On Fri, Jul 23, 2021 at 6:35 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 we 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 PROBE flag, so userspace can easily > > check if the kernel supports it(Jason). > > - use mmap_read_{lock, unlock}. > > - add some kernel-doc. > > v3: > > - In the docs also mention that PROBE doesn't guarantee that the pages > > will remain valid by the time they are actually used(Tvrtko). > > - Add a small comment for the hole finding logic(Jason). > > - Move the param next to all the other params which just return true. > > > > 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> > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Acked-by: Kenneth Graunke <kenneth@whitecape.org> > > Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++- > > drivers/gpu/drm/i915/i915_getparam.c | 1 + > > include/uapi/drm/i915_drm.h | 20 ++++++++++ > > 3 files changed, 61 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..468a7a617fbf 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > @@ -422,6 +422,34 @@ 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) { > > + /* Check for holes, note that we also update the addr below */ > > + 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 +505,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 +533,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..bbb7cac43eb4 100644 > > --- a/drivers/gpu/drm/i915/i915_getparam.c > > +++ b/drivers/gpu/drm/i915/i915_getparam.c > > @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > > case I915_PARAM_HAS_EXEC_FENCE_ARRAY: > > case I915_PARAM_HAS_EXEC_SUBMIT_FENCE: > > case I915_PARAM_HAS_EXEC_TIMELINE_FENCES: > > + case I915_PARAM_HAS_USERPTR_PROBE: > > /* For the time being all of these are always true; > > * if some supported hardware does not have one of these > > * features this value needs to be provided from > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > index 975087553ea0..0d290535a6e5 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 { > > @@ -2222,12 +2225,29 @@ 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, and also doesn't > > + * guarantee that the object will remain valid when the object is > > + * eventually used. > > + * > > + * 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation 2021-07-23 17:49 ` Jason Ekstrand @ 2021-07-26 8:03 ` Matthew Auld 0 siblings, 0 replies; 16+ messages in thread From: Matthew Auld @ 2021-07-26 8:03 UTC (permalink / raw) To: Jason Ekstrand Cc: Thomas Hellström, Daniel Vetter, Intel GFX, Maling list - DRI developers, Chris Wilson, Kenneth Graunke, Matthew Auld On Fri, 23 Jul 2021 at 18:49, Jason Ekstrand <jason@jlekstrand.net> wrote: > > Are there IGTs for this anywhere? https://patchwork.freedesktop.org/series/92580/ > > On Fri, Jul 23, 2021 at 12:47 PM Jason Ekstrand <jason@jlekstrand.net> wrote: > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/12044 > > > > On Fri, Jul 23, 2021 at 6:35 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 we 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 PROBE flag, so userspace can easily > > > check if the kernel supports it(Jason). > > > - use mmap_read_{lock, unlock}. > > > - add some kernel-doc. > > > v3: > > > - In the docs also mention that PROBE doesn't guarantee that the pages > > > will remain valid by the time they are actually used(Tvrtko). > > > - Add a small comment for the hole finding logic(Jason). > > > - Move the param next to all the other params which just return true. > > > > > > 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> > > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > Acked-by: Kenneth Graunke <kenneth@whitecape.org> > > > Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> > > > --- > > > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++- > > > drivers/gpu/drm/i915/i915_getparam.c | 1 + > > > include/uapi/drm/i915_drm.h | 20 ++++++++++ > > > 3 files changed, 61 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..468a7a617fbf 100644 > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > > @@ -422,6 +422,34 @@ 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) { > > > + /* Check for holes, note that we also update the addr below */ > > > + 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 +505,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 +533,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..bbb7cac43eb4 100644 > > > --- a/drivers/gpu/drm/i915/i915_getparam.c > > > +++ b/drivers/gpu/drm/i915/i915_getparam.c > > > @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > > > case I915_PARAM_HAS_EXEC_FENCE_ARRAY: > > > case I915_PARAM_HAS_EXEC_SUBMIT_FENCE: > > > case I915_PARAM_HAS_EXEC_TIMELINE_FENCES: > > > + case I915_PARAM_HAS_USERPTR_PROBE: > > > /* For the time being all of these are always true; > > > * if some supported hardware does not have one of these > > > * features this value needs to be provided from > > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > > index 975087553ea0..0d290535a6e5 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 { > > > @@ -2222,12 +2225,29 @@ 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, and also doesn't > > > + * guarantee that the object will remain valid when the object is > > > + * eventually used. > > > + * > > > + * 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation 2021-07-23 17:47 ` [Intel-gfx] [PATCH] " Jason Ekstrand 2021-07-23 17:49 ` Jason Ekstrand @ 2021-07-26 8:06 ` Matthew Auld 2021-07-26 15:12 ` Jason Ekstrand 1 sibling, 1 reply; 16+ messages in thread From: Matthew Auld @ 2021-07-26 8:06 UTC (permalink / raw) To: Jason Ekstrand Cc: Thomas Hellström, Daniel Vetter, Intel GFX, Maling list - DRI developers, Chris Wilson, Kenneth Graunke, Matthew Auld On Fri, 23 Jul 2021 at 18:48, Jason Ekstrand <jason@jlekstrand.net> wrote: > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/12044 Cool, is that ready to go? i.e can we start merging the kernel + IGT side. > > On Fri, Jul 23, 2021 at 6:35 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 we 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 PROBE flag, so userspace can easily > > check if the kernel supports it(Jason). > > - use mmap_read_{lock, unlock}. > > - add some kernel-doc. > > v3: > > - In the docs also mention that PROBE doesn't guarantee that the pages > > will remain valid by the time they are actually used(Tvrtko). > > - Add a small comment for the hole finding logic(Jason). > > - Move the param next to all the other params which just return true. > > > > 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> > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Acked-by: Kenneth Graunke <kenneth@whitecape.org> > > Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++- > > drivers/gpu/drm/i915/i915_getparam.c | 1 + > > include/uapi/drm/i915_drm.h | 20 ++++++++++ > > 3 files changed, 61 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..468a7a617fbf 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > @@ -422,6 +422,34 @@ 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) { > > + /* Check for holes, note that we also update the addr below */ > > + 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 +505,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 +533,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..bbb7cac43eb4 100644 > > --- a/drivers/gpu/drm/i915/i915_getparam.c > > +++ b/drivers/gpu/drm/i915/i915_getparam.c > > @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > > case I915_PARAM_HAS_EXEC_FENCE_ARRAY: > > case I915_PARAM_HAS_EXEC_SUBMIT_FENCE: > > case I915_PARAM_HAS_EXEC_TIMELINE_FENCES: > > + case I915_PARAM_HAS_USERPTR_PROBE: > > /* For the time being all of these are always true; > > * if some supported hardware does not have one of these > > * features this value needs to be provided from > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > index 975087553ea0..0d290535a6e5 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 { > > @@ -2222,12 +2225,29 @@ 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, and also doesn't > > + * guarantee that the object will remain valid when the object is > > + * eventually used. > > + * > > + * 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation 2021-07-26 8:06 ` Matthew Auld @ 2021-07-26 15:12 ` Jason Ekstrand 0 siblings, 0 replies; 16+ messages in thread From: Jason Ekstrand @ 2021-07-26 15:12 UTC (permalink / raw) To: Matthew Auld Cc: Thomas Hellström, Daniel Vetter, Intel GFX, Maling list - DRI developers, Chris Wilson, Kenneth Graunke, Matthew Auld On Mon, Jul 26, 2021 at 3:06 AM Matthew Auld <matthew.william.auld@gmail.com> wrote: > > On Fri, 23 Jul 2021 at 18:48, Jason Ekstrand <jason@jlekstrand.net> wrote: > > > > https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/12044 > > Cool, is that ready to go? i.e can we start merging the kernel + IGT side. Yes, it's all reviewed. Though, it sounds like Maarten had a comment so we should settle on that before landing. > > > > On Fri, Jul 23, 2021 at 6:35 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 we 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 PROBE flag, so userspace can easily > > > check if the kernel supports it(Jason). > > > - use mmap_read_{lock, unlock}. > > > - add some kernel-doc. > > > v3: > > > - In the docs also mention that PROBE doesn't guarantee that the pages > > > will remain valid by the time they are actually used(Tvrtko). > > > - Add a small comment for the hole finding logic(Jason). > > > - Move the param next to all the other params which just return true. > > > > > > 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> > > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > Acked-by: Kenneth Graunke <kenneth@whitecape.org> > > > Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> > > > --- > > > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++- > > > drivers/gpu/drm/i915/i915_getparam.c | 1 + > > > include/uapi/drm/i915_drm.h | 20 ++++++++++ > > > 3 files changed, 61 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..468a7a617fbf 100644 > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > > @@ -422,6 +422,34 @@ 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) { > > > + /* Check for holes, note that we also update the addr below */ > > > + 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 +505,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 +533,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..bbb7cac43eb4 100644 > > > --- a/drivers/gpu/drm/i915/i915_getparam.c > > > +++ b/drivers/gpu/drm/i915/i915_getparam.c > > > @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > > > case I915_PARAM_HAS_EXEC_FENCE_ARRAY: > > > case I915_PARAM_HAS_EXEC_SUBMIT_FENCE: > > > case I915_PARAM_HAS_EXEC_TIMELINE_FENCES: > > > + case I915_PARAM_HAS_USERPTR_PROBE: > > > /* For the time being all of these are always true; > > > * if some supported hardware does not have one of these > > > * features this value needs to be provided from > > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > > index 975087553ea0..0d290535a6e5 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 { > > > @@ -2222,12 +2225,29 @@ 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, and also doesn't > > > + * guarantee that the object will remain valid when the object is > > > + * eventually used. > > > + * > > > + * 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 16+ messages in thread
* [Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915/userptr: Probe existence of backing struct pages upon creation 2021-07-23 11:34 [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation Matthew Auld 2021-07-23 16:50 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork 2021-07-23 17:47 ` [Intel-gfx] [PATCH] " Jason Ekstrand @ 2021-07-24 1:38 ` Patchwork 2021-07-26 8:31 ` [Intel-gfx] [PATCH] " Maarten Lankhorst 3 siblings, 0 replies; 16+ messages in thread From: Patchwork @ 2021-07-24 1:38 UTC (permalink / raw) To: Matthew Auld; +Cc: intel-gfx [-- Attachment #1.1: Type: text/plain, Size: 30294 bytes --] == Series Details == Series: drm/i915/userptr: Probe existence of backing struct pages upon creation URL : https://patchwork.freedesktop.org/series/92948/ State : success == Summary == CI Bug Log - changes from CI_DRM_10379_full -> Patchwork_20695_full ==================================================== Summary ------- **SUCCESS** No regressions found. Known issues ------------ Here are the changes found in Patchwork_20695_full that come from known issues: ### IGT changes ### #### Issues hit #### * igt@feature_discovery@chamelium: - shard-iclb: NOTRUN -> [SKIP][1] ([fdo#111827]) [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@feature_discovery@chamelium.html * igt@gem_create@create-massive: - shard-apl: NOTRUN -> [DMESG-WARN][2] ([i915#3002]) [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl6/igt@gem_create@create-massive.html * igt@gem_ctx_persistence@engines-mixed: - shard-snb: NOTRUN -> [SKIP][3] ([fdo#109271] / [i915#1099]) +2 similar issues [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-snb5/igt@gem_ctx_persistence@engines-mixed.html * igt@gem_eio@unwedge-stress: - shard-tglb: [PASS][4] -> [TIMEOUT][5] ([i915#2369] / [i915#3063] / [i915#3648]) [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-tglb6/igt@gem_eio@unwedge-stress.html [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb1/igt@gem_eio@unwedge-stress.html * igt@gem_exec_fair@basic-none-share@rcs0: - shard-tglb: [PASS][6] -> [FAIL][7] ([i915#2842]) [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-tglb5/igt@gem_exec_fair@basic-none-share@rcs0.html [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb7/igt@gem_exec_fair@basic-none-share@rcs0.html * igt@gem_exec_fair@basic-pace-solo@rcs0: - shard-iclb: NOTRUN -> [FAIL][8] ([i915#2842]) +1 similar issue [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@gem_exec_fair@basic-pace-solo@rcs0.html - shard-glk: NOTRUN -> [FAIL][9] ([i915#2842]) [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk5/igt@gem_exec_fair@basic-pace-solo@rcs0.html * igt@gem_exec_fair@basic-pace@rcs0: - shard-tglb: [PASS][10] -> [FAIL][11] ([i915#2876]) [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-tglb3/igt@gem_exec_fair@basic-pace@rcs0.html [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb3/igt@gem_exec_fair@basic-pace@rcs0.html * igt@gem_exec_fair@basic-pace@vecs0: - shard-kbl: [PASS][12] -> [SKIP][13] ([fdo#109271]) [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl2/igt@gem_exec_fair@basic-pace@vecs0.html [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl1/igt@gem_exec_fair@basic-pace@vecs0.html * igt@gem_exec_flush@basic-batch-kernel-default-cmd: - shard-snb: NOTRUN -> [SKIP][14] ([fdo#109271]) +286 similar issues [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-snb5/igt@gem_exec_flush@basic-batch-kernel-default-cmd.html * igt@gem_huc_copy@huc-copy: - shard-glk: NOTRUN -> [SKIP][15] ([fdo#109271] / [i915#2190]) [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk5/igt@gem_huc_copy@huc-copy.html - shard-iclb: NOTRUN -> [SKIP][16] ([i915#2190]) [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@gem_huc_copy@huc-copy.html * igt@gem_mmap_gtt@cpuset-big-copy: - shard-glk: [PASS][17] -> [FAIL][18] ([i915#1888] / [i915#307]) [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-glk1/igt@gem_mmap_gtt@cpuset-big-copy.html [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk9/igt@gem_mmap_gtt@cpuset-big-copy.html * igt@gem_pwrite@basic-exhaustion: - shard-kbl: NOTRUN -> [WARN][19] ([i915#2658]) [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl6/igt@gem_pwrite@basic-exhaustion.html * igt@gem_render_copy@y-tiled-ccs-to-yf-tiled-mc-ccs: - shard-iclb: NOTRUN -> [SKIP][20] ([i915#768]) [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@gem_render_copy@y-tiled-ccs-to-yf-tiled-mc-ccs.html * igt@gen7_exec_parse@basic-allowed: - shard-skl: NOTRUN -> [SKIP][21] ([fdo#109271]) +34 similar issues [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl4/igt@gen7_exec_parse@basic-allowed.html * igt@i915_pm_rpm@basic-rte: - shard-iclb: NOTRUN -> [FAIL][22] ([i915#579]) [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@i915_pm_rpm@basic-rte.html * igt@i915_pm_rpm@drm-resources-equal: - shard-tglb: NOTRUN -> [SKIP][23] ([i915#579]) [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb2/igt@i915_pm_rpm@drm-resources-equal.html * igt@i915_pm_rpm@gem-mmap-type: - shard-iclb: NOTRUN -> [SKIP][24] ([i915#579]) [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@i915_pm_rpm@gem-mmap-type.html * igt@i915_pm_rpm@modeset-lpsp-stress: - shard-apl: NOTRUN -> [SKIP][25] ([fdo#109271]) +144 similar issues [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl3/igt@i915_pm_rpm@modeset-lpsp-stress.html * igt@i915_selftest@live@gt_heartbeat: - shard-tglb: [PASS][26] -> [DMESG-FAIL][27] ([i915#3768]) [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-tglb6/igt@i915_selftest@live@gt_heartbeat.html [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb2/igt@i915_selftest@live@gt_heartbeat.html * igt@i915_suspend@forcewake: - shard-kbl: [PASS][28] -> [DMESG-WARN][29] ([i915#180]) +2 similar issues [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl1/igt@i915_suspend@forcewake.html [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl7/igt@i915_suspend@forcewake.html * igt@kms_big_fb@x-tiled-64bpp-rotate-90: - shard-iclb: NOTRUN -> [SKIP][30] ([fdo#110725] / [fdo#111614]) [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_big_fb@x-tiled-64bpp-rotate-90.html * igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-0-hflip: - shard-kbl: NOTRUN -> [SKIP][31] ([fdo#109271] / [i915#3777]) [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl2/igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-0-hflip.html * igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-0-hflip: - shard-apl: NOTRUN -> [SKIP][32] ([fdo#109271] / [i915#3777]) [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl1/igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-0-hflip.html * igt@kms_big_fb@yf-tiled-64bpp-rotate-0: - shard-iclb: NOTRUN -> [SKIP][33] ([fdo#110723]) [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_big_fb@yf-tiled-64bpp-rotate-0.html * igt@kms_big_fb@yf-tiled-64bpp-rotate-180: - shard-tglb: NOTRUN -> [SKIP][34] ([fdo#111615]) [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb2/igt@kms_big_fb@yf-tiled-64bpp-rotate-180.html * igt@kms_ccs@pipe-c-bad-pixel-format-y_tiled_ccs: - shard-tglb: NOTRUN -> [SKIP][35] ([i915#3689]) [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb7/igt@kms_ccs@pipe-c-bad-pixel-format-y_tiled_ccs.html * igt@kms_chamelium@dp-mode-timings: - shard-apl: NOTRUN -> [SKIP][36] ([fdo#109271] / [fdo#111827]) +15 similar issues [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl3/igt@kms_chamelium@dp-mode-timings.html * igt@kms_chamelium@hdmi-hpd-with-enabled-mode: - shard-snb: NOTRUN -> [SKIP][37] ([fdo#109271] / [fdo#111827]) +6 similar issues [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-snb5/igt@kms_chamelium@hdmi-hpd-with-enabled-mode.html * igt@kms_chamelium@vga-frame-dump: - shard-glk: NOTRUN -> [SKIP][38] ([fdo#109271] / [fdo#111827]) +3 similar issues [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk5/igt@kms_chamelium@vga-frame-dump.html * igt@kms_color_chamelium@pipe-c-ctm-limited-range: - shard-iclb: NOTRUN -> [SKIP][39] ([fdo#109284] / [fdo#111827]) +3 similar issues [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_color_chamelium@pipe-c-ctm-limited-range.html * igt@kms_color_chamelium@pipe-c-degamma: - shard-skl: NOTRUN -> [SKIP][40] ([fdo#109271] / [fdo#111827]) +3 similar issues [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl4/igt@kms_color_chamelium@pipe-c-degamma.html * igt@kms_content_protection@atomic: - shard-iclb: NOTRUN -> [SKIP][41] ([fdo#109300] / [fdo#111066]) [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_content_protection@atomic.html * igt@kms_content_protection@atomic-dpms: - shard-apl: NOTRUN -> [TIMEOUT][42] ([i915#1319]) [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl1/igt@kms_content_protection@atomic-dpms.html * igt@kms_cursor_crc@pipe-a-cursor-32x32-random: - shard-iclb: NOTRUN -> [SKIP][43] ([fdo#109278]) +15 similar issues [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_cursor_crc@pipe-a-cursor-32x32-random.html * igt@kms_cursor_crc@pipe-a-cursor-512x170-random: - shard-iclb: NOTRUN -> [SKIP][44] ([fdo#109278] / [fdo#109279]) [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_cursor_crc@pipe-a-cursor-512x170-random.html * igt@kms_cursor_legacy@cursorb-vs-flipa-legacy: - shard-iclb: NOTRUN -> [SKIP][45] ([fdo#109274] / [fdo#109278]) [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_cursor_legacy@cursorb-vs-flipa-legacy.html * igt@kms_flip@flip-vs-expired-vblank@c-edp1: - shard-skl: [PASS][46] -> [FAIL][47] ([i915#79]) [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-skl10/igt@kms_flip@flip-vs-expired-vblank@c-edp1.html [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl10/igt@kms_flip@flip-vs-expired-vblank@c-edp1.html * igt@kms_flip@flip-vs-suspend@b-edp1: - shard-skl: [PASS][48] -> [INCOMPLETE][49] ([i915#146] / [i915#198]) [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-skl8/igt@kms_flip@flip-vs-suspend@b-edp1.html [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl1/igt@kms_flip@flip-vs-suspend@b-edp1.html * igt@kms_flip@flip-vs-suspend@c-dp1: - shard-kbl: NOTRUN -> [DMESG-WARN][50] ([i915#180]) [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl1/igt@kms_flip@flip-vs-suspend@c-dp1.html * igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-32bpp-ytileccs: - shard-skl: NOTRUN -> [INCOMPLETE][51] ([i915#3699]) [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl4/igt@kms_flip_scaled_crc@flip-32bpp-ytile-to-32bpp-ytileccs.html * igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-pri-shrfb-draw-blt: - shard-iclb: NOTRUN -> [SKIP][52] ([fdo#109280]) +8 similar issues [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-pri-shrfb-draw-blt.html * igt@kms_frontbuffer_tracking@psr-2p-scndscrn-spr-indfb-draw-render: - shard-glk: NOTRUN -> [SKIP][53] ([fdo#109271]) +29 similar issues [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk5/igt@kms_frontbuffer_tracking@psr-2p-scndscrn-spr-indfb-draw-render.html * igt@kms_frontbuffer_tracking@psr-rgb101010-draw-mmap-wc: - shard-kbl: NOTRUN -> [SKIP][54] ([fdo#109271]) +54 similar issues [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl2/igt@kms_frontbuffer_tracking@psr-rgb101010-draw-mmap-wc.html * igt@kms_pipe_crc_basic@disable-crc-after-crtc-pipe-d: - shard-apl: NOTRUN -> [SKIP][55] ([fdo#109271] / [i915#533]) +1 similar issue [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl1/igt@kms_pipe_crc_basic@disable-crc-after-crtc-pipe-d.html * igt@kms_pipe_crc_basic@hang-read-crc-pipe-d: - shard-kbl: NOTRUN -> [SKIP][56] ([fdo#109271] / [i915#533]) [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl6/igt@kms_pipe_crc_basic@hang-read-crc-pipe-d.html * igt@kms_plane_alpha_blend@pipe-a-alpha-7efc: - shard-apl: NOTRUN -> [FAIL][57] ([fdo#108145] / [i915#265]) [57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl3/igt@kms_plane_alpha_blend@pipe-a-alpha-7efc.html * igt@kms_plane_alpha_blend@pipe-b-alpha-transparent-fb: - shard-apl: NOTRUN -> [FAIL][58] ([i915#265]) +1 similar issue [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl6/igt@kms_plane_alpha_blend@pipe-b-alpha-transparent-fb.html * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max: - shard-kbl: NOTRUN -> [FAIL][59] ([fdo#108145] / [i915#265]) [59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl6/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max.html * igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min: - shard-skl: NOTRUN -> [FAIL][60] ([fdo#108145] / [i915#265]) [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl10/igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min.html * igt@kms_plane_lowres@pipe-b-tiling-none: - shard-iclb: NOTRUN -> [SKIP][61] ([i915#3536]) +1 similar issue [61]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_plane_lowres@pipe-b-tiling-none.html * igt@kms_psr2_sf@overlay-primary-update-sf-dmg-area-2: - shard-iclb: NOTRUN -> [SKIP][62] ([i915#658]) [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_psr2_sf@overlay-primary-update-sf-dmg-area-2.html - shard-glk: NOTRUN -> [SKIP][63] ([fdo#109271] / [i915#658]) [63]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk5/igt@kms_psr2_sf@overlay-primary-update-sf-dmg-area-2.html * igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-5: - shard-kbl: NOTRUN -> [SKIP][64] ([fdo#109271] / [i915#658]) +1 similar issue [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl6/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-5.html * igt@kms_psr2_su@page_flip: - shard-apl: NOTRUN -> [SKIP][65] ([fdo#109271] / [i915#658]) +2 similar issues [65]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl1/igt@kms_psr2_su@page_flip.html * igt@kms_psr@psr2_basic: - shard-iclb: [PASS][66] -> [SKIP][67] ([fdo#109441]) +2 similar issues [66]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb2/igt@kms_psr@psr2_basic.html [67]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb5/igt@kms_psr@psr2_basic.html * igt@kms_psr@psr2_cursor_mmap_cpu: - shard-iclb: NOTRUN -> [SKIP][68] ([fdo#109441]) [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_psr@psr2_cursor_mmap_cpu.html * igt@kms_setmode@basic: - shard-snb: NOTRUN -> [FAIL][69] ([i915#31]) [69]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-snb5/igt@kms_setmode@basic.html * igt@kms_vrr@flip-suspend: - shard-iclb: NOTRUN -> [SKIP][70] ([fdo#109502]) [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_vrr@flip-suspend.html * igt@kms_writeback@writeback-invalid-parameters: - shard-apl: NOTRUN -> [SKIP][71] ([fdo#109271] / [i915#2437]) [71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl3/igt@kms_writeback@writeback-invalid-parameters.html * igt@perf@polling-parameterized: - shard-tglb: [PASS][72] -> [FAIL][73] ([i915#1542]) [72]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-tglb3/igt@perf@polling-parameterized.html [73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb2/igt@perf@polling-parameterized.html * igt@perf@unprivileged-single-ctx-counters: - shard-iclb: NOTRUN -> [SKIP][74] ([fdo#109289]) +1 similar issue [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@perf@unprivileged-single-ctx-counters.html * igt@prime_nv_pcopy@test3_3: - shard-iclb: NOTRUN -> [SKIP][75] ([fdo#109291]) [75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@prime_nv_pcopy@test3_3.html * igt@sysfs_clients@sema-10: - shard-kbl: NOTRUN -> [SKIP][76] ([fdo#109271] / [i915#2994]) [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl6/igt@sysfs_clients@sema-10.html * igt@sysfs_clients@sema-25: - shard-apl: NOTRUN -> [SKIP][77] ([fdo#109271] / [i915#2994]) [77]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl1/igt@sysfs_clients@sema-25.html #### Possible fixes #### * igt@gem_ctx_isolation@preservation-s3@rcs0: - shard-skl: [INCOMPLETE][78] ([i915#198]) -> [PASS][79] +1 similar issue [78]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-skl10/igt@gem_ctx_isolation@preservation-s3@rcs0.html [79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl4/igt@gem_ctx_isolation@preservation-s3@rcs0.html * igt@gem_eio@hibernate: - {shard-rkl}: [INCOMPLETE][80] ([i915#3811] / [i915#3833]) -> [PASS][81] [80]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-rkl-1/igt@gem_eio@hibernate.html [81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-rkl-1/igt@gem_eio@hibernate.html * igt@gem_eio@in-flight-contexts-10ms: - shard-tglb: [TIMEOUT][82] ([i915#3063]) -> [PASS][83] [82]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-tglb6/igt@gem_eio@in-flight-contexts-10ms.html [83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb6/igt@gem_eio@in-flight-contexts-10ms.html * igt@gem_eio@unwedge-stress: - {shard-rkl}: [TIMEOUT][84] ([i915#3063]) -> [PASS][85] [84]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-rkl-5/igt@gem_eio@unwedge-stress.html [85]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-rkl-5/igt@gem_eio@unwedge-stress.html * igt@gem_exec_fair@basic-deadline: - shard-glk: [FAIL][86] ([i915#2846]) -> [PASS][87] [86]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-glk4/igt@gem_exec_fair@basic-deadline.html [87]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk1/igt@gem_exec_fair@basic-deadline.html * igt@gem_exec_fair@basic-none@vcs0: - shard-kbl: [FAIL][88] ([i915#2842]) -> [PASS][89] [88]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl3/igt@gem_exec_fair@basic-none@vcs0.html [89]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl6/igt@gem_exec_fair@basic-none@vcs0.html * igt@gem_exec_fair@basic-pace@rcs0: - {shard-rkl}: [FAIL][90] ([i915#2842]) -> [PASS][91] +2 similar issues [90]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-rkl-5/igt@gem_exec_fair@basic-pace@rcs0.html [91]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-rkl-5/igt@gem_exec_fair@basic-pace@rcs0.html * igt@gem_exec_fair@basic-throttle@rcs0: - shard-iclb: [FAIL][92] ([i915#2849]) -> [PASS][93] [92]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb2/igt@gem_exec_fair@basic-throttle@rcs0.html [93]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb5/igt@gem_exec_fair@basic-throttle@rcs0.html * igt@gem_exec_whisper@basic-contexts-forked: - shard-iclb: [INCOMPLETE][94] ([i915#1895]) -> [PASS][95] [94]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb8/igt@gem_exec_whisper@basic-contexts-forked.html [95]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@gem_exec_whisper@basic-contexts-forked.html * igt@gem_mmap_gtt@cpuset-medium-copy: - shard-glk: [FAIL][96] ([i915#1888] / [i915#307]) -> [PASS][97] [96]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-glk2/igt@gem_mmap_gtt@cpuset-medium-copy.html [97]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk2/igt@gem_mmap_gtt@cpuset-medium-copy.html * igt@i915_pm_rc6_residency@rc6-fence: - shard-tglb: [WARN][98] ([i915#2681]) -> [PASS][99] [98]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-tglb7/igt@i915_pm_rc6_residency@rc6-fence.html [99]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-tglb5/igt@i915_pm_rc6_residency@rc6-fence.html * igt@i915_selftest@live@hangcheck: - shard-snb: [INCOMPLETE][100] ([i915#2782]) -> [PASS][101] [100]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-snb6/igt@i915_selftest@live@hangcheck.html [101]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-snb7/igt@i915_selftest@live@hangcheck.html * igt@kms_big_fb@linear-64bpp-rotate-180: - shard-iclb: [DMESG-WARN][102] ([i915#3621]) -> [PASS][103] [102]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb1/igt@kms_big_fb@linear-64bpp-rotate-180.html [103]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb7/igt@kms_big_fb@linear-64bpp-rotate-180.html * igt@kms_cursor_crc@pipe-c-cursor-suspend: - shard-kbl: [DMESG-WARN][104] ([i915#180]) -> [PASS][105] +1 similar issue [104]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl7/igt@kms_cursor_crc@pipe-c-cursor-suspend.html [105]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl2/igt@kms_cursor_crc@pipe-c-cursor-suspend.html * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size: - shard-skl: [FAIL][106] ([i915#2346] / [i915#533]) -> [PASS][107] [106]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-skl1/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html [107]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl10/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html * igt@kms_fbcon_fbt@fbc-suspend: - shard-kbl: [INCOMPLETE][108] ([i915#155] / [i915#180] / [i915#636]) -> [PASS][109] [108]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl7/igt@kms_fbcon_fbt@fbc-suspend.html [109]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl6/igt@kms_fbcon_fbt@fbc-suspend.html * igt@kms_flip@flip-vs-expired-vblank@a-dp1: - shard-apl: [FAIL][110] ([i915#79]) -> [PASS][111] [110]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-apl1/igt@kms_flip@flip-vs-expired-vblank@a-dp1.html [111]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl1/igt@kms_flip@flip-vs-expired-vblank@a-dp1.html * igt@kms_flip@flip-vs-expired-vblank@a-edp1: - shard-skl: [FAIL][112] ([i915#79]) -> [PASS][113] [112]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-skl10/igt@kms_flip@flip-vs-expired-vblank@a-edp1.html [113]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl10/igt@kms_flip@flip-vs-expired-vblank@a-edp1.html * igt@kms_flip@flip-vs-suspend@a-dp1: - shard-kbl: [INCOMPLETE][114] ([i915#155]) -> [PASS][115] [114]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl4/igt@kms_flip@flip-vs-suspend@a-dp1.html [115]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl1/igt@kms_flip@flip-vs-suspend@a-dp1.html * igt@kms_flip@plain-flip-ts-check-interruptible@b-edp1: - shard-skl: [FAIL][116] ([i915#2122]) -> [PASS][117] [116]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-skl4/igt@kms_flip@plain-flip-ts-check-interruptible@b-edp1.html [117]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl5/igt@kms_flip@plain-flip-ts-check-interruptible@b-edp1.html * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc: - shard-skl: [FAIL][118] ([fdo#108145] / [i915#265]) -> [PASS][119] [118]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-skl9/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html [119]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-skl2/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html * igt@kms_plane_alpha_blend@pipe-c-coverage-vs-premult-vs-constant: - shard-iclb: [SKIP][120] ([fdo#109278]) -> [PASS][121] [120]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb2/igt@kms_plane_alpha_blend@pipe-c-coverage-vs-premult-vs-constant.html [121]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb5/igt@kms_plane_alpha_blend@pipe-c-coverage-vs-premult-vs-constant.html * igt@kms_psr@psr2_cursor_render: - shard-iclb: [SKIP][122] ([fdo#109441]) -> [PASS][123] [122]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb3/igt@kms_psr@psr2_cursor_render.html [123]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb2/igt@kms_psr@psr2_cursor_render.html * igt@perf@blocking: - {shard-rkl}: [FAIL][124] ([i915#1542]) -> [PASS][125] [124]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-rkl-6/igt@perf@blocking.html [125]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-rkl-6/igt@perf@blocking.html * igt@perf@polling-parameterized: - shard-apl: [FAIL][126] ([i915#1542]) -> [PASS][127] [126]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-apl3/igt@perf@polling-parameterized.html [127]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-apl7/igt@perf@polling-parameterized.html * igt@prime_vgem@coherency-blt: - shard-glk: [INCOMPLETE][128] ([i915#2944]) -> [PASS][129] [128]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-glk5/igt@prime_vgem@coherency-blt.html [129]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-glk5/igt@prime_vgem@coherency-blt.html #### Warnings #### * igt@i915_pm_rc6_residency@rc6-fence: - shard-iclb: [WARN][130] ([i915#1804] / [i915#2684]) -> [WARN][131] ([i915#2684]) [130]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb3/igt@i915_pm_rc6_residency@rc6-fence.html [131]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb8/igt@i915_pm_rc6_residency@rc6-fence.html * igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-2: - shard-iclb: [SKIP][132] ([i915#658]) -> [SKIP][133] ([i915#2920]) +1 similar issue [132]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb3/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-2.html [133]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb2/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-2.html * igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-4: - shard-iclb: [SKIP][134] ([i915#2920]) -> [SKIP][135] ([i915#658]) [134]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-iclb2/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-4.html [135]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-iclb5/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-4.html * igt@runner@aborted: - shard-kbl: ([FAIL][136], [FAIL][137], [FAIL][138], [FAIL][139], [FAIL][140], [FAIL][141], [FAIL][142], [FAIL][143]) ([i915#1436] / [i915#180] / [i915#1814] / [i915#2505] / [i915#3002] / [i915#3363] / [i915#602] / [i915#92]) -> ([FAIL][144], [FAIL][145], [FAIL][146], [FAIL][147], [FAIL][148], [FAIL][149], [FAIL][150]) ([i915#180] / [i915#1814] / [i915#3002] / [i915#3363]) [136]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl7/igt@runner@aborted.html [137]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl4/igt@runner@aborted.html [138]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl6/igt@runner@aborted.html [139]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl7/igt@runner@aborted.html [140]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl6/igt@runner@aborted.html [141]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl7/igt@runner@aborted.html [142]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl4/igt@runner@aborted.html [143]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10379/shard-kbl7/igt@runner@aborted.html [144]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl7/igt@runner@aborted.html [145]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl7/igt@runner@aborted.html [146]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl7/igt@runner@aborted.html [147]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl7/igt@runner@aborted.html [148]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl2/igt@runner@aborted.html [149]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl2/igt@runner@aborted.html [150]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/shard-kbl1/igt@runner@aborted.html - shard-iclb: ([FAIL][151], [FAIL][152], [FAIL][153]) ([i915#1814] / [i915#3 == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20695/index.html [-- Attachment #1.2: Type: text/html, Size: 34117 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation 2021-07-23 11:34 [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation Matthew Auld ` (2 preceding siblings ...) 2021-07-24 1:38 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork @ 2021-07-26 8:31 ` Maarten Lankhorst 2021-07-26 15:14 ` Jason Ekstrand 3 siblings, 1 reply; 16+ messages in thread From: Maarten Lankhorst @ 2021-07-26 8:31 UTC (permalink / raw) To: Matthew Auld, intel-gfx Cc: Thomas Hellström, dri-devel, Chris Wilson, Kenneth Graunke, Daniel Vetter Op 23-07-2021 om 13:34 schreef Matthew Auld: > 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 we 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 PROBE flag, so userspace can easily > check if the kernel supports it(Jason). > - use mmap_read_{lock, unlock}. > - add some kernel-doc. > v3: > - In the docs also mention that PROBE doesn't guarantee that the pages > will remain valid by the time they are actually used(Tvrtko). > - Add a small comment for the hole finding logic(Jason). > - Move the param next to all the other params which just return true. > > 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> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Acked-by: Kenneth Graunke <kenneth@whitecape.org> > Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> > --- > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++- > drivers/gpu/drm/i915/i915_getparam.c | 1 + > include/uapi/drm/i915_drm.h | 20 ++++++++++ > 3 files changed, 61 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..468a7a617fbf 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > @@ -422,6 +422,34 @@ 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) { > + /* Check for holes, note that we also update the addr below */ > + 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 +505,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 +533,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..bbb7cac43eb4 100644 > --- a/drivers/gpu/drm/i915/i915_getparam.c > +++ b/drivers/gpu/drm/i915/i915_getparam.c > @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > case I915_PARAM_HAS_EXEC_FENCE_ARRAY: > case I915_PARAM_HAS_EXEC_SUBMIT_FENCE: > case I915_PARAM_HAS_EXEC_TIMELINE_FENCES: > + case I915_PARAM_HAS_USERPTR_PROBE: > /* For the time being all of these are always true; > * if some supported hardware does not have one of these > * features this value needs to be provided from > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 975087553ea0..0d290535a6e5 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 { > @@ -2222,12 +2225,29 @@ 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, and also doesn't > + * guarantee that the object will remain valid when the object is > + * eventually used. > + * > + * 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. Could we use _VALIDATE instead of probe? Or at least pin the pages as well, so we don't have to do it later? We already have i915_gem_object_userptr_validate, no need to dupe it. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation 2021-07-26 8:31 ` [Intel-gfx] [PATCH] " Maarten Lankhorst @ 2021-07-26 15:14 ` Jason Ekstrand 2021-07-26 16:10 ` Tvrtko Ursulin 0 siblings, 1 reply; 16+ messages in thread From: Jason Ekstrand @ 2021-07-26 15:14 UTC (permalink / raw) To: Maarten Lankhorst Cc: Thomas Hellström, Intel GFX, Maling list - DRI developers, Chris Wilson, Kenneth Graunke, Matthew Auld, Daniel Vetter On Mon, Jul 26, 2021 at 3:31 AM Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > > Op 23-07-2021 om 13:34 schreef Matthew Auld: > > 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 we 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 PROBE flag, so userspace can easily > > check if the kernel supports it(Jason). > > - use mmap_read_{lock, unlock}. > > - add some kernel-doc. > > v3: > > - In the docs also mention that PROBE doesn't guarantee that the pages > > will remain valid by the time they are actually used(Tvrtko). > > - Add a small comment for the hole finding logic(Jason). > > - Move the param next to all the other params which just return true. > > > > 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> > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Acked-by: Kenneth Graunke <kenneth@whitecape.org> > > Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++- > > drivers/gpu/drm/i915/i915_getparam.c | 1 + > > include/uapi/drm/i915_drm.h | 20 ++++++++++ > > 3 files changed, 61 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..468a7a617fbf 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > @@ -422,6 +422,34 @@ 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) { > > + /* Check for holes, note that we also update the addr below */ > > + 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 +505,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 +533,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..bbb7cac43eb4 100644 > > --- a/drivers/gpu/drm/i915/i915_getparam.c > > +++ b/drivers/gpu/drm/i915/i915_getparam.c > > @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > > case I915_PARAM_HAS_EXEC_FENCE_ARRAY: > > case I915_PARAM_HAS_EXEC_SUBMIT_FENCE: > > case I915_PARAM_HAS_EXEC_TIMELINE_FENCES: > > + case I915_PARAM_HAS_USERPTR_PROBE: > > /* For the time being all of these are always true; > > * if some supported hardware does not have one of these > > * features this value needs to be provided from > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > index 975087553ea0..0d290535a6e5 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 { > > @@ -2222,12 +2225,29 @@ 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, and also doesn't > > + * guarantee that the object will remain valid when the object is > > + * eventually used. > > + * > > + * 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. > > Could we use _VALIDATE instead of probe? Or at least pin the pages as well, so we don't have to do it later? I only care that the name matches what it does. _VALIDATE sounds like it does a full validation of everything such that, if the import succeeds, execbuf will as well. If we pin the pages at the same time, maybe that's true? _PROBE, on the other hand, sounds a lot more like a one-time best-effort check which may race with other stuff and doesn't guarantee future success. That's in line with what the current patch does. > We already have i915_gem_object_userptr_validate, no need to dupe it. I have no opinion on this. --Jason _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation 2021-07-26 15:14 ` Jason Ekstrand @ 2021-07-26 16:10 ` Tvrtko Ursulin 2021-07-28 14:22 ` Matthew Auld 0 siblings, 1 reply; 16+ messages in thread From: Tvrtko Ursulin @ 2021-07-26 16:10 UTC (permalink / raw) To: Jason Ekstrand, Maarten Lankhorst Cc: Thomas Hellström, Intel GFX, Maling list - DRI developers, Chris Wilson, Kenneth Graunke, Matthew Auld, Daniel Vetter On 26/07/2021 16:14, Jason Ekstrand wrote: > On Mon, Jul 26, 2021 at 3:31 AM Maarten Lankhorst > <maarten.lankhorst@linux.intel.com> wrote: >> >> Op 23-07-2021 om 13:34 schreef Matthew Auld: >>> 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 we 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 PROBE flag, so userspace can easily >>> check if the kernel supports it(Jason). >>> - use mmap_read_{lock, unlock}. >>> - add some kernel-doc. >>> v3: >>> - In the docs also mention that PROBE doesn't guarantee that the pages >>> will remain valid by the time they are actually used(Tvrtko). >>> - Add a small comment for the hole finding logic(Jason). >>> - Move the param next to all the other params which just return true. >>> >>> 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> >>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> Acked-by: Kenneth Graunke <kenneth@whitecape.org> >>> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> >>> --- >>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++- >>> drivers/gpu/drm/i915/i915_getparam.c | 1 + >>> include/uapi/drm/i915_drm.h | 20 ++++++++++ >>> 3 files changed, 61 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..468a7a617fbf 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>> @@ -422,6 +422,34 @@ 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) { >>> + /* Check for holes, note that we also update the addr below */ >>> + 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 +505,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 +533,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..bbb7cac43eb4 100644 >>> --- a/drivers/gpu/drm/i915/i915_getparam.c >>> +++ b/drivers/gpu/drm/i915/i915_getparam.c >>> @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, >>> case I915_PARAM_HAS_EXEC_FENCE_ARRAY: >>> case I915_PARAM_HAS_EXEC_SUBMIT_FENCE: >>> case I915_PARAM_HAS_EXEC_TIMELINE_FENCES: >>> + case I915_PARAM_HAS_USERPTR_PROBE: >>> /* For the time being all of these are always true; >>> * if some supported hardware does not have one of these >>> * features this value needs to be provided from >>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h >>> index 975087553ea0..0d290535a6e5 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 { >>> @@ -2222,12 +2225,29 @@ 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, and also doesn't >>> + * guarantee that the object will remain valid when the object is >>> + * eventually used. >>> + * >>> + * 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. >> >> Could we use _VALIDATE instead of probe? Or at least pin the pages as well, so we don't have to do it later? > > I only care that the name matches what it does. _VALIDATE sounds like > it does a full validation of everything such that, if the import > succeeds, execbuf will as well. If we pin the pages at the same time, > maybe that's true? _PROBE, on the other hand, sounds a lot more like No it is not possible to guarantee backing store remains valid until execbuf. > a one-time best-effort check which may race with other stuff and > doesn't guarantee future success. That's in line with what the > current patch does. > >> We already have i915_gem_object_userptr_validate, no need to dupe it. > > I have no opinion on this. I was actually suggesting the same as Maarten here - that we should add a "populate" flag. But opinion was that was not desired - please look for the older threads to see the reasoning there. Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation 2021-07-26 16:10 ` Tvrtko Ursulin @ 2021-07-28 14:22 ` Matthew Auld 2021-08-03 15:09 ` Daniel Vetter 0 siblings, 1 reply; 16+ messages in thread From: Matthew Auld @ 2021-07-28 14:22 UTC (permalink / raw) To: Tvrtko Ursulin Cc: Thomas Hellström, Daniel Vetter, Intel GFX, Maling list - DRI developers, Chris Wilson, Kenneth Graunke, Matthew Auld On Mon, 26 Jul 2021 at 17:10, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > > On 26/07/2021 16:14, Jason Ekstrand wrote: > > On Mon, Jul 26, 2021 at 3:31 AM Maarten Lankhorst > > <maarten.lankhorst@linux.intel.com> wrote: > >> > >> Op 23-07-2021 om 13:34 schreef Matthew Auld: > >>> 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 we 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 PROBE flag, so userspace can easily > >>> check if the kernel supports it(Jason). > >>> - use mmap_read_{lock, unlock}. > >>> - add some kernel-doc. > >>> v3: > >>> - In the docs also mention that PROBE doesn't guarantee that the pages > >>> will remain valid by the time they are actually used(Tvrtko). > >>> - Add a small comment for the hole finding logic(Jason). > >>> - Move the param next to all the other params which just return true. > >>> > >>> 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> > >>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>> Acked-by: Kenneth Graunke <kenneth@whitecape.org> > >>> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> > >>> --- > >>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++- > >>> drivers/gpu/drm/i915/i915_getparam.c | 1 + > >>> include/uapi/drm/i915_drm.h | 20 ++++++++++ > >>> 3 files changed, 61 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..468a7a617fbf 100644 > >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > >>> @@ -422,6 +422,34 @@ 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) { > >>> + /* Check for holes, note that we also update the addr below */ > >>> + 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 +505,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 +533,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..bbb7cac43eb4 100644 > >>> --- a/drivers/gpu/drm/i915/i915_getparam.c > >>> +++ b/drivers/gpu/drm/i915/i915_getparam.c > >>> @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > >>> case I915_PARAM_HAS_EXEC_FENCE_ARRAY: > >>> case I915_PARAM_HAS_EXEC_SUBMIT_FENCE: > >>> case I915_PARAM_HAS_EXEC_TIMELINE_FENCES: > >>> + case I915_PARAM_HAS_USERPTR_PROBE: > >>> /* For the time being all of these are always true; > >>> * if some supported hardware does not have one of these > >>> * features this value needs to be provided from > >>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > >>> index 975087553ea0..0d290535a6e5 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 { > >>> @@ -2222,12 +2225,29 @@ 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, and also doesn't > >>> + * guarantee that the object will remain valid when the object is > >>> + * eventually used. > >>> + * > >>> + * 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. > >> > >> Could we use _VALIDATE instead of probe? Or at least pin the pages as well, so we don't have to do it later? > > > > I only care that the name matches what it does. _VALIDATE sounds like > > it does a full validation of everything such that, if the import > > succeeds, execbuf will as well. If we pin the pages at the same time, > > maybe that's true? _PROBE, on the other hand, sounds a lot more like > > No it is not possible to guarantee backing store remains valid until > execbuf. > > > a one-time best-effort check which may race with other stuff and > > doesn't guarantee future success. That's in line with what the > > current patch does. > > > >> We already have i915_gem_object_userptr_validate, no need to dupe it. > > > > I have no opinion on this. > > I was actually suggesting the same as Maarten here - that we should add > a "populate" flag. But opinion was that was not desired - please look > for the older threads to see the reasoning there. So how should we proceed here? Maarten? > > Regards, > > Tvrtko > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation 2021-07-28 14:22 ` Matthew Auld @ 2021-08-03 15:09 ` Daniel Vetter 2021-08-03 15:45 ` Jason Ekstrand 0 siblings, 1 reply; 16+ messages in thread From: Daniel Vetter @ 2021-08-03 15:09 UTC (permalink / raw) To: Matthew Auld Cc: Tvrtko Ursulin, Jason Ekstrand, Maarten Lankhorst, Thomas Hellström, Intel GFX, Maling list - DRI developers, Chris Wilson, Kenneth Graunke, Matthew Auld On Wed, Jul 28, 2021 at 4:22 PM Matthew Auld <matthew.william.auld@gmail.com> wrote: > > On Mon, 26 Jul 2021 at 17:10, Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com> wrote: > > > > > > On 26/07/2021 16:14, Jason Ekstrand wrote: > > > On Mon, Jul 26, 2021 at 3:31 AM Maarten Lankhorst > > > <maarten.lankhorst@linux.intel.com> wrote: > > >> > > >> Op 23-07-2021 om 13:34 schreef Matthew Auld: > > >>> 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 we 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 PROBE flag, so userspace can easily > > >>> check if the kernel supports it(Jason). > > >>> - use mmap_read_{lock, unlock}. > > >>> - add some kernel-doc. > > >>> v3: > > >>> - In the docs also mention that PROBE doesn't guarantee that the pages > > >>> will remain valid by the time they are actually used(Tvrtko). > > >>> - Add a small comment for the hole finding logic(Jason). > > >>> - Move the param next to all the other params which just return true. > > >>> > > >>> 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> > > >>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > >>> Acked-by: Kenneth Graunke <kenneth@whitecape.org> > > >>> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> > > >>> --- > > >>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++- > > >>> drivers/gpu/drm/i915/i915_getparam.c | 1 + > > >>> include/uapi/drm/i915_drm.h | 20 ++++++++++ > > >>> 3 files changed, 61 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..468a7a617fbf 100644 > > >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > >>> @@ -422,6 +422,34 @@ 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) { > > >>> + /* Check for holes, note that we also update the addr below */ > > >>> + 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 +505,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 +533,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..bbb7cac43eb4 100644 > > >>> --- a/drivers/gpu/drm/i915/i915_getparam.c > > >>> +++ b/drivers/gpu/drm/i915/i915_getparam.c > > >>> @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > > >>> case I915_PARAM_HAS_EXEC_FENCE_ARRAY: > > >>> case I915_PARAM_HAS_EXEC_SUBMIT_FENCE: > > >>> case I915_PARAM_HAS_EXEC_TIMELINE_FENCES: > > >>> + case I915_PARAM_HAS_USERPTR_PROBE: > > >>> /* For the time being all of these are always true; > > >>> * if some supported hardware does not have one of these > > >>> * features this value needs to be provided from > > >>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > >>> index 975087553ea0..0d290535a6e5 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 { > > >>> @@ -2222,12 +2225,29 @@ 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, and also doesn't > > >>> + * guarantee that the object will remain valid when the object is > > >>> + * eventually used. > > >>> + * > > >>> + * 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. > > >> > > >> Could we use _VALIDATE instead of probe? Or at least pin the pages as well, so we don't have to do it later? > > > > > > I only care that the name matches what it does. _VALIDATE sounds like > > > it does a full validation of everything such that, if the import > > > succeeds, execbuf will as well. If we pin the pages at the same time, > > > maybe that's true? _PROBE, on the other hand, sounds a lot more like > > > > No it is not possible to guarantee backing store remains valid until > > execbuf. > > > > > a one-time best-effort check which may race with other stuff and > > > doesn't guarantee future success. That's in line with what the > > > current patch does. > > > > > >> We already have i915_gem_object_userptr_validate, no need to dupe it. > > > > > > I have no opinion on this. > > > > I was actually suggesting the same as Maarten here - that we should add > > a "populate" flag. But opinion was that was not desired - please look > > for the older threads to see the reasoning there. > > So how should we proceed here? Maarten? I honestly don't care, and I think the probe flag here is perfectly fine. Reasons for that: - we don't have an immediate allocation flag for buffer creation either. So if there's a need we need a flag for this across the board, not just userptr, and a clear userspace ask - it's a fundamentally racy test anyway, userspace can munmap or map something else and then it will fail. So we really don't gain anything by pinning pages because by the time we go into execbuf they might be invalidated already - checking the vmas for VM_SPECIAL is perfectly good enough. - we can always change the implementation later on too. Hence why I think PROBE is the semantics we want/need here. Can we get some acks/reviews here or is this really a significant enough bikeshed that we need to hold up dg1 pciids for them? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation 2021-08-03 15:09 ` Daniel Vetter @ 2021-08-03 15:45 ` Jason Ekstrand 2021-08-03 15:57 ` Maarten Lankhorst 0 siblings, 1 reply; 16+ messages in thread From: Jason Ekstrand @ 2021-08-03 15:45 UTC (permalink / raw) To: Daniel Vetter Cc: Matthew Auld, Tvrtko Ursulin, Maarten Lankhorst, Thomas Hellström, Intel GFX, Maling list - DRI developers, Chris Wilson, Kenneth Graunke, Matthew Auld On Tue, Aug 3, 2021 at 10:09 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > On Wed, Jul 28, 2021 at 4:22 PM Matthew Auld > <matthew.william.auld@gmail.com> wrote: > > > > On Mon, 26 Jul 2021 at 17:10, Tvrtko Ursulin > > <tvrtko.ursulin@linux.intel.com> wrote: > > > > > > > > > On 26/07/2021 16:14, Jason Ekstrand wrote: > > > > On Mon, Jul 26, 2021 at 3:31 AM Maarten Lankhorst > > > > <maarten.lankhorst@linux.intel.com> wrote: > > > >> > > > >> Op 23-07-2021 om 13:34 schreef Matthew Auld: > > > >>> 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 we 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 PROBE flag, so userspace can easily > > > >>> check if the kernel supports it(Jason). > > > >>> - use mmap_read_{lock, unlock}. > > > >>> - add some kernel-doc. > > > >>> v3: > > > >>> - In the docs also mention that PROBE doesn't guarantee that the pages > > > >>> will remain valid by the time they are actually used(Tvrtko). > > > >>> - Add a small comment for the hole finding logic(Jason). > > > >>> - Move the param next to all the other params which just return true. > > > >>> > > > >>> 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> > > > >>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > >>> Acked-by: Kenneth Graunke <kenneth@whitecape.org> > > > >>> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> > > > >>> --- > > > >>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++- > > > >>> drivers/gpu/drm/i915/i915_getparam.c | 1 + > > > >>> include/uapi/drm/i915_drm.h | 20 ++++++++++ > > > >>> 3 files changed, 61 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..468a7a617fbf 100644 > > > >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > > >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > > > >>> @@ -422,6 +422,34 @@ 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) { > > > >>> + /* Check for holes, note that we also update the addr below */ > > > >>> + 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 +505,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 +533,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..bbb7cac43eb4 100644 > > > >>> --- a/drivers/gpu/drm/i915/i915_getparam.c > > > >>> +++ b/drivers/gpu/drm/i915/i915_getparam.c > > > >>> @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, > > > >>> case I915_PARAM_HAS_EXEC_FENCE_ARRAY: > > > >>> case I915_PARAM_HAS_EXEC_SUBMIT_FENCE: > > > >>> case I915_PARAM_HAS_EXEC_TIMELINE_FENCES: > > > >>> + case I915_PARAM_HAS_USERPTR_PROBE: > > > >>> /* For the time being all of these are always true; > > > >>> * if some supported hardware does not have one of these > > > >>> * features this value needs to be provided from > > > >>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > > >>> index 975087553ea0..0d290535a6e5 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 { > > > >>> @@ -2222,12 +2225,29 @@ 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, and also doesn't > > > >>> + * guarantee that the object will remain valid when the object is > > > >>> + * eventually used. > > > >>> + * > > > >>> + * 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. > > > >> > > > >> Could we use _VALIDATE instead of probe? Or at least pin the pages as well, so we don't have to do it later? > > > > > > > > I only care that the name matches what it does. _VALIDATE sounds like > > > > it does a full validation of everything such that, if the import > > > > succeeds, execbuf will as well. If we pin the pages at the same time, > > > > maybe that's true? _PROBE, on the other hand, sounds a lot more like > > > > > > No it is not possible to guarantee backing store remains valid until > > > execbuf. > > > > > > > a one-time best-effort check which may race with other stuff and > > > > doesn't guarantee future success. That's in line with what the > > > > current patch does. > > > > > > > >> We already have i915_gem_object_userptr_validate, no need to dupe it. > > > > > > > > I have no opinion on this. > > > > > > I was actually suggesting the same as Maarten here - that we should add > > > a "populate" flag. But opinion was that was not desired - please look > > > for the older threads to see the reasoning there. > > > > So how should we proceed here? Maarten? > > I honestly don't care, and I think the probe flag here is perfectly > fine. Reasons for that: > - we don't have an immediate allocation flag for buffer creation > either. So if there's a need we need a flag for this across the board, > not just userptr, and a clear userspace ask Both Mesa drivers would probably set that flag if we had it and it demonstrated any perf benefits, FWIW. However, I think it's fine if that's a separate flag. Also, I don't know that the perf benefits are all that great. We should get most of those benefits from VM_BIND anyway. > - it's a fundamentally racy test anyway, userspace can munmap or map > something else and then it will fail. So we really don't gain anything > by pinning pages because by the time we go into execbuf they might be > invalidated already - checking the vmas for VM_SPECIAL is perfectly > good enough. > - we can always change the implementation later on too. > > Hence why I think PROBE is the semantics we want/need here. Can we get > some acks/reviews here or is this really a significant enough bikeshed > that we need to hold up dg1 pciids for them? I don't care. I've already reviewed the patch. --Jason ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation 2021-08-03 15:45 ` Jason Ekstrand @ 2021-08-03 15:57 ` Maarten Lankhorst 2021-08-05 10:14 ` Maarten Lankhorst 0 siblings, 1 reply; 16+ messages in thread From: Maarten Lankhorst @ 2021-08-03 15:57 UTC (permalink / raw) To: Jason Ekstrand, Daniel Vetter Cc: Matthew Auld, Tvrtko Ursulin, Thomas Hellström, Intel GFX, Maling list - DRI developers, Chris Wilson, Kenneth Graunke, Matthew Auld Op 2021-08-03 om 17:45 schreef Jason Ekstrand: > On Tue, Aug 3, 2021 at 10:09 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> On Wed, Jul 28, 2021 at 4:22 PM Matthew Auld >> <matthew.william.auld@gmail.com> wrote: >>> On Mon, 26 Jul 2021 at 17:10, Tvrtko Ursulin >>> <tvrtko.ursulin@linux.intel.com> wrote: >>>> >>>> On 26/07/2021 16:14, Jason Ekstrand wrote: >>>>> On Mon, Jul 26, 2021 at 3:31 AM Maarten Lankhorst >>>>> <maarten.lankhorst@linux.intel.com> wrote: >>>>>> Op 23-07-2021 om 13:34 schreef Matthew Auld: >>>>>>> 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 we 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 PROBE flag, so userspace can easily >>>>>>> check if the kernel supports it(Jason). >>>>>>> - use mmap_read_{lock, unlock}. >>>>>>> - add some kernel-doc. >>>>>>> v3: >>>>>>> - In the docs also mention that PROBE doesn't guarantee that the pages >>>>>>> will remain valid by the time they are actually used(Tvrtko). >>>>>>> - Add a small comment for the hole finding logic(Jason). >>>>>>> - Move the param next to all the other params which just return true. >>>>>>> >>>>>>> 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> >>>>>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>>>>> Acked-by: Kenneth Graunke <kenneth@whitecape.org> >>>>>>> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> >>>>>>> --- >>>>>>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++- >>>>>>> drivers/gpu/drm/i915/i915_getparam.c | 1 + >>>>>>> include/uapi/drm/i915_drm.h | 20 ++++++++++ >>>>>>> 3 files changed, 61 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..468a7a617fbf 100644 >>>>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>>>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>>>>>> @@ -422,6 +422,34 @@ 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) { >>>>>>> + /* Check for holes, note that we also update the addr below */ >>>>>>> + 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 +505,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 +533,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..bbb7cac43eb4 100644 >>>>>>> --- a/drivers/gpu/drm/i915/i915_getparam.c >>>>>>> +++ b/drivers/gpu/drm/i915/i915_getparam.c >>>>>>> @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, >>>>>>> case I915_PARAM_HAS_EXEC_FENCE_ARRAY: >>>>>>> case I915_PARAM_HAS_EXEC_SUBMIT_FENCE: >>>>>>> case I915_PARAM_HAS_EXEC_TIMELINE_FENCES: >>>>>>> + case I915_PARAM_HAS_USERPTR_PROBE: >>>>>>> /* For the time being all of these are always true; >>>>>>> * if some supported hardware does not have one of these >>>>>>> * features this value needs to be provided from >>>>>>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h >>>>>>> index 975087553ea0..0d290535a6e5 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 { >>>>>>> @@ -2222,12 +2225,29 @@ 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, and also doesn't >>>>>>> + * guarantee that the object will remain valid when the object is >>>>>>> + * eventually used. >>>>>>> + * >>>>>>> + * 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. >>>>>> Could we use _VALIDATE instead of probe? Or at least pin the pages as well, so we don't have to do it later? >>>>> I only care that the name matches what it does. _VALIDATE sounds like >>>>> it does a full validation of everything such that, if the import >>>>> succeeds, execbuf will as well. If we pin the pages at the same time, >>>>> maybe that's true? _PROBE, on the other hand, sounds a lot more like >>>> No it is not possible to guarantee backing store remains valid until >>>> execbuf. >>>> >>>>> a one-time best-effort check which may race with other stuff and >>>>> doesn't guarantee future success. That's in line with what the >>>>> current patch does. >>>>> >>>>>> We already have i915_gem_object_userptr_validate, no need to dupe it. >>>>> I have no opinion on this. >>>> I was actually suggesting the same as Maarten here - that we should add >>>> a "populate" flag. But opinion was that was not desired - please look >>>> for the older threads to see the reasoning there. >>> So how should we proceed here? Maarten? >> I honestly don't care, and I think the probe flag here is perfectly >> fine. Reasons for that: >> - we don't have an immediate allocation flag for buffer creation >> either. So if there's a need we need a flag for this across the board, >> not just userptr, and a clear userspace ask > Both Mesa drivers would probably set that flag if we had it and it > demonstrated any perf benefits, FWIW. However, I think it's fine if > that's a separate flag. Also, I don't know that the perf benefits are > all that great. We should get most of those benefits from VM_BIND > anyway. > >> - it's a fundamentally racy test anyway, userspace can munmap or map >> something else and then it will fail. So we really don't gain anything >> by pinning pages because by the time we go into execbuf they might be >> invalidated already - checking the vmas for VM_SPECIAL is perfectly >> good enough. >> - we can always change the implementation later on too. >> >> Hence why I think PROBE is the semantics we want/need here. Can we get >> some acks/reviews here or is this really a significant enough bikeshed >> that we need to hold up dg1 pciids for them? > I don't care. I've already reviewed the patch. > > --Jason I think we should still just put the validate() call in there, but I'm not going to hold up the implementation because of that. Acked-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation 2021-08-03 15:57 ` Maarten Lankhorst @ 2021-08-05 10:14 ` Maarten Lankhorst 0 siblings, 0 replies; 16+ messages in thread From: Maarten Lankhorst @ 2021-08-05 10:14 UTC (permalink / raw) To: Jason Ekstrand, Daniel Vetter Cc: Matthew Auld, Tvrtko Ursulin, Thomas Hellström, Intel GFX, Maling list - DRI developers, Chris Wilson, Kenneth Graunke, Matthew Auld Op 03-08-2021 om 17:57 schreef Maarten Lankhorst: > Op 2021-08-03 om 17:45 schreef Jason Ekstrand: >> On Tue, Aug 3, 2021 at 10:09 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >>> On Wed, Jul 28, 2021 at 4:22 PM Matthew Auld >>> <matthew.william.auld@gmail.com> wrote: >>>> On Mon, 26 Jul 2021 at 17:10, Tvrtko Ursulin >>>> <tvrtko.ursulin@linux.intel.com> wrote: >>>>> On 26/07/2021 16:14, Jason Ekstrand wrote: >>>>>> On Mon, Jul 26, 2021 at 3:31 AM Maarten Lankhorst >>>>>> <maarten.lankhorst@linux.intel.com> wrote: >>>>>>> Op 23-07-2021 om 13:34 schreef Matthew Auld: >>>>>>>> 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 we 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 PROBE flag, so userspace can easily >>>>>>>> check if the kernel supports it(Jason). >>>>>>>> - use mmap_read_{lock, unlock}. >>>>>>>> - add some kernel-doc. >>>>>>>> v3: >>>>>>>> - In the docs also mention that PROBE doesn't guarantee that the pages >>>>>>>> will remain valid by the time they are actually used(Tvrtko). >>>>>>>> - Add a small comment for the hole finding logic(Jason). >>>>>>>> - Move the param next to all the other params which just return true. >>>>>>>> >>>>>>>> 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> >>>>>>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>>>>>> Acked-by: Kenneth Graunke <kenneth@whitecape.org> >>>>>>>> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++++- >>>>>>>> drivers/gpu/drm/i915/i915_getparam.c | 1 + >>>>>>>> include/uapi/drm/i915_drm.h | 20 ++++++++++ >>>>>>>> 3 files changed, 61 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..468a7a617fbf 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>>>>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c >>>>>>>> @@ -422,6 +422,34 @@ 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) { >>>>>>>> + /* Check for holes, note that we also update the addr below */ >>>>>>>> + 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 +505,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 +533,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..bbb7cac43eb4 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/i915_getparam.c >>>>>>>> +++ b/drivers/gpu/drm/i915/i915_getparam.c >>>>>>>> @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, >>>>>>>> case I915_PARAM_HAS_EXEC_FENCE_ARRAY: >>>>>>>> case I915_PARAM_HAS_EXEC_SUBMIT_FENCE: >>>>>>>> case I915_PARAM_HAS_EXEC_TIMELINE_FENCES: >>>>>>>> + case I915_PARAM_HAS_USERPTR_PROBE: >>>>>>>> /* For the time being all of these are always true; >>>>>>>> * if some supported hardware does not have one of these >>>>>>>> * features this value needs to be provided from >>>>>>>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h >>>>>>>> index 975087553ea0..0d290535a6e5 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 { >>>>>>>> @@ -2222,12 +2225,29 @@ 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, and also doesn't >>>>>>>> + * guarantee that the object will remain valid when the object is >>>>>>>> + * eventually used. >>>>>>>> + * >>>>>>>> + * 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. >>>>>>> Could we use _VALIDATE instead of probe? Or at least pin the pages as well, so we don't have to do it later? >>>>>> I only care that the name matches what it does. _VALIDATE sounds like >>>>>> it does a full validation of everything such that, if the import >>>>>> succeeds, execbuf will as well. If we pin the pages at the same time, >>>>>> maybe that's true? _PROBE, on the other hand, sounds a lot more like >>>>> No it is not possible to guarantee backing store remains valid until >>>>> execbuf. >>>>> >>>>>> a one-time best-effort check which may race with other stuff and >>>>>> doesn't guarantee future success. That's in line with what the >>>>>> current patch does. >>>>>> >>>>>>> We already have i915_gem_object_userptr_validate, no need to dupe it. >>>>>> I have no opinion on this. >>>>> I was actually suggesting the same as Maarten here - that we should add >>>>> a "populate" flag. But opinion was that was not desired - please look >>>>> for the older threads to see the reasoning there. >>>> So how should we proceed here? Maarten? >>> I honestly don't care, and I think the probe flag here is perfectly >>> fine. Reasons for that: >>> - we don't have an immediate allocation flag for buffer creation >>> either. So if there's a need we need a flag for this across the board, >>> not just userptr, and a clear userspace ask >> Both Mesa drivers would probably set that flag if we had it and it >> demonstrated any perf benefits, FWIW. However, I think it's fine if >> that's a separate flag. Also, I don't know that the perf benefits are >> all that great. We should get most of those benefits from VM_BIND >> anyway. >> >>> - it's a fundamentally racy test anyway, userspace can munmap or map >>> something else and then it will fail. So we really don't gain anything >>> by pinning pages because by the time we go into execbuf they might be >>> invalidated already - checking the vmas for VM_SPECIAL is perfectly >>> good enough. >>> - we can always change the implementation later on too. >>> >>> Hence why I think PROBE is the semantics we want/need here. Can we get >>> some acks/reviews here or is this really a significant enough bikeshed >>> that we need to hold up dg1 pciids for them? >> I don't care. I've already reviewed the patch. >> >> --Jason > I think we should still just put the validate() call in there, but I'm not going to hold up the implementation because of that. > > Acked-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > And pushed together with the IGT. :) ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-08-05 10:14 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-23 11:34 [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation Matthew Auld 2021-07-23 16:50 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork 2021-07-23 17:47 ` [Intel-gfx] [PATCH] " Jason Ekstrand 2021-07-23 17:49 ` Jason Ekstrand 2021-07-26 8:03 ` Matthew Auld 2021-07-26 8:06 ` Matthew Auld 2021-07-26 15:12 ` Jason Ekstrand 2021-07-24 1:38 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork 2021-07-26 8:31 ` [Intel-gfx] [PATCH] " Maarten Lankhorst 2021-07-26 15:14 ` Jason Ekstrand 2021-07-26 16:10 ` Tvrtko Ursulin 2021-07-28 14:22 ` Matthew Auld 2021-08-03 15:09 ` Daniel Vetter 2021-08-03 15:45 ` Jason Ekstrand 2021-08-03 15:57 ` Maarten Lankhorst 2021-08-05 10:14 ` Maarten Lankhorst
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).