| Message ID | 20210715223900.1840576-4-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: > > Instead of hand-rolling the same three calls in each function, pull them > into an i915_gem_object_create_user helper. Apart from re-ordering of > the placements array ENOMEM check, the only functional change here > should be that i915_gem_dumb_create now calls i915_gem_flush_free_objects > which it probably should have been calling all along. > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > --- > drivers/gpu/drm/i915/gem/i915_gem_create.c | 106 +++++++++------------ > 1 file changed, 43 insertions(+), 63 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c > index 391c8c4a12172..69bf9ec777642 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c > @@ -11,13 +11,14 @@ > #include "i915_trace.h" > #include "i915_user_extensions.h" > > -static u32 object_max_page_size(struct drm_i915_gem_object *obj) > +static u32 object_max_page_size(struct intel_memory_region **placements, > + unsigned int n_placements) > { > u32 max_page_size = 0; > int i; > > - for (i = 0; i < obj->mm.n_placements; i++) { > - struct intel_memory_region *mr = obj->mm.placements[i]; > + for (i = 0; i < n_placements; i++) { > + struct intel_memory_region *mr = placements[i]; > > GEM_BUG_ON(!is_power_of_2(mr->min_page_size)); > max_page_size = max_t(u32, max_page_size, mr->min_page_size); > @@ -81,22 +82,35 @@ static int i915_gem_publish(struct drm_i915_gem_object *obj, > return 0; > } > > -static int > -i915_gem_setup(struct drm_i915_gem_object *obj, u64 size) > +static struct drm_i915_gem_object * > +i915_gem_object_create_user(struct drm_i915_private *i915, u64 size, create_user sounds nice. > + struct intel_memory_region **placements, > + unsigned int n_placements) > { > - struct intel_memory_region *mr = obj->mm.placements[0]; > + struct intel_memory_region *mr = placements[0]; > + struct drm_i915_gem_object *obj; > unsigned int flags; > int ret; > > - size = round_up(size, object_max_page_size(obj)); > + i915_gem_flush_free_objects(i915); Needs to be a separate patch. > + > + obj = i915_gem_object_alloc(); > + if (!obj) > + return ERR_PTR(-ENOMEM); Should move this way down, so we don't accidently leak it.
On Fri, Jul 16, 2021 at 6:21 AM Matthew Auld <matthew.william.auld@gmail.com> wrote: > > On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand <jason@jlekstrand.net> wrote: > > > > Instead of hand-rolling the same three calls in each function, pull them > > into an i915_gem_object_create_user helper. Apart from re-ordering of > > the placements array ENOMEM check, the only functional change here > > should be that i915_gem_dumb_create now calls i915_gem_flush_free_objects > > which it probably should have been calling all along. > > > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_create.c | 106 +++++++++------------ > > 1 file changed, 43 insertions(+), 63 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c > > index 391c8c4a12172..69bf9ec777642 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c > > @@ -11,13 +11,14 @@ > > #include "i915_trace.h" > > #include "i915_user_extensions.h" > > > > -static u32 object_max_page_size(struct drm_i915_gem_object *obj) > > +static u32 object_max_page_size(struct intel_memory_region **placements, > > + unsigned int n_placements) > > { > > u32 max_page_size = 0; > > int i; > > > > - for (i = 0; i < obj->mm.n_placements; i++) { > > - struct intel_memory_region *mr = obj->mm.placements[i]; > > + for (i = 0; i < n_placements; i++) { > > + struct intel_memory_region *mr = placements[i]; > > > > GEM_BUG_ON(!is_power_of_2(mr->min_page_size)); > > max_page_size = max_t(u32, max_page_size, mr->min_page_size); > > @@ -81,22 +82,35 @@ static int i915_gem_publish(struct drm_i915_gem_object *obj, > > return 0; > > } > > > > -static int > > -i915_gem_setup(struct drm_i915_gem_object *obj, u64 size) > > +static struct drm_i915_gem_object * > > +i915_gem_object_create_user(struct drm_i915_private *i915, u64 size, > > create_user sounds nice. > > > + struct intel_memory_region **placements, > > + unsigned int n_placements) > > { > > - struct intel_memory_region *mr = obj->mm.placements[0]; > > + struct intel_memory_region *mr = placements[0]; > > + struct drm_i915_gem_object *obj; > > unsigned int flags; > > int ret; > > > > - size = round_up(size, object_max_page_size(obj)); > > + i915_gem_flush_free_objects(i915); > > Needs to be a separate patch. Done. > > + > > + obj = i915_gem_object_alloc(); > > + if (!obj) > > + return ERR_PTR(-ENOMEM); > > Should move this way down, so we don't accidently leak it. Done. --Jason
On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand <jason@jlekstrand.net> wrote: > > Instead of hand-rolling the same three calls in each function, pull them > into an i915_gem_object_create_user helper. Apart from re-ordering of > the placements array ENOMEM check, the only functional change here > should be that i915_gem_dumb_create now calls i915_gem_flush_free_objects > which it probably should have been calling all along. > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > --- > drivers/gpu/drm/i915/gem/i915_gem_create.c | 106 +++++++++------------ > 1 file changed, 43 insertions(+), 63 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c > index 391c8c4a12172..69bf9ec777642 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c > @@ -11,13 +11,14 @@ > #include "i915_trace.h" > #include "i915_user_extensions.h" > > -static u32 object_max_page_size(struct drm_i915_gem_object *obj) > +static u32 object_max_page_size(struct intel_memory_region **placements, > + unsigned int n_placements) > { > u32 max_page_size = 0; > int i; > > - for (i = 0; i < obj->mm.n_placements; i++) { > - struct intel_memory_region *mr = obj->mm.placements[i]; > + for (i = 0; i < n_placements; i++) { > + struct intel_memory_region *mr = placements[i]; > > GEM_BUG_ON(!is_power_of_2(mr->min_page_size)); > max_page_size = max_t(u32, max_page_size, mr->min_page_size); > @@ -81,22 +82,35 @@ static int i915_gem_publish(struct drm_i915_gem_object *obj, > return 0; > } > > -static int > -i915_gem_setup(struct drm_i915_gem_object *obj, u64 size) > +static struct drm_i915_gem_object * > +i915_gem_object_create_user(struct drm_i915_private *i915, u64 size, > + struct intel_memory_region **placements, > + unsigned int n_placements) > { > - struct intel_memory_region *mr = obj->mm.placements[0]; > + struct intel_memory_region *mr = placements[0]; > + struct drm_i915_gem_object *obj; > unsigned int flags; > int ret; > > - size = round_up(size, object_max_page_size(obj)); > + i915_gem_flush_free_objects(i915); > + > + obj = i915_gem_object_alloc(); > + if (!obj) > + return ERR_PTR(-ENOMEM); > + > + size = round_up(size, object_max_page_size(placements, n_placements)); > if (size == 0) > - return -EINVAL; > + return ERR_PTR(-EINVAL); > > /* For most of the ABI (e.g. mmap) we think in system pages */ > GEM_BUG_ON(!IS_ALIGNED(size, PAGE_SIZE)); > > if (i915_gem_object_size_2big(size)) > - return -E2BIG; > + return ERR_PTR(-E2BIG); > + > + ret = object_set_placements(obj, placements, n_placements); > + if (ret) > + goto object_free; Thinking on this again, it might be way too thorny to expose create_user as-is to other parts of i915, like we do in the last patch. Since the caller will be expected to manually validate the placements, otherwise we might crash and burn in weird ways as new users pop up. i.e it needs the same validation that happens as part of the extension. Also as new extensions arrive, like with PXP, that also has to get bolted onto create_user, which might have its own hidden constraints.
On Tue, Jul 20, 2021 at 4:35 AM Matthew Auld <matthew.william.auld@gmail.com> wrote: > > On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand <jason@jlekstrand.net> wrote: > > > > Instead of hand-rolling the same three calls in each function, pull them > > into an i915_gem_object_create_user helper. Apart from re-ordering of > > the placements array ENOMEM check, the only functional change here > > should be that i915_gem_dumb_create now calls i915_gem_flush_free_objects > > which it probably should have been calling all along. > > > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_create.c | 106 +++++++++------------ > > 1 file changed, 43 insertions(+), 63 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c > > index 391c8c4a12172..69bf9ec777642 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c > > @@ -11,13 +11,14 @@ > > #include "i915_trace.h" > > #include "i915_user_extensions.h" > > > > -static u32 object_max_page_size(struct drm_i915_gem_object *obj) > > +static u32 object_max_page_size(struct intel_memory_region **placements, > > + unsigned int n_placements) > > { > > u32 max_page_size = 0; > > int i; > > > > - for (i = 0; i < obj->mm.n_placements; i++) { > > - struct intel_memory_region *mr = obj->mm.placements[i]; > > + for (i = 0; i < n_placements; i++) { > > + struct intel_memory_region *mr = placements[i]; > > > > GEM_BUG_ON(!is_power_of_2(mr->min_page_size)); > > max_page_size = max_t(u32, max_page_size, mr->min_page_size); > > @@ -81,22 +82,35 @@ static int i915_gem_publish(struct drm_i915_gem_object *obj, > > return 0; > > } > > > > -static int > > -i915_gem_setup(struct drm_i915_gem_object *obj, u64 size) > > +static struct drm_i915_gem_object * > > +i915_gem_object_create_user(struct drm_i915_private *i915, u64 size, > > + struct intel_memory_region **placements, > > + unsigned int n_placements) > > { > > - struct intel_memory_region *mr = obj->mm.placements[0]; > > + struct intel_memory_region *mr = placements[0]; > > + struct drm_i915_gem_object *obj; > > unsigned int flags; > > int ret; > > > > - size = round_up(size, object_max_page_size(obj)); > > + i915_gem_flush_free_objects(i915); > > + > > + obj = i915_gem_object_alloc(); > > + if (!obj) > > + return ERR_PTR(-ENOMEM); > > + > > + size = round_up(size, object_max_page_size(placements, n_placements)); > > if (size == 0) > > - return -EINVAL; > > + return ERR_PTR(-EINVAL); > > > > /* For most of the ABI (e.g. mmap) we think in system pages */ > > GEM_BUG_ON(!IS_ALIGNED(size, PAGE_SIZE)); > > > > if (i915_gem_object_size_2big(size)) > > - return -E2BIG; > > + return ERR_PTR(-E2BIG); > > + > > + ret = object_set_placements(obj, placements, n_placements); > > + if (ret) > > + goto object_free; > > Thinking on this again, it might be way too thorny to expose > create_user as-is to other parts of i915, like we do in the last > patch. Since the caller will be expected to manually validate the > placements, otherwise we might crash and burn in weird ways as new > users pop up. i.e it needs the same validation that happens as part of > the extension. Also as new extensions arrive, like with PXP, that also > has to get bolted onto create_user, which might have its own hidden > constraints. Perhaps. Do you have a suggestion for how to make it available to selftests without exposing it to "the rest of i915"? If you want, I can make create_user duplicate the placements uniqueness check. That's really the only validation currently in the ioctl besides all the stuff for making sure that the class/instance provided by the user isn't bogus. But if we've got real i915_memory_region pointers, we don't need that. --Jason
On Tue, 20 Jul 2021 at 23:04, Jason Ekstrand <jason@jlekstrand.net> wrote: > > On Tue, Jul 20, 2021 at 4:35 AM Matthew Auld > <matthew.william.auld@gmail.com> wrote: > > > > On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand <jason@jlekstrand.net> wrote: > > > > > > Instead of hand-rolling the same three calls in each function, pull them > > > into an i915_gem_object_create_user helper. Apart from re-ordering of > > > the placements array ENOMEM check, the only functional change here > > > should be that i915_gem_dumb_create now calls i915_gem_flush_free_objects > > > which it probably should have been calling all along. > > > > > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > > > --- > > > drivers/gpu/drm/i915/gem/i915_gem_create.c | 106 +++++++++------------ > > > 1 file changed, 43 insertions(+), 63 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c > > > index 391c8c4a12172..69bf9ec777642 100644 > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c > > > @@ -11,13 +11,14 @@ > > > #include "i915_trace.h" > > > #include "i915_user_extensions.h" > > > > > > -static u32 object_max_page_size(struct drm_i915_gem_object *obj) > > > +static u32 object_max_page_size(struct intel_memory_region **placements, > > > + unsigned int n_placements) > > > { > > > u32 max_page_size = 0; > > > int i; > > > > > > - for (i = 0; i < obj->mm.n_placements; i++) { > > > - struct intel_memory_region *mr = obj->mm.placements[i]; > > > + for (i = 0; i < n_placements; i++) { > > > + struct intel_memory_region *mr = placements[i]; > > > > > > GEM_BUG_ON(!is_power_of_2(mr->min_page_size)); > > > max_page_size = max_t(u32, max_page_size, mr->min_page_size); > > > @@ -81,22 +82,35 @@ static int i915_gem_publish(struct drm_i915_gem_object *obj, > > > return 0; > > > } > > > > > > -static int > > > -i915_gem_setup(struct drm_i915_gem_object *obj, u64 size) > > > +static struct drm_i915_gem_object * > > > +i915_gem_object_create_user(struct drm_i915_private *i915, u64 size, > > > + struct intel_memory_region **placements, > > > + unsigned int n_placements) > > > { > > > - struct intel_memory_region *mr = obj->mm.placements[0]; > > > + struct intel_memory_region *mr = placements[0]; > > > + struct drm_i915_gem_object *obj; > > > unsigned int flags; > > > int ret; > > > > > > - size = round_up(size, object_max_page_size(obj)); > > > + i915_gem_flush_free_objects(i915); > > > + > > > + obj = i915_gem_object_alloc(); > > > + if (!obj) > > > + return ERR_PTR(-ENOMEM); > > > + > > > + size = round_up(size, object_max_page_size(placements, n_placements)); > > > if (size == 0) > > > - return -EINVAL; > > > + return ERR_PTR(-EINVAL); > > > > > > /* For most of the ABI (e.g. mmap) we think in system pages */ > > > GEM_BUG_ON(!IS_ALIGNED(size, PAGE_SIZE)); > > > > > > if (i915_gem_object_size_2big(size)) > > > - return -E2BIG; > > > + return ERR_PTR(-E2BIG); > > > + > > > + ret = object_set_placements(obj, placements, n_placements); > > > + if (ret) > > > + goto object_free; > > > > Thinking on this again, it might be way too thorny to expose > > create_user as-is to other parts of i915, like we do in the last > > patch. Since the caller will be expected to manually validate the > > placements, otherwise we might crash and burn in weird ways as new > > users pop up. i.e it needs the same validation that happens as part of > > the extension. Also as new extensions arrive, like with PXP, that also > > has to get bolted onto create_user, which might have its own hidden > > constraints. > > Perhaps. Do you have a suggestion for how to make it available to > selftests without exposing it to "the rest of i915"? If you want, I > can make create_user duplicate the placements uniqueness check. > That's really the only validation currently in the ioctl besides all > the stuff for making sure that the class/instance provided by the user > isn't bogus. But if we've got real i915_memory_region pointers, we > don't need that. Yeah, I guess the concern here was duplicated placements(that would change the meaning of n_placements > 1), and then ofc regions not supported by the device. Also maybe stolen which doesn't have a TTM backend yet. If this is just for the selftests, doing what the mman selftests do with create_region + set_placements would be one option. Otherwise maybe just add __two_underscores and a big comment, for why you should be careful when using this? > > --Jason
On Wed, Jul 21, 2021 at 3:25 AM Matthew Auld <matthew.william.auld@gmail.com> wrote: > > On Tue, 20 Jul 2021 at 23:04, Jason Ekstrand <jason@jlekstrand.net> wrote: > > > > On Tue, Jul 20, 2021 at 4:35 AM Matthew Auld > > <matthew.william.auld@gmail.com> wrote: > > > > > > On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand <jason@jlekstrand.net> wrote: > > > > > > > > Instead of hand-rolling the same three calls in each function, pull them > > > > into an i915_gem_object_create_user helper. Apart from re-ordering of > > > > the placements array ENOMEM check, the only functional change here > > > > should be that i915_gem_dumb_create now calls i915_gem_flush_free_objects > > > > which it probably should have been calling all along. > > > > > > > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > > > > --- > > > > drivers/gpu/drm/i915/gem/i915_gem_create.c | 106 +++++++++------------ > > > > 1 file changed, 43 insertions(+), 63 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c > > > > index 391c8c4a12172..69bf9ec777642 100644 > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c > > > > @@ -11,13 +11,14 @@ > > > > #include "i915_trace.h" > > > > #include "i915_user_extensions.h" > > > > > > > > -static u32 object_max_page_size(struct drm_i915_gem_object *obj) > > > > +static u32 object_max_page_size(struct intel_memory_region **placements, > > > > + unsigned int n_placements) > > > > { > > > > u32 max_page_size = 0; > > > > int i; > > > > > > > > - for (i = 0; i < obj->mm.n_placements; i++) { > > > > - struct intel_memory_region *mr = obj->mm.placements[i]; > > > > + for (i = 0; i < n_placements; i++) { > > > > + struct intel_memory_region *mr = placements[i]; > > > > > > > > GEM_BUG_ON(!is_power_of_2(mr->min_page_size)); > > > > max_page_size = max_t(u32, max_page_size, mr->min_page_size); > > > > @@ -81,22 +82,35 @@ static int i915_gem_publish(struct drm_i915_gem_object *obj, > > > > return 0; > > > > } > > > > > > > > -static int > > > > -i915_gem_setup(struct drm_i915_gem_object *obj, u64 size) > > > > +static struct drm_i915_gem_object * > > > > +i915_gem_object_create_user(struct drm_i915_private *i915, u64 size, > > > > + struct intel_memory_region **placements, > > > > + unsigned int n_placements) > > > > { > > > > - struct intel_memory_region *mr = obj->mm.placements[0]; > > > > + struct intel_memory_region *mr = placements[0]; > > > > + struct drm_i915_gem_object *obj; > > > > unsigned int flags; > > > > int ret; > > > > > > > > - size = round_up(size, object_max_page_size(obj)); > > > > + i915_gem_flush_free_objects(i915); > > > > + > > > > + obj = i915_gem_object_alloc(); > > > > + if (!obj) > > > > + return ERR_PTR(-ENOMEM); > > > > + > > > > + size = round_up(size, object_max_page_size(placements, n_placements)); > > > > if (size == 0) > > > > - return -EINVAL; > > > > + return ERR_PTR(-EINVAL); > > > > > > > > /* For most of the ABI (e.g. mmap) we think in system pages */ > > > > GEM_BUG_ON(!IS_ALIGNED(size, PAGE_SIZE)); > > > > > > > > if (i915_gem_object_size_2big(size)) > > > > - return -E2BIG; > > > > + return ERR_PTR(-E2BIG); > > > > + > > > > + ret = object_set_placements(obj, placements, n_placements); > > > > + if (ret) > > > > + goto object_free; > > > > > > Thinking on this again, it might be way too thorny to expose > > > create_user as-is to other parts of i915, like we do in the last > > > patch. Since the caller will be expected to manually validate the > > > placements, otherwise we might crash and burn in weird ways as new > > > users pop up. i.e it needs the same validation that happens as part of > > > the extension. Also as new extensions arrive, like with PXP, that also > > > has to get bolted onto create_user, which might have its own hidden > > > constraints. > > > > Perhaps. Do you have a suggestion for how to make it available to > > selftests without exposing it to "the rest of i915"? If you want, I > > can make create_user duplicate the placements uniqueness check. > > That's really the only validation currently in the ioctl besides all > > the stuff for making sure that the class/instance provided by the user > > isn't bogus. But if we've got real i915_memory_region pointers, we > > don't need that. > > Yeah, I guess the concern here was duplicated placements(that would > change the meaning of n_placements > 1), and then ofc regions not > supported by the device. Also maybe stolen which doesn't have a TTM > backend yet. > > If this is just for the selftests, doing what the mman selftests do > with create_region + set_placements would be one option. Otherwise > maybe just add __two_underscores and a big comment, for why you > should be careful when using this? I've added __two_underscores and some kerneldoc. I also looked at adding some validation to that function. I'm happy to do so if you'd like but didn't want to add overhead if you thought __ and a big fat warning were enough. --Jason
On Wed, 21 Jul 2021 at 16:47, Jason Ekstrand <jason@jlekstrand.net> wrote: > > On Wed, Jul 21, 2021 at 3:25 AM Matthew Auld > <matthew.william.auld@gmail.com> wrote: > > > > On Tue, 20 Jul 2021 at 23:04, Jason Ekstrand <jason@jlekstrand.net> wrote: > > > > > > On Tue, Jul 20, 2021 at 4:35 AM Matthew Auld > > > <matthew.william.auld@gmail.com> wrote: > > > > > > > > On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand <jason@jlekstrand.net> wrote: > > > > > > > > > > Instead of hand-rolling the same three calls in each function, pull them > > > > > into an i915_gem_object_create_user helper. Apart from re-ordering of > > > > > the placements array ENOMEM check, the only functional change here > > > > > should be that i915_gem_dumb_create now calls i915_gem_flush_free_objects > > > > > which it probably should have been calling all along. > > > > > > > > > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > > > > > --- > > > > > drivers/gpu/drm/i915/gem/i915_gem_create.c | 106 +++++++++------------ > > > > > 1 file changed, 43 insertions(+), 63 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c > > > > > index 391c8c4a12172..69bf9ec777642 100644 > > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c > > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c > > > > > @@ -11,13 +11,14 @@ > > > > > #include "i915_trace.h" > > > > > #include "i915_user_extensions.h" > > > > > > > > > > -static u32 object_max_page_size(struct drm_i915_gem_object *obj) > > > > > +static u32 object_max_page_size(struct intel_memory_region **placements, > > > > > + unsigned int n_placements) > > > > > { > > > > > u32 max_page_size = 0; > > > > > int i; > > > > > > > > > > - for (i = 0; i < obj->mm.n_placements; i++) { > > > > > - struct intel_memory_region *mr = obj->mm.placements[i]; > > > > > + for (i = 0; i < n_placements; i++) { > > > > > + struct intel_memory_region *mr = placements[i]; > > > > > > > > > > GEM_BUG_ON(!is_power_of_2(mr->min_page_size)); > > > > > max_page_size = max_t(u32, max_page_size, mr->min_page_size); > > > > > @@ -81,22 +82,35 @@ static int i915_gem_publish(struct drm_i915_gem_object *obj, > > > > > return 0; > > > > > } > > > > > > > > > > -static int > > > > > -i915_gem_setup(struct drm_i915_gem_object *obj, u64 size) > > > > > +static struct drm_i915_gem_object * > > > > > +i915_gem_object_create_user(struct drm_i915_private *i915, u64 size, > > > > > + struct intel_memory_region **placements, > > > > > + unsigned int n_placements) > > > > > { > > > > > - struct intel_memory_region *mr = obj->mm.placements[0]; > > > > > + struct intel_memory_region *mr = placements[0]; > > > > > + struct drm_i915_gem_object *obj; > > > > > unsigned int flags; > > > > > int ret; > > > > > > > > > > - size = round_up(size, object_max_page_size(obj)); > > > > > + i915_gem_flush_free_objects(i915); > > > > > + > > > > > + obj = i915_gem_object_alloc(); > > > > > + if (!obj) > > > > > + return ERR_PTR(-ENOMEM); > > > > > + > > > > > + size = round_up(size, object_max_page_size(placements, n_placements)); > > > > > if (size == 0) > > > > > - return -EINVAL; > > > > > + return ERR_PTR(-EINVAL); > > > > > > > > > > /* For most of the ABI (e.g. mmap) we think in system pages */ > > > > > GEM_BUG_ON(!IS_ALIGNED(size, PAGE_SIZE)); > > > > > > > > > > if (i915_gem_object_size_2big(size)) > > > > > - return -E2BIG; > > > > > + return ERR_PTR(-E2BIG); > > > > > + > > > > > + ret = object_set_placements(obj, placements, n_placements); > > > > > + if (ret) > > > > > + goto object_free; > > > > > > > > Thinking on this again, it might be way too thorny to expose > > > > create_user as-is to other parts of i915, like we do in the last > > > > patch. Since the caller will be expected to manually validate the > > > > placements, otherwise we might crash and burn in weird ways as new > > > > users pop up. i.e it needs the same validation that happens as part of > > > > the extension. Also as new extensions arrive, like with PXP, that also > > > > has to get bolted onto create_user, which might have its own hidden > > > > constraints. > > > > > > Perhaps. Do you have a suggestion for how to make it available to > > > selftests without exposing it to "the rest of i915"? If you want, I > > > can make create_user duplicate the placements uniqueness check. > > > That's really the only validation currently in the ioctl besides all > > > the stuff for making sure that the class/instance provided by the user > > > isn't bogus. But if we've got real i915_memory_region pointers, we > > > don't need that. > > > > Yeah, I guess the concern here was duplicated placements(that would > > change the meaning of n_placements > 1), and then ofc regions not > > supported by the device. Also maybe stolen which doesn't have a TTM > > backend yet. > > > > If this is just for the selftests, doing what the mman selftests do > > with create_region + set_placements would be one option. Otherwise > > maybe just add __two_underscores and a big comment, for why you > > should be careful when using this? > > I've added __two_underscores and some kerneldoc. I also looked at > adding some validation to that function. I'm happy to do so if you'd > like but didn't want to add overhead if you thought __ and a big fat > warning were enough. __two_underscores and a comment should be fine for now. > > --Jason
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c index 391c8c4a12172..69bf9ec777642 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c @@ -11,13 +11,14 @@ #include "i915_trace.h" #include "i915_user_extensions.h" -static u32 object_max_page_size(struct drm_i915_gem_object *obj) +static u32 object_max_page_size(struct intel_memory_region **placements, + unsigned int n_placements) { u32 max_page_size = 0; int i; - for (i = 0; i < obj->mm.n_placements; i++) { - struct intel_memory_region *mr = obj->mm.placements[i]; + for (i = 0; i < n_placements; i++) { + struct intel_memory_region *mr = placements[i]; GEM_BUG_ON(!is_power_of_2(mr->min_page_size)); max_page_size = max_t(u32, max_page_size, mr->min_page_size); @@ -81,22 +82,35 @@ static int i915_gem_publish(struct drm_i915_gem_object *obj, return 0; } -static int -i915_gem_setup(struct drm_i915_gem_object *obj, u64 size) +static struct drm_i915_gem_object * +i915_gem_object_create_user(struct drm_i915_private *i915, u64 size, + struct intel_memory_region **placements, + unsigned int n_placements) { - struct intel_memory_region *mr = obj->mm.placements[0]; + struct intel_memory_region *mr = placements[0]; + struct drm_i915_gem_object *obj; unsigned int flags; int ret; - size = round_up(size, object_max_page_size(obj)); + i915_gem_flush_free_objects(i915); + + obj = i915_gem_object_alloc(); + if (!obj) + return ERR_PTR(-ENOMEM); + + size = round_up(size, object_max_page_size(placements, n_placements)); if (size == 0) - return -EINVAL; + return ERR_PTR(-EINVAL); /* For most of the ABI (e.g. mmap) we think in system pages */ GEM_BUG_ON(!IS_ALIGNED(size, PAGE_SIZE)); if (i915_gem_object_size_2big(size)) - return -E2BIG; + return ERR_PTR(-E2BIG); + + ret = object_set_placements(obj, placements, n_placements); + if (ret) + goto object_free; /* * I915_BO_ALLOC_USER will make sure the object is cleared before @@ -106,12 +120,18 @@ i915_gem_setup(struct drm_i915_gem_object *obj, u64 size) ret = mr->ops->init_object(mr, obj, size, 0, flags); if (ret) - return ret; + goto object_free; GEM_BUG_ON(size != obj->base.size); trace_i915_gem_object_create(obj); - return 0; + return obj; + +object_free: + if (obj->mm.n_placements > 1) + kfree(obj->mm.placements); + i915_gem_object_free(obj); + return ERR_PTR(ret); } int @@ -124,7 +144,6 @@ i915_gem_dumb_create(struct drm_file *file, enum intel_memory_type mem_type; int cpp = DIV_ROUND_UP(args->bpp, 8); u32 format; - int ret; switch (cpp) { case 1: @@ -157,24 +176,13 @@ i915_gem_dumb_create(struct drm_file *file, if (HAS_LMEM(to_i915(dev))) mem_type = INTEL_MEMORY_LOCAL; - obj = i915_gem_object_alloc(); - if (!obj) - return -ENOMEM; - mr = intel_memory_region_by_type(to_i915(dev), mem_type); - ret = object_set_placements(obj, &mr, 1); - if (ret) - goto object_free; - ret = i915_gem_setup(obj, args->size); - if (ret) - goto object_free; + obj = i915_gem_object_create_user(to_i915(dev), args->size, &mr, 1); + if (IS_ERR(obj)) + return PTR_ERR(obj); return i915_gem_publish(obj, file, &args->size, &args->handle); - -object_free: - i915_gem_object_free(obj); - return ret; } /** @@ -191,28 +199,14 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data, struct drm_i915_gem_create *args = data; struct drm_i915_gem_object *obj; struct intel_memory_region *mr; - int ret; - - i915_gem_flush_free_objects(i915); - - obj = i915_gem_object_alloc(); - if (!obj) - return -ENOMEM; mr = intel_memory_region_by_type(i915, INTEL_MEMORY_SYSTEM); - ret = object_set_placements(obj, &mr, 1); - if (ret) - goto object_free; - ret = i915_gem_setup(obj, args->size); - if (ret) - goto object_free; + obj = i915_gem_object_create_user(i915, args->size, &mr, 1); + if (IS_ERR(obj)) + return PTR_ERR(obj); return i915_gem_publish(obj, file, &args->size, &args->handle); - -object_free: - i915_gem_object_free(obj); - return ret; } #define MAX_N_PLACEMENTS = (INTEL_REGION_UNKNOWN + 1) @@ -379,38 +373,24 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data, if (args->flags) return -EINVAL; - i915_gem_flush_free_objects(i915); - - obj = i915_gem_object_alloc(); - if (!obj) - return -ENOMEM; - ret = i915_user_extensions(u64_to_user_ptr(args->extensions), create_extensions, ARRAY_SIZE(create_extensions), &ext_data); if (ret) - goto object_free; + return ret; if (!ext_data.n_placements) { ext_data.placements[0] = intel_memory_region_by_type(i915, INTEL_MEMORY_SYSTEM); 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) - goto object_free; + obj = i915_gem_object_create_user(i915, args->size, + ext_data.placements, + ext_data.n_placements); + if (IS_ERR(obj)) + return PTR_ERR(obj); return i915_gem_publish(obj, file, &args->size, &args->handle); - -object_free: - if (obj->mm.n_placements > 1) - kfree(obj->mm.placements); - i915_gem_object_free(obj); - return ret; }
Instead of hand-rolling the same three calls in each function, pull them into an i915_gem_object_create_user helper. Apart from re-ordering of the placements array ENOMEM check, the only functional change here should be that i915_gem_dumb_create now calls i915_gem_flush_free_objects which it probably should have been calling all along. Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> --- drivers/gpu/drm/i915/gem/i915_gem_create.c | 106 +++++++++------------ 1 file changed, 43 insertions(+), 63 deletions(-)