[Intel-gfx] [PATCH v7 20/20] drm/i915/vm_bind: Async vm_unbind support

Niranjana Vishwanathapura niranjana.vishwanathapura at intel.com
Thu Dec 1 15:11:35 UTC 2022 

On Thu, Dec 01, 2022 at 10:10:14AM +0000, Matthew Auld wrote:
>On 29/11/2022 23:26, Niranjana Vishwanathapura wrote:
>>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.vishwanathapura at 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.
>
>Ok, maybe make that a seperate patch, since it seems to change the 
>behaviour for more than just persisent mappings, AFAICT.
>

Ok, will do.

Niranjana

>>
>>Niranjana
>>
>>>
>>>>
>>>>Niranjana
>>>>
>>>>>Niranjana
>>>>>
>>>>>>
>>>>>>Niranjana
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>Niranjana
>>>>>>>>
>>>>>>>>>Reviewed-by: Matthew Auld <matthew.auld at 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 */

More information about the Intel-gfx mailing list