| Message ID | 20210716141426.1904528-3-jason@jlekstrand.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Migrate memory to SMEM when imported cross-device (v7) | expand |
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> If CI is happy, Reviewed-by: Matthew Auld <matthew.auld@intel.com>
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.
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?
On Tue, 20 Jul 2021 at 23:07, Jason Ekstrand <jason@jlekstrand.net> wrote: > > 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? Pre-merge results for this series: igt@gem_create@create-ext-placement-sanity-check: shard-skl: PASS -> FAIL +1 similar issue shard-apl: NOTRUN -> FAIL shard-glk: PASS -> FAIL shard-iclb: PASS -> FAIL shard-kbl: PASS -> FAIL shard-tglb: NOTRUN -> FAIL
On Wed, Jul 21, 2021 at 3:32 AM Matthew Auld <matthew.william.auld@gmail.com> wrote: > > On Tue, 20 Jul 2021 at 23:07, Jason Ekstrand <jason@jlekstrand.net> wrote: > > > > 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? > > Pre-merge results for this series: > > igt@gem_create@create-ext-placement-sanity-check: > > shard-skl: PASS -> FAIL +1 similar issue > shard-apl: NOTRUN -> FAIL > shard-glk: PASS -> FAIL > shard-iclb: PASS -> FAIL > shard-kbl: PASS -> FAIL > shard-tglb: NOTRUN -> FAIL Yup. That was it. Thanks! Not sure why I didn't notice those fails.... --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..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]; return 0; @@ -308,11 +319,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); @@ -322,8 +333,6 @@ static int set_placements(struct drm_i915_gem_create_ext_memory_regions *args, drm_dbg(&i915->drm, "New placements(so far validated): %s\n", buf); } -out_free: - kfree(placements); return ret; } @@ -358,7 +367,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 +379,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 +404,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. 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(-)