| Message ID | 20231128162505.3493942-1-jonathan.cavitt@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gem: Atomically invalidate userptr on mmu-notifier | expand |
Hi Jonathan, On Tue, Nov 28, 2023 at 08:25:05AM -0800, Jonathan Cavitt wrote: > Never block for outstanding work on userptr object upon receipt of a > mmu-notifier. The reason we originally did so was to immediately unbind > the userptr and unpin its pages, but since that has been dropped in > commit b4b9731b02c3c ("drm/i915: Simplify userptr locking"), we never > return the pages to the system i.e. never drop our page->mapcount and so > do not allow the page and CPU PTE to be revoked. Based on this history, > we know we are safe to drop the wait entirely. > > Upon return from mmu-notifier, we will still have the userptr pages > pinned preventing the following PTE operation (such as try_to_unmap) > adjusting the vm_area_struct, so it is safe to keep the pages around for > as long as we still have i/o pending. > > We do not have any means currently to asynchronously revalidate the > userptr pages, that is always prior to next use. > > Signed-off-by: Chris Wilson <chris.p.wilson@linux.intel.com> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Thanks, Andi
-----Original Message----- From: Andi Shyti <andi.shyti@linux.intel.com> Sent: Monday, December 18, 2023 8:06 AM To: Cavitt, Jonathan <jonathan.cavitt@intel.com> Cc: intel-gfx@lists.freedesktop.org; Gupta, saurabhg <saurabhg.gupta@intel.com>; chris.p.wilson@linux.intel.com Subject: Re: [Intel-gfx] [PATCH] drm/i915/gem: Atomically invalidate userptr on mmu-notifier > > Hi Jonathan, > > On Tue, Nov 28, 2023 at 08:25:05AM -0800, Jonathan Cavitt wrote: > > Never block for outstanding work on userptr object upon receipt of a > > mmu-notifier. The reason we originally did so was to immediately unbind > > the userptr and unpin its pages, but since that has been dropped in > > commit b4b9731b02c3c ("drm/i915: Simplify userptr locking"), we never > > return the pages to the system i.e. never drop our page->mapcount and so > > do not allow the page and CPU PTE to be revoked. Based on this history, > > we know we are safe to drop the wait entirely. > > > > Upon return from mmu-notifier, we will still have the userptr pages > > pinned preventing the following PTE operation (such as try_to_unmap) > > adjusting the vm_area_struct, so it is safe to keep the pages around for > > as long as we still have i/o pending. > > > > We do not have any means currently to asynchronously revalidate the > > userptr pages, that is always prior to next use. > > > > Signed-off-by: Chris Wilson <chris.p.wilson@linux.intel.com> > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com> > > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Thank you for the review! I don't think I have permission to push this upstream, though, so you or someone else will have to complete the push. -Jonathan Cavitt > > Thanks, > Andi >
Hi Jonathan, On Mon, Dec 18, 2023 at 06:33:44PM +0000, Cavitt, Jonathan wrote: ... > > On Tue, Nov 28, 2023 at 08:25:05AM -0800, Jonathan Cavitt wrote: > > > Never block for outstanding work on userptr object upon receipt of a > > > mmu-notifier. The reason we originally did so was to immediately unbind > > > the userptr and unpin its pages, but since that has been dropped in > > > commit b4b9731b02c3c ("drm/i915: Simplify userptr locking"), we never > > > return the pages to the system i.e. never drop our page->mapcount and so > > > do not allow the page and CPU PTE to be revoked. Based on this history, > > > we know we are safe to drop the wait entirely. > > > > > > Upon return from mmu-notifier, we will still have the userptr pages > > > pinned preventing the following PTE operation (such as try_to_unmap) > > > adjusting the vm_area_struct, so it is safe to keep the pages around for > > > as long as we still have i/o pending. > > > > > > We do not have any means currently to asynchronously revalidate the > > > userptr pages, that is always prior to next use. > > > > > > Signed-off-by: Chris Wilson <chris.p.wilson@linux.intel.com> > > > Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com> > > > > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> > > Thank you for the review! I don't think I have permission to push this upstream, > though, so you or someone else will have to complete the push. pushed in drm-intel-gt-next. Thanks and sorry for the late merge. Andi
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index b1aa62dfb155d..eff601389ef85 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -2159,12 +2159,6 @@ static int eb_move_to_gpu(struct i915_execbuffer *eb) #ifdef CONFIG_MMU_NOTIFIER if (!err && (eb->args->flags & __EXEC_USERPTR_USED)) { - read_lock(&eb->i915->mm.notifier_lock); - - /* - * count is always at least 1, otherwise __EXEC_USERPTR_USED - * could not have been set - */ for (i = 0; i < count; i++) { struct eb_vma *ev = &eb->vma[i]; struct drm_i915_gem_object *obj = ev->vma->obj; @@ -2176,8 +2170,6 @@ static int eb_move_to_gpu(struct i915_execbuffer *eb) if (err) break; } - - read_unlock(&eb->i915->mm.notifier_lock); } #endif diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c index 1d3ebdf4069b5..0e21ce9d3e5ac 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c @@ -42,7 +42,6 @@ #include "i915_drv.h" #include "i915_gem_ioctls.h" #include "i915_gem_object.h" -#include "i915_gem_userptr.h" #include "i915_scatterlist.h" #ifdef CONFIG_MMU_NOTIFIER @@ -61,36 +60,7 @@ static bool i915_gem_userptr_invalidate(struct mmu_interval_notifier *mni, const struct mmu_notifier_range *range, unsigned long cur_seq) { - struct drm_i915_gem_object *obj = container_of(mni, struct drm_i915_gem_object, userptr.notifier); - struct drm_i915_private *i915 = to_i915(obj->base.dev); - long r; - - if (!mmu_notifier_range_blockable(range)) - return false; - - write_lock(&i915->mm.notifier_lock); - mmu_interval_set_seq(mni, cur_seq); - - write_unlock(&i915->mm.notifier_lock); - - /* - * We don't wait when the process is exiting. This is valid - * because the object will be cleaned up anyway. - * - * This is also temporarily required as a hack, because we - * cannot currently force non-consistent batch buffers to preempt - * and reschedule by waiting on it, hanging processes on exit. - */ - if (current->flags & PF_EXITING) - return true; - - /* we will unbind on next submission, still have userptr pins */ - r = dma_resv_wait_timeout(obj->base.resv, DMA_RESV_USAGE_BOOKKEEP, false, - MAX_SCHEDULE_TIMEOUT); - if (r <= 0) - drm_err(&i915->drm, "(%ld) failed to wait for idle\n", r); - return true; } @@ -580,15 +550,3 @@ i915_gem_userptr_ioctl(struct drm_device *dev, #endif } -int i915_gem_init_userptr(struct drm_i915_private *dev_priv) -{ -#ifdef CONFIG_MMU_NOTIFIER - rwlock_init(&dev_priv->mm.notifier_lock); -#endif - - return 0; -} - -void i915_gem_cleanup_userptr(struct drm_i915_private *dev_priv) -{ -} diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.h b/drivers/gpu/drm/i915/gem/i915_gem_userptr.h deleted file mode 100644 index 8dadb2f8436d4..0000000000000 --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.h +++ /dev/null @@ -1,14 +0,0 @@ -/* SPDX-License-Identifier: MIT */ -/* - * Copyright © 2021 Intel Corporation - */ - -#ifndef __I915_GEM_USERPTR_H__ -#define __I915_GEM_USERPTR_H__ - -struct drm_i915_private; - -int i915_gem_init_userptr(struct drm_i915_private *dev_priv); -void i915_gem_cleanup_userptr(struct drm_i915_private *dev_priv); - -#endif /* __I915_GEM_USERPTR_H__ */ diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 861567362abd1..e81b3b2858acc 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -165,14 +165,6 @@ struct i915_gem_mm { struct notifier_block vmap_notifier; struct shrinker *shrinker; -#ifdef CONFIG_MMU_NOTIFIER - /** - * notifier_lock for mmu notifiers, memory may not be allocated - * while holding this lock. - */ - rwlock_t notifier_lock; -#endif - /* shrinker accounting, also useful for userland debugging */ u64 shrink_memory; u32 shrink_count; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 92758b6b41f01..1391c01d7663e 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -48,7 +48,6 @@ #include "gem/i915_gem_object_frontbuffer.h" #include "gem/i915_gem_pm.h" #include "gem/i915_gem_region.h" -#include "gem/i915_gem_userptr.h" #include "gt/intel_engine_user.h" #include "gt/intel_gt.h" #include "gt/intel_gt_pm.h" @@ -1165,10 +1164,6 @@ int i915_gem_init(struct drm_i915_private *dev_priv) if (intel_vgpu_active(dev_priv) && !intel_vgpu_has_huge_gtt(dev_priv)) RUNTIME_INFO(dev_priv)->page_sizes = I915_GTT_PAGE_SIZE_4K; - ret = i915_gem_init_userptr(dev_priv); - if (ret) - return ret; - for_each_gt(gt, dev_priv, i) { intel_uc_fetch_firmwares(>->uc); intel_wopcm_init(>->wopcm);
Never block for outstanding work on userptr object upon receipt of a mmu-notifier. The reason we originally did so was to immediately unbind the userptr and unpin its pages, but since that has been dropped in commit b4b9731b02c3c ("drm/i915: Simplify userptr locking"), we never return the pages to the system i.e. never drop our page->mapcount and so do not allow the page and CPU PTE to be revoked. Based on this history, we know we are safe to drop the wait entirely. Upon return from mmu-notifier, we will still have the userptr pages pinned preventing the following PTE operation (such as try_to_unmap) adjusting the vm_area_struct, so it is safe to keep the pages around for as long as we still have i/o pending. We do not have any means currently to asynchronously revalidate the userptr pages, that is always prior to next use. Signed-off-by: Chris Wilson <chris.p.wilson@linux.intel.com> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com> --- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 8 ---- drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 42 ------------------- drivers/gpu/drm/i915/gem/i915_gem_userptr.h | 14 ------- drivers/gpu/drm/i915/i915_drv.h | 8 ---- drivers/gpu/drm/i915/i915_gem.c | 5 --- 5 files changed, 77 deletions(-) delete mode 100644 drivers/gpu/drm/i915/gem/i915_gem_userptr.h