Re: [PATCH v7 20/20] drm/i915/vm_bind: Async vm_unbind support

Tue, 29 Nov 2022 15:27:09 -0800 

On Wed, Nov 23, 2022 at 11:42:58AM +0000, Matthew Auld wrote:

On 16/11/2022 00:37, Niranjana Vishwanathapura wrote:

On Tue, Nov 15, 2022 at 03:15:03PM -0800, Niranjana Vishwanathapura wrote:
On Tue, Nov 15, 2022 at 08:33:47AM -0800, Niranjana Vishwanathapura wrote: 
On Tue, Nov 15, 2022 at 04:20:54PM +0000, Matthew Auld wrote:

On 15/11/2022 16:15, Niranjana Vishwanathapura wrote:

On Tue, Nov 15, 2022 at 11:05:21AM +0000, Matthew Auld wrote:

On 13/11/2022 07:57, Niranjana Vishwanathapura wrote:

Asynchronously unbind the vma upon vm_unbind call.
Fall back to synchronous unbind if backend doesn't support
async unbind or if async unbind fails.

No need for vm_unbind out fence support as i915 will internally
handle all sequencing and user need not try to sequence any
operation with the unbind completion.

v2: use i915_vma_destroy_async in vm_unbind ioctl
Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathap...@intel.com>
This only does it for non-partial vma, right? Or was that changed somewhere? 



No, it applies to any vma (partial or non-partial).
It was so from the beginning.
Doesn't __i915_vma_unbind_async() return an error when mm.pages != vma->pages? IIRC this was discussed before. Just trying to think about the consequences of this change. 

I am not seeing any such restriction. Let me probe and check if there
is any such restriction anywhere in the call chain.


I checked and I am not seeing any restriction anywher in the call chain.



Note that just like binding case, unbinding is also synchronous unless
there is a pending activity, in which case, it will be asynchronous.


In __i915_vma_unbind_async() there is:

if (i915_vma_is_pinned(vma) ||
   &vma->obj->mm.rsgt->table != vma->resource->bi.pages)
       return ERR_PTR(-EAGAIN);
AFAICT we then also don't get any pipelined moves with such an object, if there is such a binding present. 

I had to remove this check as otherwise for VM_BIND (persistent) mappings,
it will alwasy be true and we will always endup with -EAGAIN.
Persistent mappings, as they support partial binding, can't have an sg
table which is just a pointer to object's sg table.

Niranjana





Niranjana


Niranjana



Niranjana





Niranjana


Reviewed-by: Matthew Auld <matthew.a...@intel.com>


---
 .../drm/i915/gem/i915_gem_vm_bind_object.c    |  2 +-
 drivers/gpu/drm/i915/i915_vma.c               | 51 +++++++++++++++++-- 
 drivers/gpu/drm/i915/i915_vma.h               |  1 +
 include/uapi/drm/i915_drm.h                   |  3 +-
 4 files changed, 51 insertions(+), 6 deletions(-)
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 d87d1210365b..36651b447966 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
@@ -210,7 +210,7 @@ static int i915_gem_vm_unbind_vma(struct i915_address_space *vm, 
      */
     obj = vma->obj;
     i915_gem_object_lock(obj, NULL);
-    i915_vma_destroy(vma);
+    i915_vma_destroy_async(vma);
     i915_gem_object_unlock(obj);
     i915_gem_object_put(obj);
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c 
index 7cf77c67d755..483d25f2425c 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -42,6 +42,8 @@
 #include "i915_vma.h"
 #include "i915_vma_resource.h"
+static struct dma_fence *__i915_vma_unbind_async(struct i915_vma *vma); 
+
 static inline void assert_vma_held_evict(const struct i915_vma *vma) 
 {
     /*
@@ -1713,7 +1715,7 @@ void i915_vma_reopen(struct i915_vma *vma)
     spin_unlock_irq(&gt->closed_lock);
 }
-static void force_unbind(struct i915_vma *vma)
+static void force_unbind(struct i915_vma *vma, bool async)
 {
     if (!drm_mm_node_allocated(&vma->node))
         return;
@@ -1727,7 +1729,21 @@ static void force_unbind(struct i915_vma *vma) 
         i915_vma_set_purged(vma);
     atomic_and(~I915_VMA_PIN_MASK, &vma->flags);
-    WARN_ON(__i915_vma_unbind(vma));
+    if (async) {
+        struct dma_fence *fence;
+
+        fence = __i915_vma_unbind_async(vma);
+        if (IS_ERR_OR_NULL(fence)) {
+            async = false;
+        } else {
+            dma_resv_add_fence(vma->obj->base.resv, fence,
+                       DMA_RESV_USAGE_READ);
+            dma_fence_put(fence);
+        }
+    }
+
+    if (!async)
+        WARN_ON(__i915_vma_unbind(vma));
     GEM_BUG_ON(drm_mm_node_allocated(&vma->node));
 }
@@ -1787,7 +1803,7 @@ void i915_vma_destroy_locked(struct i915_vma *vma) 
 {
     lockdep_assert_held(&vma->vm->mutex);
-    force_unbind(vma);
+    force_unbind(vma, false);
     list_del_init(&vma->vm_link);
     release_references(vma, vma->vm->gt, false);
 }
@@ -1798,7 +1814,34 @@ void i915_vma_destroy(struct i915_vma *vma)
     bool vm_ddestroy;
     mutex_lock(&vma->vm->mutex);
-    force_unbind(vma);
+    force_unbind(vma, false);
+    list_del_init(&vma->vm_link);
+    vm_ddestroy = vma->vm_ddestroy;
+    vma->vm_ddestroy = false;
+
+    /* vma->vm may be freed when releasing vma->vm->mutex. */
+    gt = vma->vm->gt;
+    mutex_unlock(&vma->vm->mutex);
+    release_references(vma, gt, vm_ddestroy);
+}
+
+void i915_vma_destroy_async(struct i915_vma *vma)
+{
+    bool vm_ddestroy, async = vma->obj->mm.rsgt;
+    struct intel_gt *gt;
+
+    if (dma_resv_reserve_fences(vma->obj->base.resv, 1))
+        async = false;
+
+    mutex_lock(&vma->vm->mutex);
+    /*
+     * Ensure any asynchronous binding is complete while using
+     * async unbind as we will be releasing the vma here.
+     */
+    if (async && i915_active_wait(&vma->active))
+        async = false;
+
+    force_unbind(vma, async);
     list_del_init(&vma->vm_link);
     vm_ddestroy = vma->vm_ddestroy;
     vma->vm_ddestroy = false;
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h 
index 737ef310d046..25f15965dab8 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -272,6 +272,7 @@ void i915_vma_reopen(struct i915_vma *vma);
 void i915_vma_destroy_locked(struct i915_vma *vma);
 void i915_vma_destroy(struct i915_vma *vma);
+void i915_vma_destroy_async(struct i915_vma *vma);
 #define assert_vma_held(vma) dma_resv_assert_held((vma)->obj->base.resv) diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h 
index e5600f358a15..431d40bb1dee 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -3969,7 +3969,8 @@ struct drm_i915_gem_vm_bind {
  * any error.
  *
  * VM_BIND/UNBIND ioctl calls executed on different CPU threads concurrently 
- * are not ordered.
+ * are not ordered. Furthermore, parts of the VM_UNBIND operation can be done 
+ * asynchronously.
  */
 struct drm_i915_gem_vm_unbind {
     /** @vm_id: VM (address space) id to bind */

Reply via email to