On Wed, Jul 06, 2022 at 06:21:03PM +0200, Thomas Hellström wrote:
On Fri, 2022-07-01 at 15:50 -0700, Niranjana Vishwanathapura wrote:
Bind and unbind the mappings upon VM_BIND and VM_UNBIND calls.
Signed-off-by: Niranjana Vishwanathapura
<niranjana.vishwanathapura@xxxxxxxxx>
Signed-off-by: Prathap Kumar Valsan <prathap.kumar.valsan@xxxxxxxxx>
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/gem/i915_gem_create.c | 10 +-
drivers/gpu/drm/i915/gem/i915_gem_object.h | 2 +
drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h | 38 +++
.../drm/i915/gem/i915_gem_vm_bind_object.c | 233
++++++++++++++++++
drivers/gpu/drm/i915/gt/intel_gtt.c | 7 +
drivers/gpu/drm/i915/gt/intel_gtt.h | 9 +
drivers/gpu/drm/i915/i915_driver.c | 11 +-
drivers/gpu/drm/i915/i915_vma.c | 7 +-
drivers/gpu/drm/i915/i915_vma.h | 2 -
drivers/gpu/drm/i915/i915_vma_types.h | 8 +
11 files changed, 318 insertions(+), 10 deletions(-)
create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
create mode 100644
drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
diff --git a/drivers/gpu/drm/i915/Makefile
b/drivers/gpu/drm/i915/Makefile
index 522ef9b4aff3..4e1627e96c6e 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -165,6 +165,7 @@ gem-y += \
gem/i915_gem_ttm_move.o \
gem/i915_gem_ttm_pm.o \
gem/i915_gem_userptr.o \
+ gem/i915_gem_vm_bind_object.o \
gem/i915_gem_wait.o \
gem/i915_gemfs.o
i915-y += \
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c
b/drivers/gpu/drm/i915/gem/i915_gem_create.c
index 33673fe7ee0a..927a87e5ec59 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
@@ -15,10 +15,10 @@
#include "i915_trace.h"
#include "i915_user_extensions.h"
-static u32 object_max_page_size(struct intel_memory_region
**placements,
- unsigned int n_placements)
+u32 i915_gem_object_max_page_size(struct intel_memory_region
**placements,
Kerneldoc.
This is an existing function that is being modified. As I
mentioned in other thread, we probably need a prep patch early
in this series to add missing kernel-docs in i915 which this
patch series would later update.
+ unsigned int n_placements)
{
- u32 max_page_size = 0;
+ u32 max_page_size = I915_GTT_PAGE_SIZE_4K;
int i;
for (i = 0; i < n_placements; i++) {
@@ -28,7 +28,6 @@ static u32 object_max_page_size(struct
intel_memory_region **placements,
max_page_size = max_t(u32, max_page_size, mr-
>min_page_size);
}
- GEM_BUG_ON(!max_page_size);
return max_page_size;
}
Should this change be separated out? It's not immediately clear to a
reviewer why it is included.
It is being removed as max_page_size now has a non-zero default
value and hence this check is not valid anymore.
@@ -99,7 +98,8 @@ __i915_gem_object_create_user_ext(struct
drm_i915_private *i915, u64 size,
i915_gem_flush_free_objects(i915);
- size = round_up(size, object_max_page_size(placements,
n_placements));
+ size = round_up(size,
i915_gem_object_max_page_size(placements,
+
n_placements));
if (size == 0)
return ERR_PTR(-EINVAL);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 6f0a3ce35567..650de2224843 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -47,6 +47,8 @@ static inline bool i915_gem_object_size_2big(u64
size)
}
void i915_gem_init__objects(struct drm_i915_private *i915);
+u32 i915_gem_object_max_page_size(struct intel_memory_region
**placements,
+ unsigned int n_placements);
void i915_objects_module_exit(void);
int i915_objects_module_init(void);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
new file mode 100644
index 000000000000..642cdb559f17
--- /dev/null
+++ b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#ifndef __I915_GEM_VM_BIND_H
+#define __I915_GEM_VM_BIND_H
+
+#include "i915_drv.h"
+
+#define assert_vm_bind_held(vm) lockdep_assert_held(&(vm)-
>vm_bind_lock)
+
+static inline void i915_gem_vm_bind_lock(struct i915_address_space
*vm)
+{
+ mutex_lock(&vm->vm_bind_lock);
+}
+
+static inline int
+i915_gem_vm_bind_lock_interruptible(struct i915_address_space *vm)
+{
+ return mutex_lock_interruptible(&vm->vm_bind_lock);
+}
+
+static inline void i915_gem_vm_bind_unlock(struct i915_address_space
*vm)
+{
+ mutex_unlock(&vm->vm_bind_lock);
+}
+
Kerneldoc for the inlines.
+struct i915_vma *
+i915_gem_vm_bind_lookup_vma(struct i915_address_space *vm, u64 va);
+void i915_gem_vm_bind_remove(struct i915_vma *vma, bool
release_obj);
+int i915_gem_vm_bind_obj(struct i915_address_space *vm,
+ struct drm_i915_gem_vm_bind *va,
+ struct drm_file *file);
+int i915_gem_vm_unbind_obj(struct i915_address_space *vm,
+ struct drm_i915_gem_vm_unbind *va);
+
+#endif /* __I915_GEM_VM_BIND_H */
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
new file mode 100644
index 000000000000..43ceb4dcca6c
--- /dev/null
+++ b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
@@ -0,0 +1,233 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include <linux/interval_tree_generic.h>
+
+#include "gem/i915_gem_vm_bind.h"
+#include "gt/gen8_engine_cs.h"
+
+#include "i915_drv.h"
+#include "i915_gem_gtt.h"
+
+#define START(node) ((node)->start)
+#define LAST(node) ((node)->last)
+
+INTERVAL_TREE_DEFINE(struct i915_vma, rb, u64, __subtree_last,
+ START, LAST, static inline, i915_vm_bind_it)
+
+#undef START
+#undef LAST
+
+/**
+ * DOC: VM_BIND/UNBIND ioctls
+ *
+ * DRM_I915_GEM_VM_BIND/UNBIND ioctls allows UMD to bind/unbind GEM
buffer
+ * objects (BOs) or sections of a BOs at specified GPU virtual
addresses on a
+ * specified address space (VM). Multiple mappings can map to the
same physical
+ * pages of an object (aliasing). These mappings (also referred to
as persistent
+ * mappings) will be persistent across multiple GPU submissions
(execbuf calls)
+ * issued by the UMD, without user having to provide a list of all
required
+ * mappings during each submission (as required by older execbuf
mode).
+ *
+ * The VM_BIND/UNBIND calls allow UMDs to request a timeline out
fence for
+ * signaling the completion of bind/unbind operation.
+ *
+ * VM_BIND feature is advertised to user via
I915_PARAM_VM_BIND_VERSION.
+ * User has to opt-in for VM_BIND mode of binding for an address
space (VM)
+ * during VM creation time via I915_VM_CREATE_FLAGS_USE_VM_BIND
extension.
+ *
+ * VM_BIND/UNBIND ioctl calls executed on different CPU threads
concurrently
+ * are not ordered. Furthermore, parts of the VM_BIND/UNBIND
operations can be
+ * done asynchronously, when valid out fence is specified.
+ *
+ * VM_BIND locking order is as below.
+ *
+ * 1) Lock-A: A vm_bind mutex will protect vm_bind lists. This lock
is taken in
+ * vm_bind/vm_unbind ioctl calls, in the execbuf path and while
releasing the
+ * mapping.
+ *
+ * In future, when GPU page faults are supported, we can
potentially use a
+ * rwsem instead, so that multiple page fault handlers can take
the read
+ * side lock to lookup the mapping and hence can run in parallel.
+ * The older execbuf mode of binding do not need this lock.
+ *
+ * 2) Lock-B: The object's dma-resv lock will protect i915_vma state
and needs
+ * to be held while binding/unbinding a vma in the async worker
and while
+ * updating dma-resv fence list of an object. Note that private
BOs of a VM
+ * will all share a dma-resv object.
+ *
+ * The future system allocator support will use the HMM
prescribed locking
+ * instead.
I don't think the last sentence is relevant for this series. Also, are
there any other mentions for Locks A, B and C? If not, can we ditch
that naming?
It is taken from design rfc :). Yah, I think better to remove it and
probably the lock names and make it more specific to the implementation
in this patch series.
+ *
+ * 3) Lock-C: Spinlock/s to protect some of the VM's lists like the
list of
+ * invalidated vmas (due to eviction and userptr invalidation)
etc.
+ */
+
+struct i915_vma *
+i915_gem_vm_bind_lookup_vma(struct i915_address_space *vm, u64 va)
Kerneldoc for the extern functions.
+{
+ struct i915_vma *vma, *temp;
+
+ assert_vm_bind_held(vm);
+
+ vma = i915_vm_bind_it_iter_first(&vm->va, va, va);
+ /* Working around compiler error, remove later */
Is this still relevant? What compiler error is seen here?
+ if (vma)
+ temp = i915_vm_bind_it_iter_next(vma, va + vma->size,
-1);
+ return vma;
+}
+
+void i915_gem_vm_bind_remove(struct i915_vma *vma, bool release_obj)
+{
+ assert_vm_bind_held(vma->vm);
+
+ if (!list_empty(&vma->vm_bind_link)) {
+ list_del_init(&vma->vm_bind_link);
+ i915_vm_bind_it_remove(vma, &vma->vm->va);
+
+ /* Release object */
+ if (release_obj)
+ i915_vma_put(vma);
i915_vma_put() here is confusing. Can we use i915_gem_object_put() to
further make it clear that the persistent vmas actually take a
reference on the object?
makes sense.
+ }
+}
+
+int i915_gem_vm_unbind_obj(struct i915_address_space *vm,
+ struct drm_i915_gem_vm_unbind *va)
+{
+ struct drm_i915_gem_object *obj;
+ struct i915_vma *vma;
+ int ret;
+
+ va->start = gen8_noncanonical_addr(va->start);
+ ret = i915_gem_vm_bind_lock_interruptible(vm);
+ if (ret)
+ return ret;
+
+ vma = i915_gem_vm_bind_lookup_vma(vm, va->start);
+ if (!vma) {
+ ret = -ENOENT;
+ goto out_unlock;
+ }
+
+ if (vma->size != va->length)
+ ret = -EINVAL;
+ else
+ i915_gem_vm_bind_remove(vma, false);
+
+out_unlock:
+ i915_gem_vm_bind_unlock(vm);
+ if (ret || !vma)
+ return ret;
+
+ /* Destroy vma and then release object */
+ obj = vma->obj;
+ ret = i915_gem_object_lock(obj, NULL);
+ if (ret)
+ return ret;
This call never returns an error and we could GEM_WARN_ON(...), or
(void) to annotate that the return value is wilfully ignored.
makes sense.
+
+ i915_vma_destroy(vma);
+ i915_gem_object_unlock(obj);
+ i915_gem_object_put(obj);
+
+ return 0;
+}
+
+static struct i915_vma *vm_bind_get_vma(struct i915_address_space
*vm,
+ struct drm_i915_gem_object
*obj,
+ struct drm_i915_gem_vm_bind
*va)
+{
+ struct i915_ggtt_view view;
+ struct i915_vma *vma;
+
+ va->start = gen8_noncanonical_addr(va->start);
+ vma = i915_gem_vm_bind_lookup_vma(vm, va->start);
+ if (vma)
+ return ERR_PTR(-EEXIST);
+
+ view.type = I915_GGTT_VIEW_PARTIAL;
+ view.partial.offset = va->offset >> PAGE_SHIFT;
+ view.partial.size = va->length >> PAGE_SHIFT;
IIRC, this vma view is not handled correctly in the vma code, that only
understands views for ggtt bindings.
This patch series extends the partial view to ppgtt also.
Yah, the naming is still i915_ggtt_view, but I am hoping we can fix the
name in a follow up patch later.
+ vma = i915_vma_instance(obj, vm, &view);
+ if (IS_ERR(vma))
+ return vma;
+
+ vma->start = va->start;
+ vma->last = va->start + va->length - 1;
+
+ return vma;
+}
+
+int i915_gem_vm_bind_obj(struct i915_address_space *vm,
+ struct drm_i915_gem_vm_bind *va,
+ struct drm_file *file)
+{
+ struct drm_i915_gem_object *obj;
+ struct i915_vma *vma = NULL;
+ struct i915_gem_ww_ctx ww;
+ u64 pin_flags;
+ int ret = 0;
+
+ if (!vm->vm_bind_mode)
+ return -EOPNOTSUPP;
+
+ obj = i915_gem_object_lookup(file, va->handle);
+ if (!obj)
+ return -ENOENT;
+
+ if (!va->length ||
+ !IS_ALIGNED(va->offset | va->length,
+ i915_gem_object_max_page_size(obj-
>mm.placements,
+ obj-
>mm.n_placements)) ||
+ range_overflows_t(u64, va->offset, va->length, obj-
>base.size)) {
+ ret = -EINVAL;
+ goto put_obj;
+ }
+
+ ret = i915_gem_vm_bind_lock_interruptible(vm);
+ if (ret)
+ goto put_obj;
+
+ vma = vm_bind_get_vma(vm, obj, va);
+ if (IS_ERR(vma)) {
+ ret = PTR_ERR(vma);
+ goto unlock_vm;
+ }
+
+ i915_gem_ww_ctx_init(&ww, true);
+ pin_flags = va->start | PIN_OFFSET_FIXED | PIN_USER;
+retry:
+ ret = i915_gem_object_lock(vma->obj, &ww);
+ if (ret)
+ goto out_ww;
+
+ ret = i915_vma_pin_ww(vma, &ww, 0, 0, pin_flags);
+ if (ret)
+ goto out_ww;
+
+ /* Make it evictable */
+ __i915_vma_unpin(vma);
A considerable effort has been put into avoiding short term vma pins in
i915. We should add an interface like i915_vma_bind_ww() that avoids
the pin altoghether.
Currently in i915 driver VA managment and device page table bindings
are tightly coupled. i915_vma_pin_ww() does the both VA allocation and
biding. And we also interpret VA being allocated (drm_mm node allocated)
also as vma is bound.
Decoupling it would be ideal but I think it needs to be carefully done
in a separate patch series to not cause any regression.
+
+ list_add_tail(&vma->vm_bind_link, &vm->vm_bound_list);
+ i915_vm_bind_it_insert(vma, &vm->va);
+
+ /* Hold object reference until vm_unbind */
+ i915_gem_object_get(vma->obj);
+out_ww:
+ if (ret == -EDEADLK) {
+ ret = i915_gem_ww_ctx_backoff(&ww);
+ if (!ret)
+ goto retry;
+ }
+
+ if (ret)
+ i915_vma_destroy(vma);
+
+ i915_gem_ww_ctx_fini(&ww);
Could use for_i915_gem_ww()?
Yah, I think it is a better idea to use it.
+unlock_vm:
+ i915_gem_vm_bind_unlock(vm);
+put_obj:
+ i915_gem_object_put(obj);
+ return ret;
+}
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c
b/drivers/gpu/drm/i915/gt/intel_gtt.c
index b67831833c9a..135dc4a76724 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -176,6 +176,8 @@ int i915_vm_lock_objects(struct
i915_address_space *vm,
void i915_address_space_fini(struct i915_address_space *vm)
{
drm_mm_takedown(&vm->mm);
+ GEM_BUG_ON(!RB_EMPTY_ROOT(&vm->va.rb_root));
+ mutex_destroy(&vm->vm_bind_lock);
}
/**
@@ -282,6 +284,11 @@ void i915_address_space_init(struct
i915_address_space *vm, int subclass)
INIT_LIST_HEAD(&vm->bound_list);
INIT_LIST_HEAD(&vm->unbound_list);
+
+ vm->va = RB_ROOT_CACHED;
+ INIT_LIST_HEAD(&vm->vm_bind_list);
+ INIT_LIST_HEAD(&vm->vm_bound_list);
+ mutex_init(&vm->vm_bind_lock);
}
void *__px_vaddr(struct drm_i915_gem_object *p)
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h
b/drivers/gpu/drm/i915/gt/intel_gtt.h
index c812aa9708ae..d4a6ce65251d 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
@@ -259,6 +259,15 @@ struct i915_address_space {
*/
struct list_head unbound_list;
+ /**
+ * List of VM_BIND objects.
+ */
Proper kerneldoc + intel locking guidelines comments, please.
+ struct mutex vm_bind_lock; /* Protects vm_bind lists */
+ struct list_head vm_bind_list;
+ struct list_head vm_bound_list;
+ /* va tree of persistent vmas */
+ struct rb_root_cached va;
+
/* Global GTT */
bool is_ggtt:1;
diff --git a/drivers/gpu/drm/i915/i915_driver.c
b/drivers/gpu/drm/i915/i915_driver.c
index ccf990dfd99b..776ab7844f60 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -68,6 +68,7 @@
#include "gem/i915_gem_ioctls.h"
#include "gem/i915_gem_mman.h"
#include "gem/i915_gem_pm.h"
+#include "gem/i915_gem_vm_bind.h"
#include "gt/intel_gt.h"
#include "gt/intel_gt_pm.h"
#include "gt/intel_rc6.h"
@@ -1783,13 +1784,16 @@ static int i915_gem_vm_bind_ioctl(struct
drm_device *dev, void *data,
{
struct drm_i915_gem_vm_bind *args = data;
struct i915_address_space *vm;
+ int ret;
vm = i915_gem_vm_lookup(file->driver_priv, args->vm_id);
if (unlikely(!vm))
return -ENOENT;
+ ret = i915_gem_vm_bind_obj(vm, args, file);
+
i915_vm_put(vm);
- return -EINVAL;
+ return ret;
}
static int i915_gem_vm_unbind_ioctl(struct drm_device *dev, void
*data,
@@ -1797,13 +1801,16 @@ static int i915_gem_vm_unbind_ioctl(struct
drm_device *dev, void *data,
{
struct drm_i915_gem_vm_unbind *args = data;
struct i915_address_space *vm;
+ int ret;
vm = i915_gem_vm_lookup(file->driver_priv, args->vm_id);
if (unlikely(!vm))
return -ENOENT;
+ ret = i915_gem_vm_unbind_obj(vm, args);
+
i915_vm_put(vm);
- return -EINVAL;
+ return ret;
}
static const struct drm_ioctl_desc i915_ioctls[] = {
diff --git a/drivers/gpu/drm/i915/i915_vma.c
b/drivers/gpu/drm/i915/i915_vma.c
index 43339ecabd73..d324e29cef0a 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -29,6 +29,7 @@
#include "display/intel_frontbuffer.h"
#include "gem/i915_gem_lmem.h"
#include "gem/i915_gem_tiling.h"
+#include "gem/i915_gem_vm_bind.h"
#include "gt/intel_engine.h"
#include "gt/intel_engine_heartbeat.h"
#include "gt/intel_gt.h"
@@ -234,6 +235,7 @@ vma_create(struct drm_i915_gem_object *obj,
spin_unlock(&obj->vma.lock);
mutex_unlock(&vm->mutex);
+ INIT_LIST_HEAD(&vma->vm_bind_link);
return vma;
err_unlock:
@@ -290,7 +292,6 @@ i915_vma_instance(struct drm_i915_gem_object
*obj,
{
struct i915_vma *vma;
- GEM_BUG_ON(view && !i915_is_ggtt_or_dpt(vm));
GEM_BUG_ON(!kref_read(&vm->ref));
spin_lock(&obj->vma.lock);
@@ -1660,6 +1661,10 @@ static void release_references(struct i915_vma
*vma, bool vm_ddestroy)
spin_unlock(&obj->vma.lock);
+ i915_gem_vm_bind_lock(vma->vm);
+ i915_gem_vm_bind_remove(vma, true);
+ i915_gem_vm_bind_unlock(vma->vm);
+
The vm might be destroyed at this point already.
Ah, due to async vma resource release...
From what I understand we can destroy the vma from three call sites:
1) VM_UNBIND -> The vma has already been removed from the vm_bind
address space,
2) object destruction -> since the vma has an object reference while in
the vm_bind address space, it must also have been removed from the
address space if called from object destruction.
3) vm destruction. Suggestion is to call VM_UNBIND from under the
vm_bind lock early in vm destruction.
Then the above added code can be removed and replaced with an assert
that the vm_bind address space RB_NODE is indeed empty.
...yah, makes sense to move this code to early in VM destruction than
here.
Niranjana
spin_lock_irq(>->closed_lock);
__i915_vma_remove_closed(vma);
spin_unlock_irq(>->closed_lock);
diff --git a/drivers/gpu/drm/i915/i915_vma.h
b/drivers/gpu/drm/i915/i915_vma.h
index 88ca0bd9c900..dcb49f79ff7e 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -164,8 +164,6 @@ i915_vma_compare(struct i915_vma *vma,
{
ptrdiff_t cmp;
- GEM_BUG_ON(view && !i915_is_ggtt_or_dpt(vm));
-
cmp = ptrdiff(vma->vm, vm);
if (cmp)
return cmp;
diff --git a/drivers/gpu/drm/i915/i915_vma_types.h
b/drivers/gpu/drm/i915/i915_vma_types.h
index be6e028c3b57..b6d179bdbfa0 100644
--- a/drivers/gpu/drm/i915/i915_vma_types.h
+++ b/drivers/gpu/drm/i915/i915_vma_types.h
@@ -289,6 +289,14 @@ struct i915_vma {
/** This object's place on the active/inactive lists */
struct list_head vm_link;
+ struct list_head vm_bind_link; /* Link in persistent VMA list
*/
+
+ /** Interval tree structures for persistent vma */
Proper kerneldoc.
+ struct rb_node rb;
+ u64 start;
+ u64 last;
+ u64 __subtree_last;
+
struct list_head obj_link; /* Link in the object's VMA list
*/
struct rb_node obj_node;
struct hlist_node obj_hash;
Thanks,
Thomas
