drm/i915/gem: Refactor placement setup for i915_gem_object_create* (v2)

Since we don't allow changing the set of regions after creation, we can
make ext_set_placements() build up the region set directly in the
create_ext and assign it to the object later.  This is similar to what
we did for contexts with the proto-context only simpler because there's
no funny object shuffling.  This will be used in the next patch to allow
us to de-duplicate a bunch of code.  Also, since we know the maximum
number of regions up-front, we can use a fixed-size temporary array for
the regions.  This simplifies memory management a bit for this new
delayed approach.

v2 (Matthew Auld):
 - Get rid of MAX_N_PLACEMENTS
 - Drop kfree(placements) from set_placements()
v3 (Matthew Auld):
 - Properly set ext_data->n_placements

Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20210723172142.3273510-3-jason@jlekstrand.net

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c
index 51f92e4b1a69d0d521de833d80ca8866f6d7aa5f..aa687b10dcd455cbb273addde20301d868318331 100644 (file)
--- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
@@ -27,10 +27,13 @@ static u32 object_max_page_size(struct drm_i915_gem_object *obj)
        return max_page_size;
 }
 
-static void object_set_placements(struct drm_i915_gem_object *obj,
-                                 struct intel_memory_region **placements,
-                                 unsigned int n_placements)
+static int object_set_placements(struct drm_i915_gem_object *obj,
+                                struct intel_memory_region **placements,
+                                unsigned int n_placements)
 {
+       struct intel_memory_region **arr;
+       unsigned int i;
+
        GEM_BUG_ON(!n_placements);
 
        /*
@@ -44,9 +47,20 @@ static void object_set_placements(struct drm_i915_gem_object *obj,
                obj->mm.placements = &i915->mm.regions[mr->id];
                obj->mm.n_placements = 1;
        } else {
-               obj->mm.placements = placements;
+               arr = kmalloc_array(n_placements,
+                                   sizeof(struct intel_memory_region *),
+                                   GFP_KERNEL);
+               if (!arr)
+                       return -ENOMEM;
+
+               for (i = 0; i < n_placements; i++)
+                       arr[i] = placements[i];
+
+               obj->mm.placements = arr;
                obj->mm.n_placements = n_placements;
        }
+
+       return 0;
 }
 
 static int i915_gem_publish(struct drm_i915_gem_object *obj,
@@ -148,7 +162,9 @@ i915_gem_dumb_create(struct drm_file *file,
                return -ENOMEM;
 
        mr = intel_memory_region_by_type(to_i915(dev), mem_type);
-       object_set_placements(obj, &mr, 1);
+       ret = object_set_placements(obj, &mr, 1);
+       if (ret)
+               goto object_free;
 
        ret = i915_gem_setup(obj, args->size);
        if (ret)
@@ -184,7 +200,9 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
                return -ENOMEM;
 
        mr = intel_memory_region_by_type(i915, INTEL_MEMORY_SYSTEM);
-       object_set_placements(obj, &mr, 1);
+       ret = object_set_placements(obj, &mr, 1);
+       if (ret)
+               goto object_free;
 
        ret = i915_gem_setup(obj, args->size);
        if (ret)
@@ -199,7 +217,8 @@ object_free:
 
 struct create_ext {
        struct drm_i915_private *i915;
-       struct drm_i915_gem_object *vanilla_object;
+       struct intel_memory_region *placements[INTEL_REGION_UNKNOWN];
+       unsigned int n_placements;
 };
 
 static void repr_placements(char *buf, size_t size,
@@ -230,8 +249,7 @@ static int set_placements(struct drm_i915_gem_create_ext_memory_regions *args,
        struct drm_i915_private *i915 = ext_data->i915;
        struct drm_i915_gem_memory_class_instance __user *uregions =
                u64_to_user_ptr(args->regions);
-       struct drm_i915_gem_object *obj = ext_data->vanilla_object;
-       struct intel_memory_region **placements;
+       struct intel_memory_region *placements[INTEL_REGION_UNKNOWN];
        u32 mask;
        int i, ret = 0;
 
@@ -245,6 +263,8 @@ static int set_placements(struct drm_i915_gem_create_ext_memory_regions *args,
                ret = -EINVAL;
        }
 
+       BUILD_BUG_ON(ARRAY_SIZE(i915->mm.regions) != ARRAY_SIZE(placements));
+       BUILD_BUG_ON(ARRAY_SIZE(ext_data->placements) != ARRAY_SIZE(placements));
        if (args->num_regions > ARRAY_SIZE(i915->mm.regions)) {
                drm_dbg(&i915->drm, "num_regions is too large\n");
                ret = -EINVAL;
@@ -253,21 +273,13 @@ static int set_placements(struct drm_i915_gem_create_ext_memory_regions *args,
        if (ret)
                return ret;
 
-       placements = kmalloc_array(args->num_regions,
-                                  sizeof(struct intel_memory_region *),
-                                  GFP_KERNEL);
-       if (!placements)
-               return -ENOMEM;
-
        mask = 0;
        for (i = 0; i < args->num_regions; i++) {
                struct drm_i915_gem_memory_class_instance region;
                struct intel_memory_region *mr;
 
-               if (copy_from_user(&region, uregions, sizeof(region))) {
-                       ret = -EFAULT;
-                       goto out_free;
-               }
+               if (copy_from_user(&region, uregions, sizeof(region)))
+                       return -EFAULT;
 
                mr = intel_memory_region_lookup(i915,
                                                region.memory_class,
@@ -293,14 +305,14 @@ static int set_placements(struct drm_i915_gem_create_ext_memory_regions *args,
                ++uregions;
        }
 
-       if (obj->mm.placements) {
+       if (ext_data->n_placements) {
                ret = -EINVAL;
                goto out_dump;
        }
 
-       object_set_placements(obj, placements, args->num_regions);
-       if (args->num_regions == 1)
-               kfree(placements);
+       ext_data->n_placements = args->num_regions;
+       for (i = 0; i < args->num_regions; i++)
+               ext_data->placements[i] = placements[i];
 
        return 0;
 
@@ -308,11 +320,11 @@ out_dump:
        if (1) {
                char buf[256];
 
-               if (obj->mm.placements) {
+               if (ext_data->n_placements) {
                        repr_placements(buf,
                                        sizeof(buf),
-                                       obj->mm.placements,
-                                       obj->mm.n_placements);
+                                       ext_data->placements,
+                                       ext_data->n_placements);
                        drm_dbg(&i915->drm,
                                "Placements were already set in previous EXT. Existing placements: %s\n",
                                buf);
@@ -322,8 +334,6 @@ out_dump:
                drm_dbg(&i915->drm, "New placements(so far validated): %s\n", buf);
        }
 
-out_free:
-       kfree(placements);
        return ret;
 }
 
@@ -358,7 +368,6 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
        struct drm_i915_private *i915 = to_i915(dev);
        struct drm_i915_gem_create_ext *args = data;
        struct create_ext ext_data = { .i915 = i915 };
-       struct intel_memory_region **placements_ext;
        struct drm_i915_gem_object *obj;
        int ret;
 
@@ -371,21 +380,22 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
        if (!obj)
                return -ENOMEM;
 
-       ext_data.vanilla_object = obj;
        ret = i915_user_extensions(u64_to_user_ptr(args->extensions),
                                   create_extensions,
                                   ARRAY_SIZE(create_extensions),
                                   &ext_data);
-       placements_ext = obj->mm.placements;
        if (ret)
                goto object_free;
 
-       if (!placements_ext) {
-               struct intel_memory_region *mr =
+       if (!ext_data.n_placements) {
+               ext_data.placements[0] =
                        intel_memory_region_by_type(i915, INTEL_MEMORY_SYSTEM);
-
-               object_set_placements(obj, &mr, 1);
+               ext_data.n_placements = 1;
        }
+       ret = object_set_placements(obj, ext_data.placements,
+                                   ext_data.n_placements);
+       if (ret)
+               goto object_free;
 
        ret = i915_gem_setup(obj, args->size);
        if (ret)
@@ -395,7 +405,7 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
 
 object_free:
        if (obj->mm.n_placements > 1)
-               kfree(placements_ext);
+               kfree(obj->mm.placements);
        i915_gem_object_free(obj);
        return ret;
 }