On Mon, Jul 19, 2021 at 3:18 AM Matthew Auld matthew.william.auld@gmail.com wrote:
On Fri, 16 Jul 2021 at 15:14, Jason Ekstrand jason@jlekstrand.net wrote:
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()
Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Matthew Auld matthew.auld@intel.com
drivers/gpu/drm/i915/gem/i915_gem_create.c | 81 ++++++++++++---------- 1 file changed, 45 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c index 51f92e4b1a69d..5766749a449c0 100644 --- 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 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
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(®ion, uregions, sizeof(region))) {ret = -EFAULT;goto out_free;}
if (copy_from_user(®ion, uregions, sizeof(region)))return -EFAULT; mr = intel_memory_region_lookup(i915, region.memory_class,@@ -293,14 +305,13 @@ 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);
for (i = 0; i < args->num_regions; i++)ext_data->placements[i] = placements[i];I guess here we forget to set the ext_data->n_placements, which would explain the CI failure.
What CI failure are you referring to?