drm/i915: Lazily unbind vma on close

When userspace is passing around swapbuffers using DRI, we frequently
have to open and close the same object in the foreign address space.
This shows itself as the same object being rebound at roughly 30fps
(with a second object also being rebound at 30fps), which involves us
having to rewrite the page tables and maintain the drm_mm range manager
every time.

However, since the object still exists and it is only the local handle
that disappears, if we are lazy and do not unbind the VMA immediately
when the local user closes the object but defer it until the GPU is
idle, then we can reuse the same VMA binding. We still have to be
careful to mark the handle and lookup tables as closed to maintain the
uABI, just allowing the underlying VMA to be resurrected if the user is
able to access the same object from the same context again.

If the object itself is destroyed (neither userspace keeping a handle to
it), the VMA will be reaped immediately as usual.

In the future, this will be even more useful as instantiating a new VMA
for use on the GPU will become heavier. A nuisance indeed, so nip it in
the bud.

v2: s/__i915_vma_final_close/i915_vma_destroy/ etc.
v3: Leave a hint as to why we deferred the unbind on close.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20180503195115.22309-1-chris@chris-wilson.co.uk

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 11ff84eef52a8d0076767b99c2829e7eb5032617..04e27806e58131976724181f0a43b7011f644a0c 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2062,6 +2062,7 @@ struct drm_i915_private {
                struct list_head timelines;
 
                struct list_head active_rings;
+               struct list_head closed_vma;
                u32 active_requests;
                u32 request_serial;
 

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 484354f25f98413fcf7ee9d1c899b0b9b509641a..5ece6ae4bdff04267cbac9a764eb026a56bb6746 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -165,6 +165,7 @@ static u32 __i915_gem_park(struct drm_i915_private *i915)
        i915_timelines_park(i915);
 
        i915_pmu_gt_parked(i915);
+       i915_vma_parked(i915);
 
        i915->gt.awake = false;
 
@@ -4795,7 +4796,7 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
                                         &obj->vma_list, obj_link) {
                        GEM_BUG_ON(i915_vma_is_active(vma));
                        vma->flags &= ~I915_VMA_PIN_MASK;
-                       i915_vma_close(vma);
+                       i915_vma_destroy(vma);
                }
                GEM_BUG_ON(!list_empty(&obj->vma_list));
                GEM_BUG_ON(!RB_EMPTY_ROOT(&obj->vma_tree));
@@ -5598,6 +5599,7 @@ int i915_gem_init_early(struct drm_i915_private *dev_priv)
 
        INIT_LIST_HEAD(&dev_priv->gt.timelines);
        INIT_LIST_HEAD(&dev_priv->gt.active_rings);
+       INIT_LIST_HEAD(&dev_priv->gt.closed_vma);
 
        i915_gem_init__mm(dev_priv);
 

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index c74f5df3fb5a91b234fb5ad3d3d45919b5fcbd44..f627a8c47c58a36f6ff92f17a4d6d672b28bc00b 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -762,7 +762,8 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
                }
 
                /* transfer ref to ctx */
-               vma->open_count++;
+               if (!vma->open_count++)
+                       i915_vma_reopen(vma);
                list_add(&lut->obj_link, &obj->lut_list);
                list_add(&lut->ctx_link, &eb->ctx->handles_list);
                lut->ctx = eb->ctx;

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index e9d828324f678141d1877807b156bc9c82cdb0c4..272d6bb407ccf22997ca228872fc421af2731614 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2218,6 +2218,12 @@ i915_ppgtt_create(struct drm_i915_private *dev_priv,
 }
 
 void i915_ppgtt_close(struct i915_address_space *vm)
+{
+       GEM_BUG_ON(vm->closed);
+       vm->closed = true;
+}
+
+static void ppgtt_destroy_vma(struct i915_address_space *vm)
 {
        struct list_head *phases[] = {
                &vm->active_list,
@@ -2226,15 +2232,12 @@ void i915_ppgtt_close(struct i915_address_space *vm)
                NULL,
        }, **phase;
 
-       GEM_BUG_ON(vm->closed);
        vm->closed = true;
-
        for (phase = phases; *phase; phase++) {
                struct i915_vma *vma, *vn;
 
                list_for_each_entry_safe(vma, vn, *phase, vm_link)
-                       if (!i915_vma_is_closed(vma))
-                               i915_vma_close(vma);
+                       i915_vma_destroy(vma);
        }
 }
 
@@ -2245,7 +2248,8 @@ void i915_ppgtt_release(struct kref *kref)
 
        trace_i915_ppgtt_release(&ppgtt->base);
 
-       /* vmas should already be unbound and destroyed */
+       ppgtt_destroy_vma(&ppgtt->base);
+
        GEM_BUG_ON(!list_empty(&ppgtt->base.active_list));
        GEM_BUG_ON(!list_empty(&ppgtt->base.inactive_list));
        GEM_BUG_ON(!list_empty(&ppgtt->base.unbound_list));

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 4bda3bd29bf5efa50d480b80a13729cb24c39da1..9324d476e0a7c356b39cb02374e904a2b0a95262 100644 (file)
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -46,8 +46,6 @@ i915_vma_retire(struct i915_gem_active *active, struct i915_request *rq)
 
        GEM_BUG_ON(!drm_mm_node_allocated(&vma->node));
        list_move_tail(&vma->vm_link, &vma->vm->inactive_list);
-       if (unlikely(i915_vma_is_closed(vma) && !i915_vma_is_pinned(vma)))
-               WARN_ON(i915_vma_unbind(vma));
 
        GEM_BUG_ON(!i915_gem_object_is_active(obj));
        if (--obj->active_count)
@@ -232,7 +230,6 @@ i915_vma_instance(struct drm_i915_gem_object *obj,
        if (!vma)
                vma = vma_create(obj, vm, view);
 
-       GEM_BUG_ON(!IS_ERR(vma) && i915_vma_is_closed(vma));
        GEM_BUG_ON(!IS_ERR(vma) && i915_vma_compare(vma, vm, view));
        GEM_BUG_ON(!IS_ERR(vma) && vma_lookup(obj, vm, view) != vma);
        return vma;
@@ -684,13 +681,43 @@ err_unpin:
        return ret;
 }
 
-static void i915_vma_destroy(struct i915_vma *vma)
+void i915_vma_close(struct i915_vma *vma)
+{
+       lockdep_assert_held(&vma->vm->i915->drm.struct_mutex);
+
+       GEM_BUG_ON(i915_vma_is_closed(vma));
+       vma->flags |= I915_VMA_CLOSED;
+
+       /*
+        * We defer actually closing, unbinding and destroying the VMA until
+        * the next idle point, or if the object is freed in the meantime. By
+        * postponing the unbind, we allow for it to be resurrected by the
+        * client, avoiding the work required to rebind the VMA. This is
+        * advantageous for DRI, where the client/server pass objects
+        * between themselves, temporarily opening a local VMA to the
+        * object, and then closing it again. The same object is then reused
+        * on the next frame (or two, depending on the depth of the swap queue)
+        * causing us to rebind the VMA once more. This ends up being a lot
+        * of wasted work for the steady state.
+        */
+       list_add_tail(&vma->closed_link, &vma->vm->i915->gt.closed_vma);
+}
+
+void i915_vma_reopen(struct i915_vma *vma)
+{
+       lockdep_assert_held(&vma->vm->i915->drm.struct_mutex);
+
+       if (vma->flags & I915_VMA_CLOSED) {
+               vma->flags &= ~I915_VMA_CLOSED;
+               list_del(&vma->closed_link);
+       }
+}
+
+static void __i915_vma_destroy(struct i915_vma *vma)
 {
        int i;
 
        GEM_BUG_ON(vma->node.allocated);
-       GEM_BUG_ON(i915_vma_is_active(vma));
-       GEM_BUG_ON(!i915_vma_is_closed(vma));
        GEM_BUG_ON(vma->fence);
 
        for (i = 0; i < ARRAY_SIZE(vma->last_read); i++)
@@ -699,6 +726,7 @@ static void i915_vma_destroy(struct i915_vma *vma)
 
        list_del(&vma->obj_link);
        list_del(&vma->vm_link);
+       rb_erase(&vma->obj_node, &vma->obj->vma_tree);
 
        if (!i915_vma_is_ggtt(vma))
                i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm));
@@ -706,15 +734,30 @@ static void i915_vma_destroy(struct i915_vma *vma)
        kmem_cache_free(to_i915(vma->obj->base.dev)->vmas, vma);
 }
 
-void i915_vma_close(struct i915_vma *vma)
+void i915_vma_destroy(struct i915_vma *vma)
 {
-       GEM_BUG_ON(i915_vma_is_closed(vma));
-       vma->flags |= I915_VMA_CLOSED;
+       lockdep_assert_held(&vma->vm->i915->drm.struct_mutex);
 
-       rb_erase(&vma->obj_node, &vma->obj->vma_tree);
+       GEM_BUG_ON(i915_vma_is_active(vma));
+       GEM_BUG_ON(i915_vma_is_pinned(vma));
+
+       if (i915_vma_is_closed(vma))
+               list_del(&vma->closed_link);
+
+       WARN_ON(i915_vma_unbind(vma));
+       __i915_vma_destroy(vma);
+}
+
+void i915_vma_parked(struct drm_i915_private *i915)
+{
+       struct i915_vma *vma, *next;
 
-       if (!i915_vma_is_active(vma) && !i915_vma_is_pinned(vma))
-               WARN_ON(i915_vma_unbind(vma));
+       list_for_each_entry_safe(vma, next, &i915->gt.closed_vma, closed_link) {
+               GEM_BUG_ON(!i915_vma_is_closed(vma));
+               i915_vma_destroy(vma);
+       }
+
+       GEM_BUG_ON(!list_empty(&i915->gt.closed_vma));
 }
 
 static void __i915_vma_iounmap(struct i915_vma *vma)
@@ -804,7 +847,7 @@ int i915_vma_unbind(struct i915_vma *vma)
                return -EBUSY;
 
        if (!drm_mm_node_allocated(&vma->node))
-               goto destroy;
+               return 0;
 
        GEM_BUG_ON(obj->bind_count == 0);
        GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj));
@@ -841,10 +884,6 @@ int i915_vma_unbind(struct i915_vma *vma)
 
        i915_vma_remove(vma);
 
-destroy:
-       if (unlikely(i915_vma_is_closed(vma)))
-               i915_vma_destroy(vma);
-
        return 0;
 }
 

diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index 8c50220954183aa0c3e019fe3ebe2ceddc791349..fc4294cfaa91314bbfccaa75aa2692b9194c737b 100644 (file)
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -119,6 +119,8 @@ struct i915_vma {
        /** This vma's place in the eviction list */
        struct list_head evict_link;
 
+       struct list_head closed_link;
+
        /**
         * Used for performing relocations during execbuffer insertion.
         */
@@ -285,6 +287,8 @@ void i915_vma_revoke_mmap(struct i915_vma *vma);
 int __must_check i915_vma_unbind(struct i915_vma *vma);
 void i915_vma_unlink_ctx(struct i915_vma *vma);
 void i915_vma_close(struct i915_vma *vma);
+void i915_vma_reopen(struct i915_vma *vma);
+void i915_vma_destroy(struct i915_vma *vma);
 
 int __i915_vma_do_pin(struct i915_vma *vma,
                      u64 size, u64 alignment, u64 flags);
@@ -408,6 +412,8 @@ i915_vma_unpin_fence(struct i915_vma *vma)
                __i915_vma_unpin_fence(vma);
 }
 
+void i915_vma_parked(struct drm_i915_private *i915);
+
 #define for_each_until(cond) if (cond) break; else
 
 /**

diff --git a/drivers/gpu/drm/i915/selftests/huge_pages.c b/drivers/gpu/drm/i915/selftests/huge_pages.c
index 05bbef363fff5119874d4570b038b79e624b3f59..d7c8ef8e6764b1acc66864efc7e06bfd1986d379 100644 (file)
--- a/drivers/gpu/drm/i915/selftests/huge_pages.c
+++ b/drivers/gpu/drm/i915/selftests/huge_pages.c
@@ -1091,7 +1091,7 @@ static int __igt_write_huge(struct i915_gem_context *ctx,
 out_vma_unpin:
        i915_vma_unpin(vma);
 out_vma_close:
-       i915_vma_close(vma);
+       i915_vma_destroy(vma);
 
        return err;
 }

diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
index a662c0450e77904f067c6a71997d7aaa1f3add5e..4b6622c6986a18f4babedf5ab8ab4e3b8a00a206 100644 (file)
--- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
+++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
@@ -226,6 +226,7 @@ struct drm_i915_private *mock_gem_device(void)
 
        INIT_LIST_HEAD(&i915->gt.timelines);
        INIT_LIST_HEAD(&i915->gt.active_rings);
+       INIT_LIST_HEAD(&i915->gt.closed_vma);
 
        mutex_lock(&i915->drm.struct_mutex);
        mock_init_ggtt(i915);