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

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 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, 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, 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

On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand jason@jlekstrand.net wrote:

__i915_gem_ttm_object_init() was ignoring the placement requests coming from the client and always placing all BOs in SMEM upon creation. Instead, compute the requested placement set from the object and pass that into ttm_bo_init_reserved().

AFAIK this is intentional, where we use SYS as a safe placeholder specifically for object creation, since that avoids allocating any actual pages. Later at get_pages() time we do the real allocation, where we need to consider the placements.

If we set the real placements here, the ttm_bo_validate() call in init_reserved() might allocate the backing pages here for the lmem-only case, which is not what we want in i915.

Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: Matthew Auld matthew.auld@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com

---

drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 6589411396d3f..d30f274c329c7 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -898,6 +898,8 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, { static struct lock_class_key lock_class; struct drm_i915_private *i915 = mem->i915;

•   struct ttm_place requested, busy[I915_TTM_MAX_PLACEMENTS];
  

•   struct ttm_placement placement;
    struct ttm_operation_ctx ctx = {
            .interruptible = true,
            .no_wait_gpu = false,
  

@@ -919,6 +921,9 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, /* Forcing the page size is kernel internal only */ GEM_BUG_ON(page_size && obj->mm.n_placements);

•   GEM_BUG_ON(obj->mm.n_placements > I915_TTM_MAX_PLACEMENTS);
  

•   i915_ttm_placement_from_obj(obj, &requested, busy, &placement);
  

•   /*
     * If this function fails, it will call the destructor, but
     * our caller still owns the object. So no freeing in the
  

@@ -927,8 +932,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, * until successful initialization. */ ret = ttm_bo_init_reserved(&i915->bdev, i915_gem_to_ttm(obj), size,

•                              bo_type, &i915_sys_placement,
  

•                              page_size >> PAGE_SHIFT,
  

•                              bo_type, &placement, page_size >> PAGE_SHIFT,
                               &ctx, NULL, NULL, i915_ttm_bo_destroy);
    if (ret)
            return i915_ttm_err_to_gem(ret);
  

-- 2.31.1

On Fri, Jul 16, 2021 at 8:54 AM Matthew Auld matthew.william.auld@gmail.com wrote:

On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand jason@jlekstrand.net wrote:

...

Whenever we had a user object (n_placements > 0), we were ignoring obj->mm.region and always putting obj->placements[0] as the requested region. For LMEM+SMEM objects, this was causing them to get shoved into LMEM on every i915_ttm_get_pages() even when SMEM was requested by, say, i915_gem_object_migrate().

i915_ttm_migrate calls i915_ttm_place_from_region() directly with the requested region, so there shouldn't be an issue with migration right? Do you have some more details?

With i915_ttm_migrate directly, no. But, in the last patch in the series, we're trying to migrate LMEM+SMEM buffers into SMEM on attach() and pin it there. This blows up in a very unexpected (IMO) way. The flow goes something like this:

- Client attempts a dma-buf import from another device - In attach() we call i915_gem_object_migrate() which calls i915_ttm_migrate() which migrates as requested. - Once the migration is complete, we call i915_gem_object_pin_pages() which calls i915_ttm_get_pages() which depends on i915_ttm_placement_from_obj() and so migrates it right back to LMEM.

Maybe the problem here is actually that our TTM code isn't respecting obj->mm.pages_pin_count?

In case you can't tell, I really have no clue what I'm doing here. I'm really stumbling around in the dark finding things that make my bug go away. I'm happy for the feedback.

--Jason

...

Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: Matthew Auld matthew.auld@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com

---

drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index d30f274c329c7..5985e994d56cf 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -150,8 +150,7 @@ i915_ttm_placement_from_obj(const struct drm_i915_gem_object *obj, unsigned int i;

    placement->num_placement = 1;

•   i915_ttm_place_from_region(num_allowed ? obj->mm.placements[0] :
  

•                              obj->mm.region, requested, flags);
  

•   i915_ttm_place_from_region(obj->mm.region, requested, flags);
  
    /* Cache this on object? */
    placement->num_busy_placement = num_allowed;
  

-- 2.31.1

On Fri, 16 Jul 2021 at 16:52, Matthew Auld matthew.william.auld@gmail.com wrote:

On Fri, 16 Jul 2021 at 15:10, Jason Ekstrand jason@jlekstrand.net wrote:

...

On Fri, Jul 16, 2021 at 8:54 AM Matthew Auld matthew.william.auld@gmail.com wrote:

...

On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand jason@jlekstrand.net wrote:

...

Whenever we had a user object (n_placements > 0), we were ignoring obj->mm.region and always putting obj->placements[0] as the requested region. For LMEM+SMEM objects, this was causing them to get shoved into LMEM on every i915_ttm_get_pages() even when SMEM was requested by, say, i915_gem_object_migrate().

i915_ttm_migrate calls i915_ttm_place_from_region() directly with the requested region, so there shouldn't be an issue with migration right? Do you have some more details?

With i915_ttm_migrate directly, no. But, in the last patch in the series, we're trying to migrate LMEM+SMEM buffers into SMEM on attach() and pin it there. This blows up in a very unexpected (IMO) way. The flow goes something like this:

• Client attempts a dma-buf import from another device
• In attach() we call i915_gem_object_migrate() which calls

i915_ttm_migrate() which migrates as requested.

• Once the migration is complete, we call i915_gem_object_pin_pages()

which calls i915_ttm_get_pages() which depends on i915_ttm_placement_from_obj() and so migrates it right back to LMEM.

The mm.pages must be NULL here, otherwise it would just increment the pages_pin_count?

...

Maybe the problem here is actually that our TTM code isn't respecting obj->mm.pages_pin_count?

I think if the resource is moved, we always nuke the mm.pages after being notified of the move. Also TTM is also not allowed to move pinned buffers.

I guess if we are evicted/swapped, so assuming we are not holding the object lock, and it's not pinned, the future call to get_pages() will see mm.pages = NULL, even though the ttm_resource is still there, and because we prioritise the placements[0], instead of mm.region we end up moving it for no good reason. But in your case you are holding the lock, or it's pinned? Also is this just with the selftest, or something real?

Or at least in the selftest I see ____i915_gem_object_get_pages() which doesn't even consider the mm.pages AFAIK.

...

In case you can't tell, I really have no clue what I'm doing here. I'm really stumbling around in the dark finding things that make my bug go away. I'm happy for the feedback.

--Jason

...

...

Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: Matthew Auld matthew.auld@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com

---

drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index d30f274c329c7..5985e994d56cf 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -150,8 +150,7 @@ i915_ttm_placement_from_obj(const struct drm_i915_gem_object *obj, unsigned int i;

    placement->num_placement = 1;

•   i915_ttm_place_from_region(num_allowed ? obj->mm.placements[0] :
  

•                              obj->mm.region, requested, flags);
  

•   i915_ttm_place_from_region(obj->mm.region, requested, flags);
  
    /* Cache this on object? */
    placement->num_busy_placement = num_allowed;
  

-- 2.31.1

On Fri, 16 Jul 2021 at 18:39, Jason Ekstrand jason@jlekstrand.net wrote:

On Fri, Jul 16, 2021 at 11:00 AM Matthew Auld matthew.william.auld@gmail.com wrote:

...

On Fri, 16 Jul 2021 at 16:52, Matthew Auld matthew.william.auld@gmail.com wrote:

...

On Fri, 16 Jul 2021 at 15:10, Jason Ekstrand jason@jlekstrand.net wrote:

...

On Fri, Jul 16, 2021 at 8:54 AM Matthew Auld matthew.william.auld@gmail.com wrote:

...

On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand jason@jlekstrand.net wrote:

...

Whenever we had a user object (n_placements > 0), we were ignoring obj->mm.region and always putting obj->placements[0] as the requested region. For LMEM+SMEM objects, this was causing them to get shoved into LMEM on every i915_ttm_get_pages() even when SMEM was requested by, say, i915_gem_object_migrate().

i915_ttm_migrate calls i915_ttm_place_from_region() directly with the requested region, so there shouldn't be an issue with migration right? Do you have some more details?

With i915_ttm_migrate directly, no. But, in the last patch in the series, we're trying to migrate LMEM+SMEM buffers into SMEM on attach() and pin it there. This blows up in a very unexpected (IMO) way. The flow goes something like this:

• Client attempts a dma-buf import from another device
• In attach() we call i915_gem_object_migrate() which calls

i915_ttm_migrate() which migrates as requested.

• Once the migration is complete, we call i915_gem_object_pin_pages()

which calls i915_ttm_get_pages() which depends on i915_ttm_placement_from_obj() and so migrates it right back to LMEM.

The mm.pages must be NULL here, otherwise it would just increment the pages_pin_count?

Given that the test is using the ____four_underscores version, it doesn't have that check. However, this executes after we've done the dma-buf import which pinned pages. So we should definitely have pages.

We shouldn't call ____four_underscores() if we might already have pages though. Under non-TTM that would leak the pages, and in TTM we might hit the WARN_ON(mm->pages) in __i915_ttm_get_pages(), if for example nothing was moved. I take it we can't just call pin_pages()? Four scary underscores usually means "don't call this in normal code".

...

...

...

Maybe the problem here is actually that our TTM code isn't respecting obj->mm.pages_pin_count?

I think if the resource is moved, we always nuke the mm.pages after being notified of the move. Also TTM is also not allowed to move pinned buffers.

I guess if we are evicted/swapped, so assuming we are not holding the object lock, and it's not pinned, the future call to get_pages() will see mm.pages = NULL, even though the ttm_resource is still there, and because we prioritise the placements[0], instead of mm.region we end up moving it for no good reason. But in your case you are holding the lock, or it's pinned? Also is this just with the selftest, or something real?

Or at least in the selftest I see ____i915_gem_object_get_pages() which doesn't even consider the mm.pages AFAIK.

The bogus migration is happening as part of the __i915_gem_object_get_pages() (2 __underscores) call in i915_gem_dmabuf_attach (see last patch). That code is attempting to migrate the BO to SMEM and then pin it there using the obvious calls to do so. However, in the pin_pages call, it gets implicitly migrated back to LMEM thanks to i915_ttm_get_pages(). Why is _get_pages() migrating things at all?

Not sure yet, but __two_underscores() checks if i915_gem_object_has_pages() before actually calling into i915_ttm_get_pages(), so the mm.pages would have to be NULL here for some reason, so best guess is something to do with move_notify().

--Jason

...

...

...

In case you can't tell, I really have no clue what I'm doing here. I'm really stumbling around in the dark finding things that make my bug go away. I'm happy for the feedback.

--Jason

...

...

Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: Matthew Auld matthew.auld@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com

---

drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index d30f274c329c7..5985e994d56cf 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -150,8 +150,7 @@ i915_ttm_placement_from_obj(const struct drm_i915_gem_object *obj, unsigned int i;

    placement->num_placement = 1;

•   i915_ttm_place_from_region(num_allowed ? obj->mm.placements[0] :
  

•                              obj->mm.region, requested, flags);
  

•   i915_ttm_place_from_region(obj->mm.region, requested, flags);
  
    /* Cache this on object? */
    placement->num_busy_placement = num_allowed;
  

-- 2.31.1