On Thu, Jul 07, 2022 at 04:27:23AM -0700, Hellstrom, Thomas wrote:
On Fri, 2022-07-01 at 15:50 -0700, Niranjana Vishwanathapura wrote:
Treat VM_BIND vmas as persistent and handle them during the
request submission in the execbuff path.
Support eviction by maintaining a list of evicted persistent vmas
for rebinding during next submission.
Signed-off-by: Niranjana Vishwanathapura
<niranjana.vishwanathapura@xxxxxxxxx>
---
drivers/gpu/drm/i915/gem/i915_gem_object.c | 1 +
drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h | 3 +
.../drm/i915/gem/i915_gem_vm_bind_object.c | 12 ++-
drivers/gpu/drm/i915/gt/intel_gtt.c | 2 +
drivers/gpu/drm/i915/gt/intel_gtt.h | 2 +
drivers/gpu/drm/i915/i915_gem_gtt.h | 22 ++++++
drivers/gpu/drm/i915/i915_vma.c | 32 +++++++-
drivers/gpu/drm/i915/i915_vma.h | 78
+++++++++++++++++--
drivers/gpu/drm/i915/i915_vma_types.h | 23 ++++++
9 files changed, 163 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index ccec4055fde3..5121f02ba95c 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -38,6 +38,7 @@
#include "i915_gem_mman.h"
#include "i915_gem_object.h"
#include "i915_gem_ttm.h"
+#include "i915_gem_vm_bind.h"
#include "i915_memcpy.h"
#include "i915_trace.h"
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
index 849bf3c1061e..eaadf5a6ab09 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind.h
@@ -6,6 +6,7 @@
#ifndef __I915_GEM_VM_BIND_H
#define __I915_GEM_VM_BIND_H
+#include <linux/dma-resv.h>
#include "i915_drv.h"
#define assert_vm_bind_held(vm) lockdep_assert_held(&(vm)-
>vm_bind_lock)
@@ -26,6 +27,8 @@ static inline void i915_gem_vm_bind_unlock(struct
i915_address_space *vm)
mutex_unlock(&vm->vm_bind_lock);
}
+#define assert_vm_priv_held(vm) assert_object_held((vm)->root_obj)
+
static inline int i915_gem_vm_priv_lock(struct i915_address_space
*vm,
struct i915_gem_ww_ctx *ww)
{
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
index 96f139cc8060..1a8efa83547f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_vm_bind_object.c
@@ -85,6 +85,13 @@ void i915_gem_vm_bind_remove(struct i915_vma *vma,
bool release_obj)
{
assert_vm_bind_held(vma->vm);
+ spin_lock(&vma->vm->vm_rebind_lock);
+ if (!list_empty(&vma->vm_rebind_link))
+ list_del_init(&vma->vm_rebind_link);
+ i915_vma_set_purged(vma);
+ i915_vma_set_freed(vma);
Following the vma destruction discussion, can we ditch the freed flag
now?
Yes
+ spin_unlock(&vma->vm->vm_rebind_lock);
+
if (!list_empty(&vma->vm_bind_link)) {
list_del_init(&vma->vm_bind_link);
list_del_init(&vma->non_priv_vm_bind_link);
@@ -220,6 +227,7 @@ static struct i915_vma *vm_bind_get_vma(struct
i915_address_space *vm,
vma->start = va->start;
vma->last = va->start + va->length - 1;
+ i915_vma_set_persistent(vma);
return vma;
}
@@ -304,8 +312,10 @@ int i915_gem_vm_bind_obj(struct
i915_address_space *vm,
i915_vm_bind_put_fence(vma);
put_vma:
- if (ret)
+ if (ret) {
+ i915_vma_set_freed(vma);
i915_vma_destroy(vma);
+ }
i915_gem_ww_ctx_fini(&ww);
unlock_vm:
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c
b/drivers/gpu/drm/i915/gt/intel_gtt.c
index df0a8459c3c6..55d5389b2c6c 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -293,6 +293,8 @@ void i915_address_space_init(struct
i915_address_space *vm, int subclass)
INIT_LIST_HEAD(&vm->non_priv_vm_bind_list);
vm->root_obj = i915_gem_object_create_internal(vm->i915,
PAGE_SIZE);
GEM_BUG_ON(IS_ERR(vm->root_obj));
+ INIT_LIST_HEAD(&vm->vm_rebind_list);
+ spin_lock_init(&vm->vm_rebind_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 f538ce9115c9..fe5485c4a1cd 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
@@ -265,6 +265,8 @@ struct i915_address_space {
struct mutex vm_bind_lock; /* Protects vm_bind lists */
struct list_head vm_bind_list;
struct list_head vm_bound_list;
+ struct list_head vm_rebind_list;
+ spinlock_t vm_rebind_lock; /* Protects vm_rebind_list */
/* va tree of persistent vmas */
struct rb_root_cached va;
struct list_head non_priv_vm_bind_list;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 8c2f57eb5dda..09b89d1913fc 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -51,4 +51,26 @@ int i915_gem_gtt_insert(struct i915_address_space
*vm,
#define PIN_OFFSET_MASK I915_GTT_PAGE_MASK
+static inline int i915_vm_sync(struct i915_address_space *vm)
No need to make this an inline function. Only for performance reasons.
Kerneldoc.
+{
+ int ret;
+
+ /* Wait for all requests under this vm to finish */
+ ret = dma_resv_wait_timeout(vm->root_obj->base.resv,
+ DMA_RESV_USAGE_BOOKKEEP, false,
+ MAX_SCHEDULE_TIMEOUT);
+ if (ret < 0)
+ return ret;
+ else if (ret > 0)
+ return 0;
+ else
+ return -ETIMEDOUT;
+}
+
+static inline bool i915_vm_is_active(const struct i915_address_space
*vm)
+{
+ return !dma_resv_test_signaled(vm->root_obj->base.resv,
+ DMA_RESV_USAGE_BOOKKEEP);
+}
+
#endif
diff --git a/drivers/gpu/drm/i915/i915_vma.c
b/drivers/gpu/drm/i915/i915_vma.c
index 6737236b7884..6adb013579be 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -237,6 +237,7 @@ vma_create(struct drm_i915_gem_object *obj,
INIT_LIST_HEAD(&vma->vm_bind_link);
INIT_LIST_HEAD(&vma->non_priv_vm_bind_link);
+ INIT_LIST_HEAD(&vma->vm_rebind_link);
return vma;
err_unlock:
@@ -1622,7 +1623,8 @@ void i915_vma_close(struct i915_vma *vma)
if (atomic_dec_and_lock_irqsave(&vma->open_count,
>->closed_lock,
flags)) {
- __vma_close(vma, gt);
+ if (!i915_vma_is_persistent(vma))
+ __vma_close(vma, gt);
This change is not needed since persistent vmas shouldn't take part in
the other vma open-close life-time management.
Yah, looks like a relic from when we did not have separate execbuf3.
Should be removed.
spin_unlock_irqrestore(>->closed_lock, flags);
}
}
@@ -1647,6 +1649,13 @@ static void force_unbind(struct i915_vma *vma)
if (!drm_mm_node_allocated(&vma->node))
return;
+ /*
+ * Mark persistent vma as purged to avoid it waiting
+ * for VM to be released.
+ */
+ if (i915_vma_is_persistent(vma))
+ i915_vma_set_purged(vma);
+
atomic_and(~I915_VMA_PIN_MASK, &vma->flags);
WARN_ON(__i915_vma_unbind(vma));
GEM_BUG_ON(drm_mm_node_allocated(&vma->node));
@@ -1666,9 +1675,12 @@ 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);
+ if (i915_vma_is_persistent(vma) &&
+ !i915_vma_is_freed(vma)) {
+ i915_gem_vm_bind_lock(vma->vm);
+ i915_gem_vm_bind_remove(vma, true);
+ i915_gem_vm_bind_unlock(vma->vm);
+ }
spin_lock_irq(>->closed_lock);
__i915_vma_remove_closed(vma);
@@ -1839,6 +1851,8 @@ int _i915_vma_move_to_active(struct i915_vma
*vma,
int err;
assert_object_held(obj);
+ if (i915_vma_is_persistent(vma))
+ return -EINVAL;
GEM_BUG_ON(!vma->pages);
@@ -1999,6 +2013,16 @@ int __i915_vma_unbind(struct i915_vma *vma)
__i915_vma_evict(vma, false);
drm_mm_remove_node(&vma->node); /* pairs with
i915_vma_release() */
+
+ if (i915_vma_is_persistent(vma)) {
+ spin_lock(&vma->vm->vm_rebind_lock);
+ if (list_empty(&vma->vm_rebind_link) &&
+ !i915_vma_is_purged(vma))
+ list_add_tail(&vma->vm_rebind_link,
+ &vma->vm->vm_rebind_list);
+ spin_unlock(&vma->vm->vm_rebind_lock);
+ }
+
return 0;
}
diff --git a/drivers/gpu/drm/i915/i915_vma.h
b/drivers/gpu/drm/i915/i915_vma.h
index dcb49f79ff7e..6c1369a40e03 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -47,12 +47,6 @@ i915_vma_instance(struct drm_i915_gem_object *obj,
void i915_vma_unpin_and_release(struct i915_vma **p_vma, unsigned
int flags);
#define I915_VMA_RELEASE_MAP BIT(0)
-
-static inline bool i915_vma_is_active(const struct i915_vma *vma)
-{
- return !i915_active_is_idle(&vma->active);
-}
-
/* do not reserve memory to prevent deadlocks */
#define __EXEC_OBJECT_NO_RESERVE BIT(31)
@@ -138,6 +132,48 @@ static inline u32 i915_ggtt_pin_bias(struct
i915_vma *vma)
return i915_vm_to_ggtt(vma->vm)->pin_bias;
}
+static inline bool i915_vma_is_persistent(const struct i915_vma
*vma)
+{
+ return test_bit(I915_VMA_PERSISTENT_BIT,
__i915_vma_flags(vma));
+}
+
+static inline void i915_vma_set_persistent(struct i915_vma *vma)
+{
+ set_bit(I915_VMA_PERSISTENT_BIT, __i915_vma_flags(vma));
+}
+
+static inline bool i915_vma_is_purged(const struct i915_vma *vma)
+{
+ return test_bit(I915_VMA_PURGED_BIT, __i915_vma_flags(vma));
+}
+
+static inline void i915_vma_set_purged(struct i915_vma *vma)
+{
+ set_bit(I915_VMA_PURGED_BIT, __i915_vma_flags(vma));
+}
+
+static inline bool i915_vma_is_freed(const struct i915_vma *vma)
+{
+ return test_bit(I915_VMA_FREED_BIT, __i915_vma_flags(vma));
+}
+
+static inline void i915_vma_set_freed(struct i915_vma *vma)
+{
+ set_bit(I915_VMA_FREED_BIT, __i915_vma_flags(vma));
+}
+
+static inline bool i915_vma_is_active(const struct i915_vma *vma)
+{
+ if (i915_vma_is_persistent(vma)) {
+ if (i915_vma_is_purged(vma))
+ return false;
+
+ return i915_vm_is_active(vma->vm);
+ }
+
+ return !i915_active_is_idle(&vma->active);
+}
+
static inline struct i915_vma *i915_vma_get(struct i915_vma *vma)
{
i915_gem_object_get(vma->obj);
@@ -408,8 +444,36 @@ int i915_vma_wait_for_bind(struct i915_vma
*vma);
static inline int i915_vma_sync(struct i915_vma *vma)
{
+ int ret;
+
/* Wait for the asynchronous bindings and pending GPU reads
*/
- return i915_active_wait(&vma->active);
+ ret = i915_active_wait(&vma->active);
+ if (ret || !i915_vma_is_persistent(vma) ||
i915_vma_is_purged(vma))
+ return ret;
+
+ return i915_vm_sync(vma->vm);
+}
+
+static inline bool i915_vma_is_bind_complete(struct i915_vma *vma)
+{
+ /* Ensure vma bind is initiated */
+ if (!i915_vma_is_bound(vma, I915_VMA_BIND_MASK))
+ return false;
+
+ /* Ensure any binding started is complete */
+ if (rcu_access_pointer(vma->active.excl.fence)) {
+ struct dma_fence *fence;
+
+ rcu_read_lock();
+ fence = dma_fence_get_rcu_safe(&vma-
>active.excl.fence);
+ rcu_read_unlock();
+ if (fence) {
+ dma_fence_put(fence);
+ return false;
+ }
+ }
Could we use i915_active_fence_get() instead of first testing the
pointer and then open-coding i915_active_fence_get()? Also no need to
inline this.
Yah. Looks like we can also use i915_vma_verify_bind_complete() (just
move it out of I915_DEBUG_GEM).
Niranjana
+
+ return true;
}
/**
diff --git a/drivers/gpu/drm/i915/i915_vma_types.h
b/drivers/gpu/drm/i915/i915_vma_types.h
index 7d830a6a0b51..405c82e1bc30 100644
--- a/drivers/gpu/drm/i915/i915_vma_types.h
+++ b/drivers/gpu/drm/i915/i915_vma_types.h
@@ -264,6 +264,28 @@ struct i915_vma {
#define I915_VMA_SCANOUT_BIT 17
#define I915_VMA_SCANOUT ((int)BIT(I915_VMA_SCANOUT_BIT))
+ /**
+ * I915_VMA_PERSISTENT_BIT:
+ * The vma is persistent (created with VM_BIND call).
+ *
+ * I915_VMA_PURGED_BIT:
+ * The persistent vma is force unbound either due to VM_UNBIND
call
+ * from UMD or VM is released. Do not check/wait for VM activeness
+ * in i915_vma_is_active() and i915_vma_sync() calls.
+ *
+ * I915_VMA_FREED_BIT:
+ * The persistent vma is being released by UMD via VM_UNBIND call.
+ * While releasing the vma, do not take VM_BIND lock as VM_UNBIND
call
+ * already holds the lock.
+ */
+#define I915_VMA_PERSISTENT_BIT 19
+#define I915_VMA_PURGED_BIT 20
+#define I915_VMA_FREED_BIT 21
+
+#define I915_VMA_PERSISTENT ((int)BIT(I915_VMA_PERSISTENT_BIT))
+#define
I915_VMA_PURGED ((int)BIT(I915_VMA_PURGED_BIT))
+#define I915_VMA_FREED ((int)BIT(I915_VMA_FREED_BIT))
+
struct i915_active active;
#define I915_VMA_PAGES_BIAS 24
@@ -292,6 +314,7 @@ struct i915_vma {
struct list_head vm_bind_link; /* Link in persistent VMA list
*/
Protected by..
/* Link in non-private persistent VMA list */
struct list_head non_priv_vm_bind_link;
+ struct list_head vm_rebind_link; /* Link in vm_rebind_list */
/** Timeline fence for vm_bind completion notification */
struct {