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