[PATCH 27/28] drm/i915: Fix userptr so we do not have to worry about obj->mm.lock
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Mon Aug 3 08:46:55 UTC 2020
Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
---
drivers/gpu/drm/i915/display/intel_display.c | 2 +-
.../gpu/drm/i915/gem/i915_gem_execbuffer.c | 19 +-
drivers/gpu/drm/i915/gem/i915_gem_object.h | 11 +
.../gpu/drm/i915/gem/i915_gem_object_types.h | 7 +-
drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 691 ++++--------------
drivers/gpu/drm/i915/i915_drv.h | 9 +-
6 files changed, 185 insertions(+), 554 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 8e413dae4d78..b73c0268dacc 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -17105,7 +17105,7 @@ static int intel_user_framebuffer_create_handle(struct drm_framebuffer *fb,
struct drm_i915_gem_object *obj = intel_fb_obj(fb);
struct drm_i915_private *i915 = to_i915(obj->base.dev);
- if (obj->userptr.mm) {
+ if (i915_gem_object_is_userptr(obj)) {
drm_dbg(&i915->drm,
"attempting to use a userptr for a framebuffer, denied\n");
return -EINVAL;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index e50283cb3f4b..2a6e100a4465 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -884,6 +884,12 @@ static int eb_validate_vmas(struct i915_execbuffer *eb)
if (err)
return err;
+ if (i915_gem_object_is_userptr(vma->obj)) {
+ err = i915_gem_object_userptr_submit_begin(vma->obj);
+ if (err)
+ return err;
+ }
+
if (eb_pin_vma(eb, entry, ev)) {
if (entry->offset != vma->node.start) {
entry->offset = vma->node.start | UPDATE;
@@ -1934,9 +1940,6 @@ static noinline int eb_relocate_parse_slow(struct i915_execbuffer *eb,
err = 0;
}
- if (!err)
- flush_workqueue(eb->i915->mm.userptr_wq);
-
err_relock:
i915_gem_ww_ctx_init(&eb->ww, true);
if (err)
@@ -2123,6 +2126,10 @@ static int eb_move_to_gpu(struct i915_execbuffer *eb)
unsigned int i = count;
int err = 0;
+ err = mutex_lock_interruptible(&eb->i915->mm.notifier_lock);
+ if (err)
+ return err;
+
while (i--) {
struct eb_vma *ev = &eb->vma[i];
struct i915_vma *vma = ev->vma;
@@ -2142,6 +2149,9 @@ static int eb_move_to_gpu(struct i915_execbuffer *eb)
}
}
+ if (err == 0 && i915_gem_object_is_userptr(obj))
+ err = i915_gem_object_userptr_submit_done(obj);
+
/*
* If the GPU is not _reading_ through the CPU cache, we need
* to make sure that any writes (both previous GPU writes from
@@ -2168,6 +2178,8 @@ static int eb_move_to_gpu(struct i915_execbuffer *eb)
err = i915_vma_move_to_active(vma, eb->request, flags);
}
+ mutex_unlock(&eb->i915->mm.notifier_lock);
+
if (unlikely(err))
goto err_skip;
@@ -3115,6 +3127,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
trace_i915_request_queue(eb.request, eb.batch_flags);
err = eb_submit(&eb, batch);
+
err_request:
i915_request_get(eb.request);
eb_request_add(&eb);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index afde1952c119..11f28f9ada28 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -32,6 +32,8 @@ i915_gem_object_create_shmem_from_data(struct drm_i915_private *i915,
const void *data, resource_size_t size);
extern const struct drm_i915_gem_object_ops i915_gem_shmem_ops;
+extern const struct drm_i915_gem_object_ops i915_gem_userptr_ops;
+
void __i915_gem_object_release_shmem(struct drm_i915_gem_object *obj,
struct sg_table *pages,
bool needs_clflush);
@@ -521,4 +523,13 @@ i915_gem_object_invalidate_frontbuffer(struct drm_i915_gem_object *obj,
__i915_gem_object_invalidate_frontbuffer(obj, origin);
}
+static inline bool
+i915_gem_object_is_userptr(struct drm_i915_gem_object *obj)
+{
+ return obj->ops == &i915_gem_userptr_ops;
+}
+
+int i915_gem_object_userptr_submit_begin(struct drm_i915_gem_object *obj);
+int i915_gem_object_userptr_submit_done(struct drm_i915_gem_object *obj);
+
#endif
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index b5c15557cc87..46d0d82e0d32 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -7,6 +7,8 @@
#ifndef __I915_GEM_OBJECT_TYPES_H__
#define __I915_GEM_OBJECT_TYPES_H__
+#include <linux/mmu_notifier.h>
+
#include <drm/drm_gem.h>
#include <uapi/drm/i915_drm.h>
@@ -284,10 +286,9 @@ struct drm_i915_gem_object {
union {
struct i915_gem_userptr {
uintptr_t ptr;
+ unsigned long notifier_seq;
- struct i915_mm_struct *mm;
- struct i915_mmu_object *mmu_object;
- struct work_struct *work;
+ struct mmu_interval_notifier notifier;
} userptr;
unsigned long scratch;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index e946032b13e4..a4d3fe63a9a4 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -2,10 +2,39 @@
* SPDX-License-Identifier: MIT
*
* Copyright © 2012-2014 Intel Corporation
+ *
+ * Based on amdgpu_mn, which bears the following notice:
+ *
+ * Copyright 2014 Advanced Micro Devices, Inc.
+ * All Rights Reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sub license, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
+ * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+ * USE OR OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ */
+/*
+ * Authors:
+ * Christian König <christian.koenig at amd.com>
*/
#include <linux/mmu_context.h>
-#include <linux/mmu_notifier.h>
#include <linux/mempolicy.h>
#include <linux/swap.h>
#include <linux/sched/mm.h>
@@ -15,261 +44,66 @@
#include "i915_gem_object.h"
#include "i915_scatterlist.h"
-struct i915_mm_struct {
- struct mm_struct *mm;
- struct drm_i915_private *i915;
- struct i915_mmu_notifier *mn;
- struct hlist_node node;
- struct kref kref;
- struct rcu_work work;
-};
-
-#if defined(CONFIG_MMU_NOTIFIER)
-#include <linux/interval_tree.h>
+#ifdef CONFIG_MMU_NOTIFIER
-struct i915_mmu_notifier {
- spinlock_t lock;
- struct hlist_node node;
- struct mmu_notifier mn;
- struct rb_root_cached objects;
- struct i915_mm_struct *mm;
-};
-
-struct i915_mmu_object {
- struct i915_mmu_notifier *mn;
- struct drm_i915_gem_object *obj;
- struct interval_tree_node it;
-};
-
-static void add_object(struct i915_mmu_object *mo)
-{
- GEM_BUG_ON(!RB_EMPTY_NODE(&mo->it.rb));
- interval_tree_insert(&mo->it, &mo->mn->objects);
-}
-
-static void del_object(struct i915_mmu_object *mo)
-{
- if (RB_EMPTY_NODE(&mo->it.rb))
- return;
-
- interval_tree_remove(&mo->it, &mo->mn->objects);
- RB_CLEAR_NODE(&mo->it.rb);
-}
-
-static void
-__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value)
-{
- struct i915_mmu_object *mo = obj->userptr.mmu_object;
-
- /*
- * During mm_invalidate_range we need to cancel any userptr that
- * overlaps the range being invalidated. Doing so requires the
- * struct_mutex, and that risks recursion. In order to cause
- * recursion, the user must alias the userptr address space with
- * a GTT mmapping (possible with a MAP_FIXED) - then when we have
- * to invalidate that mmaping, mm_invalidate_range is called with
- * the userptr address *and* the struct_mutex held. To prevent that
- * we set a flag under the i915_mmu_notifier spinlock to indicate
- * whether this object is valid.
- */
- if (!mo)
- return;
-
- spin_lock(&mo->mn->lock);
- if (value)
- add_object(mo);
- else
- del_object(mo);
- spin_unlock(&mo->mn->lock);
-}
-
-static int
-userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
- const struct mmu_notifier_range *range)
+/**
+ * i915_gem_userptr_invalidate - callback to notify about mm change
+ *
+ * @mni: the range (mm) is about to update
+ * @range: details on the invalidation
+ * @cur_seq: Value to pass to mmu_interval_set_seq()
+ *
+ * Block for operations on BOs to finish and mark pages as accessed and
+ * potentially dirty.
+ */
+static bool i915_gem_userptr_invalidate(struct mmu_interval_notifier *mni,
+ const struct mmu_notifier_range *range,
+ unsigned long cur_seq)
{
- struct i915_mmu_notifier *mn =
- container_of(_mn, struct i915_mmu_notifier, mn);
- struct interval_tree_node *it;
- unsigned long end;
- int ret = 0;
-
- if (RB_EMPTY_ROOT(&mn->objects.rb_root))
- return 0;
-
- /* interval ranges are inclusive, but invalidate range is exclusive */
- end = range->end - 1;
-
- spin_lock(&mn->lock);
- it = interval_tree_iter_first(&mn->objects, range->start, end);
- while (it) {
- struct drm_i915_gem_object *obj;
+ 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)) {
- ret = -EAGAIN;
- break;
- }
+ if (!mmu_notifier_range_blockable(range))
+ return false;
- /*
- * The mmu_object is released late when destroying the
- * GEM object so it is entirely possible to gain a
- * reference on an object in the process of being freed
- * since our serialisation is via the spinlock and not
- * the struct_mutex - and consequently use it after it
- * is freed and then double free it. To prevent that
- * use-after-free we only acquire a reference on the
- * object if it is not in the process of being destroyed.
- */
- obj = container_of(it, struct i915_mmu_object, it)->obj;
- if (!kref_get_unless_zero(&obj->base.refcount)) {
- it = interval_tree_iter_next(it, range->start, end);
- continue;
- }
- spin_unlock(&mn->lock);
+ mutex_lock(&i915->mm.notifier_lock);
- ret = i915_gem_object_unbind(obj,
- I915_GEM_OBJECT_UNBIND_ACTIVE |
- I915_GEM_OBJECT_UNBIND_BARRIER);
- if (ret == 0)
- ret = __i915_gem_object_put_pages(obj);
- i915_gem_object_put(obj);
- if (ret)
- return ret;
+ mmu_interval_set_seq(mni, cur_seq);
- spin_lock(&mn->lock);
+ r = dma_resv_wait_timeout_rcu(obj->base.resv, true, false,
+ MAX_SCHEDULE_TIMEOUT);
+ if (r <= 0)
+ drm_err(&i915->drm, "(%ld) failed to wait for idle\n", r);
- /*
- * As we do not (yet) protect the mmu from concurrent insertion
- * over this range, there is no guarantee that this search will
- * terminate given a pathologic workload.
- */
- it = interval_tree_iter_first(&mn->objects, range->start, end);
+ r = i915_gem_object_unbind(obj, I915_GEM_OBJECT_UNBIND_ACTIVE);
+ mutex_unlock(&i915->mm.notifier_lock);
+ if (r < 0) {
+ drm_err(&i915->drm, "(%ld) failed to unbind\n", r);
+ return false;
}
- spin_unlock(&mn->lock);
-
- return ret;
+ return true;
}
-static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
- .invalidate_range_start = userptr_mn_invalidate_range_start,
+static const struct mmu_interval_notifier_ops i915_gem_userptr_notifier_ops = {
+ .invalidate = i915_gem_userptr_invalidate,
};
-static struct i915_mmu_notifier *
-i915_mmu_notifier_create(struct i915_mm_struct *mm)
-{
- struct i915_mmu_notifier *mn;
-
- mn = kmalloc(sizeof(*mn), GFP_KERNEL);
- if (mn == NULL)
- return ERR_PTR(-ENOMEM);
-
- spin_lock_init(&mn->lock);
- mn->mn.ops = &i915_gem_userptr_notifier;
- mn->objects = RB_ROOT_CACHED;
- mn->mm = mm;
-
- return mn;
-}
-
-static void
-i915_gem_userptr_release__mmu_notifier(struct drm_i915_gem_object *obj)
-{
- struct i915_mmu_object *mo;
-
- mo = fetch_and_zero(&obj->userptr.mmu_object);
- if (!mo)
- return;
-
- spin_lock(&mo->mn->lock);
- del_object(mo);
- spin_unlock(&mo->mn->lock);
- kfree(mo);
-}
-
-static struct i915_mmu_notifier *
-i915_mmu_notifier_find(struct i915_mm_struct *mm)
-{
- struct i915_mmu_notifier *mn, *old;
- int err;
-
- mn = READ_ONCE(mm->mn);
- if (likely(mn))
- return mn;
-
- mn = i915_mmu_notifier_create(mm);
- if (IS_ERR(mn))
- return mn;
-
- err = mmu_notifier_register(&mn->mn, mm->mm);
- if (err) {
- kfree(mn);
- return ERR_PTR(err);
- }
-
- old = cmpxchg(&mm->mn, NULL, mn);
- if (old) {
- mmu_notifier_unregister(&mn->mn, mm->mm);
- kfree(mn);
- mn = old;
- }
-
- return mn;
-}
-
static int
i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj,
unsigned flags)
{
- struct i915_mmu_notifier *mn;
- struct i915_mmu_object *mo;
-
if (flags & I915_USERPTR_UNSYNCHRONIZED)
return capable(CAP_SYS_ADMIN) ? 0 : -EPERM;
- if (GEM_WARN_ON(!obj->userptr.mm))
- return -EINVAL;
-
- mn = i915_mmu_notifier_find(obj->userptr.mm);
- if (IS_ERR(mn))
- return PTR_ERR(mn);
-
- mo = kzalloc(sizeof(*mo), GFP_KERNEL);
- if (!mo)
- return -ENOMEM;
-
- mo->mn = mn;
- mo->obj = obj;
- mo->it.start = obj->userptr.ptr;
- mo->it.last = obj->userptr.ptr + obj->base.size - 1;
- RB_CLEAR_NODE(&mo->it.rb);
-
- obj->userptr.mmu_object = mo;
- return 0;
-}
-
-static void
-i915_mmu_notifier_free(struct i915_mmu_notifier *mn,
- struct mm_struct *mm)
-{
- if (mn == NULL)
- return;
-
- mmu_notifier_unregister(&mn->mn, mm);
- kfree(mn);
+ return mmu_interval_notifier_insert(&obj->userptr.notifier, current->mm,
+ obj->userptr.ptr, obj->base.size,
+ &i915_gem_userptr_notifier_ops);
}
#else
-static void
-__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value)
-{
-}
-
-static void
-i915_gem_userptr_release__mmu_notifier(struct drm_i915_gem_object *obj)
-{
-}
-
static int
i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj,
unsigned flags)
@@ -283,119 +117,8 @@ i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj,
return 0;
}
-static void
-i915_mmu_notifier_free(struct i915_mmu_notifier *mn,
- struct mm_struct *mm)
-{
-}
-
#endif
-static struct i915_mm_struct *
-__i915_mm_struct_find(struct drm_i915_private *i915, struct mm_struct *real)
-{
- struct i915_mm_struct *it, *mm = NULL;
-
- rcu_read_lock();
- hash_for_each_possible_rcu(i915->mm_structs,
- it, node,
- (unsigned long)real)
- if (it->mm == real && kref_get_unless_zero(&it->kref)) {
- mm = it;
- break;
- }
- rcu_read_unlock();
-
- return mm;
-}
-
-static int
-i915_gem_userptr_init__mm_struct(struct drm_i915_gem_object *obj)
-{
- struct drm_i915_private *i915 = to_i915(obj->base.dev);
- struct i915_mm_struct *mm, *new;
- int ret = 0;
-
- /* During release of the GEM object we hold the struct_mutex. This
- * precludes us from calling mmput() at that time as that may be
- * the last reference and so call exit_mmap(). exit_mmap() will
- * attempt to reap the vma, and if we were holding a GTT mmap
- * would then call drm_gem_vm_close() and attempt to reacquire
- * the struct mutex. So in order to avoid that recursion, we have
- * to defer releasing the mm reference until after we drop the
- * struct_mutex, i.e. we need to schedule a worker to do the clean
- * up.
- */
- mm = __i915_mm_struct_find(i915, current->mm);
- if (mm)
- goto out;
-
- new = kmalloc(sizeof(*mm), GFP_KERNEL);
- if (!new)
- return -ENOMEM;
-
- kref_init(&new->kref);
- new->i915 = to_i915(obj->base.dev);
- new->mm = current->mm;
- new->mn = NULL;
-
- spin_lock(&i915->mm_lock);
- mm = __i915_mm_struct_find(i915, current->mm);
- if (!mm) {
- hash_add_rcu(i915->mm_structs,
- &new->node,
- (unsigned long)new->mm);
- mmgrab(current->mm);
- mm = new;
- }
- spin_unlock(&i915->mm_lock);
- if (mm != new)
- kfree(new);
-
-out:
- obj->userptr.mm = mm;
- return ret;
-}
-
-static void
-__i915_mm_struct_free__worker(struct work_struct *work)
-{
- struct i915_mm_struct *mm = container_of(work, typeof(*mm), work.work);
-
- i915_mmu_notifier_free(mm->mn, mm->mm);
- mmdrop(mm->mm);
- kfree(mm);
-}
-
-static void
-__i915_mm_struct_free(struct kref *kref)
-{
- struct i915_mm_struct *mm = container_of(kref, typeof(*mm), kref);
-
- spin_lock(&mm->i915->mm_lock);
- hash_del_rcu(&mm->node);
- spin_unlock(&mm->i915->mm_lock);
-
- INIT_RCU_WORK(&mm->work, __i915_mm_struct_free__worker);
- queue_rcu_work(system_wq, &mm->work);
-}
-
-static void
-i915_gem_userptr_release__mm_struct(struct drm_i915_gem_object *obj)
-{
- if (obj->userptr.mm == NULL)
- return;
-
- kref_put(&obj->userptr.mm->kref, __i915_mm_struct_free);
- obj->userptr.mm = NULL;
-}
-
-struct get_pages_work {
- struct work_struct work;
- struct drm_i915_gem_object *obj;
- struct task_struct *task;
-};
-
static struct sg_table *
__i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
struct page **pvec, unsigned long num_pages)
@@ -439,200 +162,53 @@ __i915_gem_userptr_alloc_pages(struct drm_i915_gem_object *obj,
return st;
}
-static void
-__i915_gem_userptr_get_pages_worker(struct work_struct *_work)
-{
- struct get_pages_work *work = container_of(_work, typeof(*work), work);
- struct drm_i915_gem_object *obj = work->obj;
- const unsigned long npages = obj->base.size >> PAGE_SHIFT;
- unsigned long pinned;
- struct page **pvec;
- int ret;
-
- ret = -ENOMEM;
- pinned = 0;
-
- pvec = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
- if (pvec != NULL) {
- struct mm_struct *mm = obj->userptr.mm->mm;
- unsigned int flags = 0;
- int locked = 0;
-
- if (!i915_gem_object_is_readonly(obj))
- flags |= FOLL_WRITE;
-
- ret = -EFAULT;
- if (mmget_not_zero(mm)) {
- while (pinned < npages) {
- if (!locked) {
- mmap_read_lock(mm);
- locked = 1;
- }
- ret = pin_user_pages_remote
- (work->task, mm,
- obj->userptr.ptr + pinned * PAGE_SIZE,
- npages - pinned,
- flags,
- pvec + pinned, NULL, &locked);
- if (ret < 0)
- break;
-
- pinned += ret;
- }
- if (locked)
- mmap_read_unlock(mm);
- mmput(mm);
- }
- }
-
- mutex_lock_nested(&obj->mm.lock, I915_MM_GET_PAGES);
- 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)) {
- pinned = 0;
- pages = NULL;
- }
- }
-
- obj->userptr.work = ERR_CAST(pages);
- if (IS_ERR(pages))
- __i915_gem_userptr_set_active(obj, false);
- }
- mutex_unlock(&obj->mm.lock);
-
- unpin_user_pages(pvec, pinned);
- kvfree(pvec);
-
- i915_gem_object_put(obj);
- put_task_struct(work->task);
- kfree(work);
-}
-
-static struct sg_table *
-__i915_gem_userptr_get_pages_schedule(struct drm_i915_gem_object *obj)
-{
- struct get_pages_work *work;
-
- /* Spawn a worker so that we can acquire the
- * user pages without holding our mutex. Access
- * to the user pages requires mmap_lock, and we have
- * a strict lock ordering of mmap_lock, struct_mutex -
- * we already hold struct_mutex here and so cannot
- * call gup without encountering a lock inversion.
- *
- * Userspace will keep on repeating the operation
- * (thanks to EAGAIN) until either we hit the fast
- * path or the worker completes. If the worker is
- * cancelled or superseded, the task is still run
- * but the results ignored. (This leads to
- * complications that we may have a stray object
- * refcount that we need to be wary of when
- * checking for existing objects during creation.)
- * If the worker encounters an error, it reports
- * that error back to this function through
- * obj->userptr.work = ERR_PTR.
- */
- work = kmalloc(sizeof(*work), GFP_KERNEL);
- if (work == NULL)
- return ERR_PTR(-ENOMEM);
-
- obj->userptr.work = &work->work;
-
- work->obj = i915_gem_object_get(obj);
-
- work->task = current;
- get_task_struct(work->task);
-
- 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);
-}
-
static int i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
{
const unsigned long 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;
unsigned int gup_flags = 0;
+ int pinned, ret;
- /* 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.
- *
- * However, that still leaves open the possibility of the vma
- * being copied upon fork. Which falls under the same userspace
- * synchronisation issue as a regular bo, except that this time
- * the process may not be expecting that a particular piece of
- * memory is tied to the GPU.
- *
- * Fortunately, we can hook into the mmu_notifier in order to
- * discard the page references prior to anything nasty happening
- * to the vma (discard or cloning) which should prevent the more
- * egregious cases from causing harm.
- */
+ if (obj->userptr.notifier.mm && obj->userptr.notifier.mm != current->mm)
+ return -EFAULT;
- if (obj->userptr.work) {
- /* active flag should still be held for the pending work */
- if (IS_ERR(obj->userptr.work))
- return PTR_ERR(obj->userptr.work);
- else
- return -EAGAIN;
- }
+ pvec = kvmalloc_array(num_pages, sizeof(struct page *), GFP_KERNEL);
+ if (!pvec)
+ return -ENOMEM;
- pvec = NULL;
- pinned = 0;
+ /*
+ * Using __get_user_pages_fast() with a read-only
+ * access is questionable. A read-only page may be
+ * COW-broken, and then this might end up giving
+ * the wrong side of the COW..
+ *
+ * We may or may not care.
+ */
+ if (!i915_gem_object_is_readonly(obj))
+ gup_flags |= FOLL_WRITE;
+
+ pinned = ret = 0;
+ while (pinned < num_pages) {
+ ret = pin_user_pages_fast(obj->userptr.ptr + pinned * PAGE_SIZE,
+ num_pages - pinned, gup_flags,
+ &pvec[pinned]);
+ if (ret < 0)
+ break;
- if (mm == current->mm) {
- pvec = kvmalloc_array(num_pages, sizeof(struct page *),
- GFP_KERNEL |
- __GFP_NORETRY |
- __GFP_NOWARN);
- /*
- * Using __get_user_pages_fast() with a read-only
- * access is questionable. A read-only page may be
- * COW-broken, and then this might end up giving
- * the wrong side of the COW..
- *
- * We may or may not care.
- */
- if (pvec) {
- /* defer to worker if malloc fails */
- if (!i915_gem_object_is_readonly(obj))
- gup_flags |= FOLL_WRITE;
- pinned = pin_user_pages_fast_only(obj->userptr.ptr,
- num_pages, gup_flags,
- pvec);
- }
+ pinned += ret;
}
- 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 {
+ if (ret >= 0) {
pages = __i915_gem_userptr_alloc_pages(obj, pvec, num_pages);
- active = !IS_ERR(pages);
+ ret = PTR_ERR_OR_ZERO(pages);
}
- if (active)
- __i915_gem_userptr_set_active(obj, true);
- if (IS_ERR(pages))
+ if (ret < 0 && pinned > 0)
unpin_user_pages(pvec, pinned);
kvfree(pvec);
- return PTR_ERR_OR_ZERO(pages);
+ return ret;
}
static void
@@ -642,9 +218,6 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
struct sgt_iter sgt_iter;
struct page *page;
- /* Cancel any inflight work and force them to restart their gup */
- obj->userptr.work = NULL;
- __i915_gem_userptr_set_active(obj, false);
if (!pages)
return;
@@ -692,23 +265,68 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
kfree(pages);
}
+int i915_gem_object_userptr_submit_begin(struct drm_i915_gem_object *obj)
+{
+ struct sg_table *pages;
+ int err;
+
+ err = i915_gem_object_unbind(obj, I915_GEM_OBJECT_UNBIND_ACTIVE);
+ if (err)
+ return err;
+
+ if (GEM_WARN_ON(i915_gem_object_has_pinned_pages(obj)))
+ return -EBUSY;
+
+ mutex_lock_nested(&obj->mm.lock, I915_MM_GET_PAGES);
+
+ pages = __i915_gem_object_unset_pages(obj);
+ if (!IS_ERR_OR_NULL(pages))
+ i915_gem_userptr_put_pages(obj, pages);
+
+ if (obj->userptr.notifier.mm)
+ obj->userptr.notifier_seq = mmu_interval_read_begin(&obj->userptr.notifier);
+
+ err = ____i915_gem_object_get_pages(obj);
+ mutex_unlock(&obj->mm.lock);
+
+ return err;
+}
+
+int i915_gem_object_userptr_submit_done(struct drm_i915_gem_object *obj)
+{
+ if (obj->userptr.notifier.mm &&
+ mmu_interval_read_retry(&obj->userptr.notifier,
+ obj->userptr.notifier_seq)) {
+
+ /* We collided with the mmu notifier, need to retry */
+
+ return -EAGAIN;
+ }
+
+ return 0;
+}
+
static void
i915_gem_userptr_release(struct drm_i915_gem_object *obj)
{
- i915_gem_userptr_release__mmu_notifier(obj);
- i915_gem_userptr_release__mm_struct(obj);
+ if (!obj->userptr.notifier.mm)
+ return;
+
+ mmu_interval_notifier_remove(&obj->userptr.notifier);
+ obj->userptr.notifier.mm = NULL;
}
static int
i915_gem_userptr_dmabuf_export(struct drm_i915_gem_object *obj)
{
- if (obj->userptr.mmu_object)
- return 0;
+ /* Needs to be created with an interval notifier */
+ if (!obj->userptr.notifier.mm)
+ return -EPERM;
- return i915_gem_userptr_init__mmu_notifier(obj, 0);
+ return 0;
}
-static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
+const struct drm_i915_gem_object_ops i915_gem_userptr_ops = {
.name = "i915_gem_object_userptr",
.flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE |
I915_GEM_OBJECT_IS_SHRINKABLE |
@@ -824,6 +442,7 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
obj->userptr.ptr = args->user_ptr;
+ obj->userptr.notifier_seq = ULONG_MAX;
if (args->flags & I915_USERPTR_READ_ONLY)
i915_gem_object_set_readonly(obj);
@@ -831,9 +450,7 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
* at binding. This means that we need to hook into the mmu_notifier
* in order to detect if the mmu is destroyed.
*/
- ret = i915_gem_userptr_init__mm_struct(obj);
- if (ret == 0)
- ret = i915_gem_userptr_init__mmu_notifier(obj, args->flags);
+ ret = i915_gem_userptr_init__mmu_notifier(obj, args->flags);
if (ret == 0)
ret = drm_gem_handle_create(file, &obj->base, &handle);
@@ -848,20 +465,12 @@ i915_gem_userptr_ioctl(struct drm_device *dev,
int i915_gem_init_userptr(struct drm_i915_private *dev_priv)
{
- spin_lock_init(&dev_priv->mm_lock);
- hash_init(dev_priv->mm_structs);
-
- dev_priv->mm.userptr_wq =
- alloc_workqueue("i915-userptr-acquire",
- WQ_HIGHPRI | WQ_UNBOUND,
- 0);
- if (!dev_priv->mm.userptr_wq)
- return -ENOMEM;
+ mutex_init(&dev_priv->mm.notifier_lock);
return 0;
}
void i915_gem_cleanup_userptr(struct drm_i915_private *dev_priv)
{
- destroy_workqueue(dev_priv->mm.userptr_wq);
+ mutex_destroy(&dev_priv->mm.notifier_lock);
}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4fc14923e5a8..d856edfde337 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -598,11 +598,10 @@ struct i915_gem_mm {
struct shrinker shrinker;
/**
- * Workqueue to fault in userptr pages, flushed by the execbuf
- * when required but otherwise left to userspace to try again
- * on EAGAIN.
+ * notifier_lock for mmu notifiers, memory may not be allocated
+ * while holding this lock.
*/
- struct workqueue_struct *userptr_wq;
+ struct mutex notifier_lock;
/* shrinker accounting, also useful for userland debugging */
u64 shrink_memory;
@@ -984,8 +983,6 @@ struct drm_i915_private {
struct i915_ggtt ggtt; /* VM representing the global address space */
struct i915_gem_mm mm;
- DECLARE_HASHTABLE(mm_structs, 7);
- spinlock_t mm_lock;
/* Kernel Modesetting */
--
2.28.0.rc2
More information about the Intel-gfx-trybot
mailing list