drm/i915: Simplify eb_lookup_vmas()

Since the introduction of being able to perform a lockless lookup of an
object (i915_gem_object_get_rcu() in 254199ca8f79 ("drm/i915: Move object
release to a freelist + worker") we no longer need to split the
object/vma lookup into 3 phases and so combine them into a much simpler
single loop.

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

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index da6cb2fe5f853dd220bd3ab50d3e70d2197cc5bd..95e461259d242d69f97ef30b432302ea0bb762b0 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -450,7 +450,9 @@ eb_validate_vma(struct i915_execbuffer *eb,
 }
 
 static int
-eb_add_vma(struct i915_execbuffer *eb, unsigned int i, struct i915_vma *vma)
+eb_add_vma(struct i915_execbuffer *eb,
+          unsigned int i, struct i915_vma *vma,
+          unsigned int flags)
 {
        struct drm_i915_gem_exec_object2 *entry = &eb->exec[i];
        int err;
@@ -480,7 +482,7 @@ eb_add_vma(struct i915_execbuffer *eb, unsigned int i, struct i915_vma *vma)
         * to find the right target VMA when doing relocations.
         */
        eb->vma[i] = vma;
-       eb->flags[i] = entry->flags;
+       eb->flags[i] = entry->flags | flags;
        vma->exec_flags = &eb->flags[i];
 
        err = 0;
@@ -686,13 +688,9 @@ static int eb_select_context(struct i915_execbuffer *eb)
 
 static int eb_lookup_vmas(struct i915_execbuffer *eb)
 {
-#define INTERMEDIATE BIT(0)
-       const unsigned int count = eb->buffer_count;
        struct i915_gem_context_vma_lut *lut = &eb->ctx->vma_lut;
-       struct i915_vma *vma;
-       struct idr *idr;
+       struct drm_i915_gem_object *uninitialized_var(obj);
        unsigned int i;
-       int slow_pass = -1;
        int err;
 
        if (unlikely(i915_gem_context_is_closed(eb->ctx)))
@@ -708,82 +706,38 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
                flush_work(&lut->resize);
        GEM_BUG_ON(lut->ht_size & I915_CTX_RESIZE_IN_PROGRESS);
 
-       for (i = 0; i < count; i++) {
-               hlist_for_each_entry(vma,
-                                    ht_head(lut, eb->exec[i].handle),
-                                    ctx_node) {
-                       if (vma->ctx_handle != eb->exec[i].handle)
-                               continue;
-
-                       err = eb_add_vma(eb, i, vma);
-                       if (unlikely(err))
-                               return err;
-                       goto next_vma;
-               }
-
-               if (slow_pass < 0)
-                       slow_pass = i;
-next_vma: ;
-       }
+       for (i = 0; i < eb->buffer_count; i++) {
+               u32 handle = eb->exec[i].handle;
+               struct hlist_head *hl = ht_head(lut, handle);
+               unsigned int flags = 0;
+               struct i915_vma *vma;
 
-       if (slow_pass < 0)
-               goto out;
+               hlist_for_each_entry(vma, hl, ctx_node) {
+                       GEM_BUG_ON(vma->ctx != eb->ctx);
 
-       spin_lock(&eb->file->table_lock);
-       /*
-        * Grab a reference to the object and release the lock so we can lookup
-        * or create the VMA without using GFP_ATOMIC
-        */
-       idr = &eb->file->object_idr;
-       for (i = slow_pass; i < count; i++) {
-               struct drm_i915_gem_object *obj;
+                       if (vma->ctx_handle != handle)
+                               continue;
 
-               if (eb->vma[i])
-                       continue;
+                       goto add_vma;
+               }
 
-               obj = to_intel_bo(idr_find(idr, eb->exec[i].handle));
+               obj = i915_gem_object_lookup(eb->file, handle);
                if (unlikely(!obj)) {
-                       spin_unlock(&eb->file->table_lock);
-                       DRM_DEBUG("Invalid object handle %d at index %d\n",
-                                 eb->exec[i].handle, i);
                        err = -ENOENT;
-                       goto err;
+                       goto err_vma;
                }
 
-               eb->vma[i] = (struct i915_vma *)
-                       ptr_pack_bits(obj, INTERMEDIATE, 1);
-       }
-       spin_unlock(&eb->file->table_lock);
-
-       for (i = slow_pass; i < count; i++) {
-               struct drm_i915_gem_object *obj;
-               unsigned int is_obj;
-
-               obj = (typeof(obj))ptr_unpack_bits(eb->vma[i], &is_obj, 1);
-               if (!is_obj)
-                       continue;
-
-               /*
-                * NOTE: We can leak any vmas created here when something fails
-                * later on. But that's no issue since vma_unbind can deal with
-                * vmas which are not actually bound. And since only
-                * lookup_or_create exists as an interface to get at the vma
-                * from the (obj, vm) we don't run the risk of creating
-                * duplicated vmas for the same vm.
-                */
                vma = i915_vma_instance(obj, eb->vm, NULL);
                if (unlikely(IS_ERR(vma))) {
-                       DRM_DEBUG("Failed to lookup VMA\n");
                        err = PTR_ERR(vma);
-                       goto err;
+                       goto err_obj;
                }
 
                /* First come, first served */
                if (!vma->ctx) {
                        vma->ctx = eb->ctx;
-                       vma->ctx_handle = eb->exec[i].handle;
-                       hlist_add_head(&vma->ctx_node,
-                                      ht_head(lut, eb->exec[i].handle));
+                       vma->ctx_handle = handle;
+                       hlist_add_head(&vma->ctx_node, hl);
                        lut->ht_count++;
                        lut->ht_size |= I915_CTX_RESIZE_IN_PROGRESS;
                        if (i915_vma_is_ggtt(vma)) {
@@ -791,21 +745,19 @@ next_vma: ;
                                obj->vma_hashed = vma;
                        }
 
-                       i915_vma_get(vma);
+                       /* transfer ref to ctx */
+                       obj = NULL;
+               } else {
+                       flags = __EXEC_OBJECT_HAS_REF;
                }
 
-               err = eb_add_vma(eb, i, vma);
+add_vma:
+               err = eb_add_vma(eb, i, vma, flags);
                if (unlikely(err))
-                       goto err;
+                       goto err_obj;
 
                GEM_BUG_ON(vma != eb->vma[i]);
                GEM_BUG_ON(vma->exec_flags != &eb->flags[i]);
-
-               /* Only after we validated the user didn't use our bits */
-               if (vma->ctx != eb->ctx) {
-                       i915_vma_get(vma);
-                       *vma->exec_flags |= __EXEC_OBJECT_HAS_REF;
-               }
        }
 
        if (lut->ht_size & I915_CTX_RESIZE_IN_PROGRESS) {
@@ -815,7 +767,6 @@ next_vma: ;
                        lut->ht_size &= ~I915_CTX_RESIZE_IN_PROGRESS;
        }
 
-out:
        /* take note of the batch buffer before we might reorder the lists */
        i = eb_batch_index(eb);
        eb->batch = eb->vma[i];
@@ -838,14 +789,13 @@ out:
        eb->args->flags |= __EXEC_VALIDATED;
        return eb_reserve(eb);
 
-err:
-       for (i = slow_pass; i < count; i++) {
-               if (ptr_unmask_bits(eb->vma[i], 1))
-                       eb->vma[i] = NULL;
-       }
+err_obj:
+       if (obj)
+               i915_gem_object_put(obj);
+err_vma:
+       eb->vma[i] = NULL;
        lut->ht_size &= ~I915_CTX_RESIZE_IN_PROGRESS;
        return err;
-#undef INTERMEDIATE
 }
 
 static struct i915_vma *
@@ -878,7 +828,7 @@ static void eb_release_vmas(const struct i915_execbuffer *eb)
                unsigned int flags = eb->flags[i];
 
                if (!vma)
-                       continue;
+                       break;
 
                GEM_BUG_ON(vma->exec_flags != &eb->flags[i]);
                vma->exec_flags = NULL;
@@ -2268,8 +2218,8 @@ i915_gem_do_execbuffer(struct drm_device *dev,
                args->flags |= __EXEC_HAS_RELOC;
 
        eb.exec = exec;
-       eb.vma = memset(exec + args->buffer_count + 1, 0,
-                       (args->buffer_count + 1) * sizeof(*eb.vma));
+       eb.vma = (struct i915_vma **)(exec + args->buffer_count + 1);
+       eb.vma[0] = NULL;
        eb.flags = (unsigned int *)(eb.vma + args->buffer_count + 1);
 
        eb.invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;