| Message ID | 20210715223900.1840576-3-jason@jlekstrand.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Migrate memory to SMEM when imported cross-device | expand |
On Thu, 15 Jul 2021 at 23:39, 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. > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> I think the plan was to have each extension directly hang/apply their state on the vanilla/proto object, or at least that was the idea behind: 357814f878f9 ("drm/i915: rework gem_create flow for upcoming extensions"). A previous version looked similar to this IIRC. > --- > drivers/gpu/drm/i915/gem/i915_gem_create.c | 75 +++++++++++++--------- > 1 file changed, 45 insertions(+), 30 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..391c8c4a12172 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) > @@ -197,9 +215,12 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data, > return ret; > } > > +#define MAX_N_PLACEMENTS = (INTEL_REGION_UNKNOWN + 1) Unused? > + > 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 +251,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 +265,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,12 +275,6 @@ 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; > @@ -293,14 +309,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]; kfree(placements) is still in the error unwind?
On Fri, Jul 16, 2021 at 6:12 AM Matthew Auld <matthew.william.auld@gmail.com> wrote: > > On Thu, 15 Jul 2021 at 23:39, 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. > > > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > > I think the plan was to have each extension directly hang/apply their > state on the vanilla/proto object, or at least that was the idea > behind: 357814f878f9 ("drm/i915: rework gem_create flow for upcoming > extensions"). A previous version looked similar to this IIRC. Right. In my mind, struct create_extis that proto-obj, more-or-less. If we wanted to codify that somehow, we could rename it i915_gem_object_create_params and make _create_user take one instead of separate parameters. With only having three right now, it didn't seem necessary to go that far. > > --- > > drivers/gpu/drm/i915/gem/i915_gem_create.c | 75 +++++++++++++--------- > > 1 file changed, 45 insertions(+), 30 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..391c8c4a12172 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) > > @@ -197,9 +215,12 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data, > > return ret; > > } > > > > +#define MAX_N_PLACEMENTS = (INTEL_REGION_UNKNOWN + 1) > > Unused? Yup. Left-over from an earlier version. Dropped. > > + > > 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 +251,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 +265,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,12 +275,6 @@ 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; > > @@ -293,14 +309,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]; > > kfree(placements) is still in the error unwind? Yup. Fixed. Thanks! --Jason
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c index 51f92e4b1a69d..391c8c4a12172 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) @@ -197,9 +215,12 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data, return ret; } +#define MAX_N_PLACEMENTS = (INTEL_REGION_UNKNOWN + 1) + 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 +251,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 +265,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,12 +275,6 @@ 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; @@ -293,14 +309,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]; return 0; @@ -308,11 +323,11 @@ static int set_placements(struct drm_i915_gem_create_ext_memory_regions *args, 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); @@ -358,7 +373,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 +385,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 +410,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; }
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. Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> --- drivers/gpu/drm/i915/gem/i915_gem_create.c | 75 +++++++++++++--------- 1 file changed, 45 insertions(+), 30 deletions(-)