drm/i915: inline vma_create into lookup_or_create_vma

In the execbuf code we don't clean up any vmas which ended up not
getting bound for code simplicity. To make sure that we don't end up
creating multiple vma for the same vm kill the somewhat dangerous
vma_create function and inline it into lookup_or_create.

This is just a safety measure to prevent surprises in the future.

Also update the somewhat confused comment in the execbuf code and
clarify what kind of magic is going on with a new one.

v2: Keep the function separate as requested by Chris. But give it a __
prefix for paranoia and move it tighter together with the other vma
stuff.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b6494b24098d87f92ebedcf111f0d0fa579b2a2a..d5cff971810327efa8cb3b18f2bf9b6fa4cb834e 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1826,8 +1826,6 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
 struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
                                                  size_t size);
 void i915_gem_free_object(struct drm_gem_object *obj);
-struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,
-                                    struct i915_address_space *vm);
 void i915_gem_vma_destroy(struct i915_vma *vma);
 
 int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj,

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c4c56f98d6e69069bbd680e3f2c722cc6d638227..6820d0a1231f569a3ffa12714822822350788a49 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4109,8 +4109,19 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
        i915_gem_object_free(obj);
 }
 
-struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,
+struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
                                     struct i915_address_space *vm)
+{
+       struct i915_vma *vma;
+       list_for_each_entry(vma, &obj->vma_list, vma_link)
+               if (vma->vm == vm)
+                       return vma;
+
+       return NULL;
+}
+
+static struct i915_vma *__i915_gem_vma_create(struct drm_i915_gem_object *obj,
+                                             struct i915_address_space *vm)
 {
        struct i915_vma *vma = kzalloc(sizeof(*vma), GFP_KERNEL);
        if (vma == NULL)
@@ -4131,6 +4142,19 @@ struct i915_vma *i915_gem_vma_create(struct drm_i915_gem_object *obj,
        return vma;
 }
 
+struct i915_vma *
+i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
+                                 struct i915_address_space *vm)
+{
+       struct i915_vma *vma;
+
+       vma = i915_gem_obj_to_vma(obj, vm);
+       if (!vma)
+               vma = __i915_gem_vma_create(obj, vm);
+
+       return vma;
+}
+
 void i915_gem_vma_destroy(struct i915_vma *vma)
 {
        WARN_ON(vma->node.allocated);
@@ -4857,27 +4881,3 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
 
        return 0;
 }
-
-struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
-                                    struct i915_address_space *vm)
-{
-       struct i915_vma *vma;
-       list_for_each_entry(vma, &obj->vma_list, vma_link)
-               if (vma->vm == vm)
-                       return vma;
-
-       return NULL;
-}
-
-struct i915_vma *
-i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
-                                 struct i915_address_space *vm)
-{
-       struct i915_vma *vma;
-
-       vma = i915_gem_obj_to_vma(obj, vm);
-       if (!vma)
-               vma = i915_gem_vma_create(obj, vm);
-
-       return vma;
-}

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 5dcfd4f59f87c7b1e287e02a8799eac1da9bbd80..93c8b28ff2e14cb913b05062188fd061e22df34f 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -123,11 +123,16 @@ eb_lookup_vmas(struct eb_vmas *eb,
        list_for_each_entry(obj, &objects, obj_exec_link) {
                struct i915_vma *vma;
 
+               /*
+                * 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_gem_obj_lookup_or_create_vma(obj, vm);
                if (IS_ERR(vma)) {
-                       /* XXX: We don't need an error path fro vma because if
-                        * the vma was created just for this execbuf, object
-                        * unreference should kill it off.*/
                        DRM_DEBUG("Failed to lookup VMA\n");
                        ret = PTR_ERR(vma);
                        goto out;

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 9969d10b80f518c6d032d45ff57cf2839afbd5bf..b902f2afc8e25275f1de78e54ec8ff20043aa88a 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -392,7 +392,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
        if (gtt_offset == I915_GTT_OFFSET_NONE)
                return obj;
 
-       vma = i915_gem_vma_create(obj, ggtt);
+       vma = i915_gem_obj_lookup_or_create_vma(obj, ggtt);
        if (IS_ERR(vma)) {
                ret = PTR_ERR(vma);
                goto err_out;