We want to exclude any GGTT objects from being present on our internal
lists to avoid the deadlock we may run into with our requirement for
struct_mutex during invalidate. However, if the gup_fast fails, we put
the userptr onto the workqueue and mark it as active, so that we
remember to serialise the worker upon mmu_invalidate.
Note that despite the previous fix, it is still better to avoid the
struct_mutex recursion where possible, leaving the recursion only to
handle the shrinker-esque paths.
v2: Hold mmap_sem to prevent modifications to the mm while we probe and
add ourselves to the interval-tree for notificiation.
v3: Rely on mmap_sem for a simpler patch.
v4: Mark up the mmap_sem nesting
v5: Don't deactivate on -EAGAIN as that means the worker is queued
v6: Fight the indentation and chained if-else error handling
v7: Fight again.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104209
Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
Cc: Michał Winiarski <michal.winiar...@intel.com>
---
drivers/gpu/drm/i915/i915_gem_userptr.c | 150 ++++++++++++++++--------
1 file changed, 98 insertions(+), 52 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c
b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 9c2008a480e2..7b9e14bc3bc5 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -435,7 +435,7 @@ struct get_pages_work {
struct task_struct *task;
};
-static struct sg_table *
+static int
__i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
struct page **pvec, int num_pages)
{
@@ -446,7 +446,7 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object
*obj,
st = kmalloc(sizeof(*st), GFP_KERNEL);
if (!st)
- return ERR_PTR(-ENOMEM);
+ return -ENOMEM;
alloc_table:
ret = __sg_alloc_table_from_pages(st, pvec, num_pages,
@@ -455,7 +455,7 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object
*obj,
GFP_KERNEL);
if (ret) {
kfree(st);
- return ERR_PTR(ret);
+ return ret;
}
ret = i915_gem_gtt_prepare_pages(obj, st);
@@ -468,14 +468,14 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object
*obj,
}
kfree(st);
- return ERR_PTR(ret);
+ return ret;
}
sg_page_sizes = i915_sg_page_sizes(st->sgl);
__i915_gem_object_set_pages(obj, st, sg_page_sizes);
- return st;
+ return 0;
}
static void
@@ -520,19 +520,14 @@ __i915_gem_userptr_get_pages_worker(struct work_struct
*_work)
mutex_lock(&obj->mm.lock);
if (obj->userptr.work == &work->work) {
- struct sg_table *pages = ERR_PTR(ret);
-
if (pinned == npages) {
- pages = __i915_gem_userptr_alloc_pages(obj, pvec,
- npages);
- if (!IS_ERR(pages)) {
+ ret = __i915_gem_userptr_alloc_pages(obj, pvec, npages);
+ if (!ret)
pinned = 0;
- pages = NULL;
- }
}
- obj->userptr.work = ERR_CAST(pages);
- if (IS_ERR(pages))
+ obj->userptr.work = ERR_PTR(ret);
+ if (ret)
__i915_gem_userptr_set_active(obj, false);
}
mutex_unlock(&obj->mm.lock);
@@ -545,7 +540,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct
*_work)
kfree(work);
}
-static struct sg_table *
+static int
__i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
{
struct get_pages_work *work;
@@ -571,7 +566,7 @@ __i915_gem_userptr_get_pages_schedule(struct
drm_i915_gem_object *obj)
*/
work = kmalloc(sizeof(*work), GFP_KERNEL);
if (work == NULL)
- return ERR_PTR(-ENOMEM);
+ return -ENOMEM;
obj->userptr.work = &work->work;
@@ -583,19 +578,86 @@ __i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
INIT_WORK(&work->work, __i915_gem_userptr_get_pages_worker);
queue_work(to_i915(obj->base.dev)->mm.userptr_wq, &work->work);
- return ERR_PTR(-EAGAIN);
+ return -EAGAIN;
}
-static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
+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;
+
+ for (vma = find_vma(mm, addr); vma; vma = vma->vm_next) {
+ if (vma->vm_start > addr)
+ break;
+
+ /*
+ * Exclude any VMA that is not backed only by struct_page, i.e.
+ * IO regions that include our own GGTT mmaps. We cannot handle
+ * such ranges, as we may encounter deadlocks around our
+ * struct_mutex on mmu_invalidate_range.
+ */
+ if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))
+ break;
+
+ if (vma->vm_end >= end) {
+ ret = 0;
+ break;
+ }
+
+ addr = vma->vm_end;
+ }
+
+ return ret;
+}
+
+static int try_fast_gup(struct drm_i915_gem_object *obj)
{
const int num_pages = obj->base.size >> PAGE_SHIFT;
- struct mm_struct *mm = obj->userptr.mm->mm;
struct page **pvec;
- struct sg_table *pages;
- bool active;
- int pinned;
+ int pinned, err;
+
+ pvec = kvmalloc_array(num_pages, sizeof(struct page *),
+ GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
+ if (!pvec) /* defer to worker if malloc fails */
+ return -ENOMEM;
+
+ pinned = __get_user_pages_fast(obj->userptr.ptr,
+ num_pages,
+ !i915_gem_object_is_readonly(obj),
+ pvec);
+ if (pinned < 0) {
+ err = pinned;
+ pinned = 0;
+ goto out_pvec;
+ }
- /* If userspace should engineer that these pages are replaced in
+ if (pinned < num_pages) {
+ err = -EFAULT;
+ goto out_pinned;
+ }
+
+ err = __i915_gem_userptr_alloc_pages(obj, pvec, num_pages);
+ if (err)
+ goto out_pinned;
+
+ __i915_gem_userptr_set_active(obj, true);
+ pinned = 0;
+out_pinned:
+ release_pages(pvec, pinned);
+out_pvec:
+ kvfree(pvec);
+ return err;
+}
+
+static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
+{
+ struct mm_struct *mm = obj->userptr.mm->mm;
+ int err;
+
+ /*
+ * If userspace should engineer that these pages are replaced in
* the vma between us binding this page into the GTT and completion
* of rendering... Their loss. If they change the mapping of their
* pages they need to create a new bo to point to the new vma.
@@ -620,40 +682,24 @@ static int i915_gem_userptr_get_pages(struct
drm_i915_gem_object *obj)
return -EAGAIN;
}
- pvec = NULL;
- pinned = 0;
+ if (mm == current->mm && try_fast_gup(obj) == 0)
+ return 0;
- if (mm == current->mm) {
- pvec = kvmalloc_array(num_pages, sizeof(struct page *),
- GFP_KERNEL |
- __GFP_NORETRY |
- __GFP_NOWARN);
- if (pvec) /* defer to worker if malloc fails */
- pinned = __get_user_pages_fast(obj->userptr.ptr,
- num_pages,
-
!i915_gem_object_is_readonly(obj),
- pvec);
- }
+ /* lockdep doesn't yet automatically allow nesting of readers */
+ down_read_nested(&mm->mmap_sem, SINGLE_DEPTH_NESTING);
- active = false;
- if (pinned < 0) {
- pages = ERR_PTR(pinned);
- pinned = 0;
- } else if (pinned < num_pages) {
- pages = __i915_gem_userptr_get_pages_schedule(obj);
- active = pages == ERR_PTR(-EAGAIN);
- } else {
- pages = __i915_gem_userptr_alloc_pages(obj, pvec, num_pages);
- active = !IS_ERR(pages);
- }
- if (active)
+ err = probe_range(mm, obj->userptr.ptr, obj->base.size);
