[Intel-gfx] [PATCH 0/7] drm/i915: Migrate memory to SMEM when imported cross-device

[Intel-gfx] [PATCH 0/7] drm/i915: Migrate memory to SMEM when imported cross-device

[Jason Ekstrand]

This patch series fixes an issue with discrete graphics on Intel where we
allowed dma-buf import while leaving the object in local memory.  This
breaks down pretty badly if the import happened on a different physical
device.

Jason Ekstrand (5):
  drm/i915/gem: Check object_can_migrate from object_migrate
  drm/i915/gem: Refactor placement setup for i915_gem_object_create*
  drm/i915/gem: Unify user object creation
  drm/i915/gem/ttm: Place new BOs in the requested region
  drm/i915/gem/ttm: Respect the objection region in placement_from_obj

Thomas Hellström (2):
  drm/i915/gem: Correct the locking and pin pattern for dma-buf (v6)
  drm/i915/gem: Migrate to system at dma-buf attach time (v6)

 drivers/gpu/drm/i915/gem/i915_gem_create.c    | 159 ++++++++-------
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  64 ++++--
 drivers/gpu/drm/i915/gem/i915_gem_object.c    |  13 +-
 drivers/gpu/drm/i915/gem/i915_gem_object.h    |   4 +
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c       |  11 +-
 .../drm/i915/gem/selftests/i915_gem_dmabuf.c  | 184 +++++++++++++++++-
 .../drm/i915/gem/selftests/i915_gem_migrate.c |  15 --
 7 files changed, 324 insertions(+), 126 deletions(-)

-- 
2.31.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/7] drm/i915/gem: Check object_can_migrate from object_migrate

[Jason Ekstrand]

We don't roll them together entirely because there are still a couple
cases where we want a separate can_migrate check.  For instance, the
display code checks that you can migrate a buffer to LMEM before it
accepts it in fb_create.  The dma-buf import code also uses it to do an
early check and return a different error code if someone tries to attach
a LMEM-only dma-buf to another driver.

However, no one actually wants to call object_migrate when can_migrate
has failed.  The stated intention is for self-tests but none of those
actually take advantage of this unsafe migration.

Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
Cc: Daniel Vetter <daniel@ffwll.ch>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_object.c        | 13 ++-----------
 .../gpu/drm/i915/gem/selftests/i915_gem_migrate.c | 15 ---------------
 2 files changed, 2 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 9da7b288b7ede..f2244ae09a613 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -584,12 +584,6 @@ bool i915_gem_object_can_migrate(struct drm_i915_gem_object *obj,
  * completed yet, and to accomplish that, i915_gem_object_wait_migration()
  * must be called.
  *
- * This function is a bit more permissive than i915_gem_object_can_migrate()
- * to allow for migrating objects where the caller knows exactly what is
- * happening. For example within selftests. More specifically this
- * function allows migrating I915_BO_ALLOC_USER objects to regions
- * that are not in the list of allowable regions.
- *
  * Note: the @ww parameter is not used yet, but included to make sure
  * callers put some effort into obtaining a valid ww ctx if one is
  * available.
@@ -616,11 +610,8 @@ int i915_gem_object_migrate(struct drm_i915_gem_object *obj,
 	if (obj->mm.region == mr)
 		return 0;
 
-	if (!i915_gem_object_evictable(obj))
-		return -EBUSY;
-
-	if (!obj->ops->migrate)
-		return -EOPNOTSUPP;
+	if (!i915_gem_object_can_migrate(obj, id))
+		return -EINVAL;
 
 	return obj->ops->migrate(obj, mr);
 }
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c
index 0b7144d2991ca..28a700f08b49a 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c
@@ -61,11 +61,6 @@ static int igt_create_migrate(struct intel_gt *gt, enum intel_region_id src,
 		if (err)
 			continue;
 
-		if (!i915_gem_object_can_migrate(obj, dst)) {
-			err = -EINVAL;
-			continue;
-		}
-
 		err = i915_gem_object_migrate(obj, &ww, dst);
 		if (err)
 			continue;
@@ -114,11 +109,6 @@ static int lmem_pages_migrate_one(struct i915_gem_ww_ctx *ww,
 		return err;
 
 	if (i915_gem_object_is_lmem(obj)) {
-		if (!i915_gem_object_can_migrate(obj, INTEL_REGION_SMEM)) {
-			pr_err("object can't migrate to smem.\n");
-			return -EINVAL;
-		}
-
 		err = i915_gem_object_migrate(obj, ww, INTEL_REGION_SMEM);
 		if (err) {
 			pr_err("Object failed migration to smem\n");
@@ -137,11 +127,6 @@ static int lmem_pages_migrate_one(struct i915_gem_ww_ctx *ww,
 		}
 
 	} else {
-		if (!i915_gem_object_can_migrate(obj, INTEL_REGION_LMEM)) {
-			pr_err("object can't migrate to lmem.\n");
-			return -EINVAL;
-		}
-
 		err = i915_gem_object_migrate(obj, ww, INTEL_REGION_LMEM);
 		if (err) {
 			pr_err("Object failed migration to lmem\n");
-- 
2.31.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/7] drm/i915/gem: Refactor placement setup for i915_gem_object_create*

[Jason Ekstrand]

Since we don't allow changing the set of regions after creation, we can
make ext_set_placements() build up the region set directly in the
create_ext and assign it to the object later.  This is similar to what
we did for contexts with the proto-context only simpler because there's
no funny object shuffling.  This will be used in the next patch to allow
us to de-duplicate a bunch of code.  Also, since we know the maximum
number of regions up-front, we can use a fixed-size temporary array for
the regions.  This simplifies memory management a bit for this new
delayed approach.

Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
---
 drivers/gpu/drm/i915/gem/i915_gem_create.c | 75 +++++++++++++---------
 1 file changed, 45 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c
index 51f92e4b1a69d..391c8c4a12172 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
@@ -27,10 +27,13 @@ static u32 object_max_page_size(struct drm_i915_gem_object *obj)
 	return max_page_size;
 }
 
-static void object_set_placements(struct drm_i915_gem_object *obj,
-				  struct intel_memory_region **placements,
-				  unsigned int n_placements)
+static int object_set_placements(struct drm_i915_gem_object *obj,
+				 struct intel_memory_region **placements,
+				 unsigned int n_placements)
 {
+	struct intel_memory_region **arr;
+	unsigned int i;
+
 	GEM_BUG_ON(!n_placements);
 
 	/*
@@ -44,9 +47,20 @@ static void object_set_placements(struct drm_i915_gem_object *obj,
 		obj->mm.placements = &i915->mm.regions[mr->id];
 		obj->mm.n_placements = 1;
 	} else {
-		obj->mm.placements = placements;
+		arr = kmalloc_array(n_placements,
+				    sizeof(struct intel_memory_region *),
+				    GFP_KERNEL);
+		if (!arr)
+			return -ENOMEM;
+
+		for (i = 0; i < n_placements; i++)
+			arr[i] = placements[i];
+
+		obj->mm.placements = arr;
 		obj->mm.n_placements = n_placements;
 	}
+
+	return 0;
 }
 
 static int i915_gem_publish(struct drm_i915_gem_object *obj,
@@ -148,7 +162,9 @@ i915_gem_dumb_create(struct drm_file *file,
 		return -ENOMEM;
 
 	mr = intel_memory_region_by_type(to_i915(dev), mem_type);
-	object_set_placements(obj, &mr, 1);
+	ret = object_set_placements(obj, &mr, 1);
+	if (ret)
+		goto object_free;
 
 	ret = i915_gem_setup(obj, args->size);
 	if (ret)
@@ -184,7 +200,9 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
 		return -ENOMEM;
 
 	mr = intel_memory_region_by_type(i915, INTEL_MEMORY_SYSTEM);
-	object_set_placements(obj, &mr, 1);
+	ret = object_set_placements(obj, &mr, 1);
+	if (ret)
+		goto object_free;
 
 	ret = i915_gem_setup(obj, args->size);
 	if (ret)
@@ -197,9 +215,12 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
 	return ret;
 }
 
+#define MAX_N_PLACEMENTS = (INTEL_REGION_UNKNOWN + 1)
+
 struct create_ext {
 	struct drm_i915_private *i915;
-	struct drm_i915_gem_object *vanilla_object;
+	struct intel_memory_region *placements[INTEL_REGION_UNKNOWN];
+	unsigned int n_placements;
 };
 
 static void repr_placements(char *buf, size_t size,
@@ -230,8 +251,7 @@ static int set_placements(struct drm_i915_gem_create_ext_memory_regions *args,
 	struct drm_i915_private *i915 = ext_data->i915;
 	struct drm_i915_gem_memory_class_instance __user *uregions =
 		u64_to_user_ptr(args->regions);
-	struct drm_i915_gem_object *obj = ext_data->vanilla_object;
-	struct intel_memory_region **placements;
+	struct intel_memory_region *placements[INTEL_REGION_UNKNOWN];
 	u32 mask;
 	int i, ret = 0;
 
@@ -245,6 +265,8 @@ static int set_placements(struct drm_i915_gem_create_ext_memory_regions *args,
 		ret = -EINVAL;
 	}
 
+	BUILD_BUG_ON(ARRAY_SIZE(i915->mm.regions) != ARRAY_SIZE(placements));
+	BUILD_BUG_ON(ARRAY_SIZE(ext_data->placements) != ARRAY_SIZE(placements));
 	if (args->num_regions > ARRAY_SIZE(i915->mm.regions)) {
 		drm_dbg(&i915->drm, "num_regions is too large\n");
 		ret = -EINVAL;
@@ -253,12 +275,6 @@ static int set_placements(struct drm_i915_gem_create_ext_memory_regions *args,
 	if (ret)
 		return ret;
 
-	placements = kmalloc_array(args->num_regions,
-				   sizeof(struct intel_memory_region *),
-				   GFP_KERNEL);
-	if (!placements)
-		return -ENOMEM;
-
 	mask = 0;
 	for (i = 0; i < args->num_regions; i++) {
 		struct drm_i915_gem_memory_class_instance region;
@@ -293,14 +309,13 @@ static int set_placements(struct drm_i915_gem_create_ext_memory_regions *args,
 		++uregions;
 	}
 
-	if (obj->mm.placements) {
+	if (ext_data->n_placements) {
 		ret = -EINVAL;
 		goto out_dump;
 	}
 
-	object_set_placements(obj, placements, args->num_regions);
-	if (args->num_regions == 1)
-		kfree(placements);
+	for (i = 0; i < args->num_regions; i++)
+		ext_data->placements[i] = placements[i];
 
 	return 0;
 
@@ -308,11 +323,11 @@ static int set_placements(struct drm_i915_gem_create_ext_memory_regions *args,
 	if (1) {
 		char buf[256];
 
-		if (obj->mm.placements) {
+		if (ext_data->n_placements) {
 			repr_placements(buf,
 					sizeof(buf),
-					obj->mm.placements,
-					obj->mm.n_placements);
+					ext_data->placements,
+					ext_data->n_placements);
 			drm_dbg(&i915->drm,
 				"Placements were already set in previous EXT. Existing placements: %s\n",
 				buf);
@@ -358,7 +373,6 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
 	struct drm_i915_private *i915 = to_i915(dev);
 	struct drm_i915_gem_create_ext *args = data;
 	struct create_ext ext_data = { .i915 = i915 };
-	struct intel_memory_region **placements_ext;
 	struct drm_i915_gem_object *obj;
 	int ret;
 
@@ -371,21 +385,22 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
 	if (!obj)
 		return -ENOMEM;
 
-	ext_data.vanilla_object = obj;
 	ret = i915_user_extensions(u64_to_user_ptr(args->extensions),
 				   create_extensions,
 				   ARRAY_SIZE(create_extensions),
 				   &ext_data);
-	placements_ext = obj->mm.placements;
 	if (ret)
 		goto object_free;
 
-	if (!placements_ext) {
-		struct intel_memory_region *mr =
+	if (!ext_data.n_placements) {
+		ext_data.placements[0] =
 			intel_memory_region_by_type(i915, INTEL_MEMORY_SYSTEM);
-
-		object_set_placements(obj, &mr, 1);
+		ext_data.n_placements = 1;
 	}
+	ret = object_set_placements(obj, ext_data.placements,
+				    ext_data.n_placements);
+	if (ret)
+		goto object_free;
 
 	ret = i915_gem_setup(obj, args->size);
 	if (ret)
@@ -395,7 +410,7 @@ i915_gem_create_ext_ioctl(struct drm_device *dev, void *data,
 
 object_free:
 	if (obj->mm.n_placements > 1)
-		kfree(placements_ext);
+		kfree(obj->mm.placements);
 	i915_gem_object_free(obj);
 	return ret;
 }
-- 
2.31.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 3/7] drm/i915/gem: Unify user object creation

[Jason Ekstrand]

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;
 
 	/*
 	 * 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;
 }
-- 
2.31.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 4/7] drm/i915/gem/ttm: Place new BOs in the requested region

[Jason Ekstrand]

__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().

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj

[Jason Ekstrand]

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().

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 6/7] drm/i915/gem: Correct the locking and pin pattern for dma-buf (v6)

[Jason Ekstrand]

From: Thomas Hellström <thomas.hellstrom@linux.intel.com>

If our exported dma-bufs are imported by another instance of our driver,
that instance will typically have the imported dma-bufs locked during
dma_buf_map_attachment(). But the exporter also locks the same reservation
object in the map_dma_buf() callback, which leads to recursive locking.

So taking the lock inside _pin_pages_unlocked() is incorrect.

Additionally, the current pinning code path is contrary to the defined
way that pinning should occur.

Remove the explicit pin/unpin from the map/umap functions and move them
to the attach/detach allowing correct locking to occur, and to match
the static dma-buf drm_prime pattern.

Add a live selftest to exercise both dynamic and non-dynamic
exports.

v2:
- Extend the selftest with a fake dynamic importer.
- Provide real pin and unpin callbacks to not abuse the interface.
v3: (ruhl)
- Remove the dynamic export support and move the pinning into the
  attach/detach path.
v4: (ruhl)
- Put pages does not need to assert on the dma-resv
v5: (jason)
- Lock around dma_buf_unmap_attachment() when emulating a dynamic
  importer in the subtests.
- Use pin_pages_unlocked
v6: (jason)
- Use dma_buf_attach instead of dma_buf_attach_dynamic in the selftests

Reported-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
---
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    |  43 ++++++--
 .../drm/i915/gem/selftests/i915_gem_dmabuf.c  | 103 +++++++++++++++++-
 2 files changed, 132 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
index 616c3a2f1baf0..9a655f69a0671 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
@@ -12,6 +12,8 @@
 #include "i915_gem_object.h"
 #include "i915_scatterlist.h"
 
+I915_SELFTEST_DECLARE(static bool force_different_devices;)
+
 static struct drm_i915_gem_object *dma_buf_to_obj(struct dma_buf *buf)
 {
 	return to_intel_bo(buf->priv);
@@ -25,15 +27,11 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme
 	struct scatterlist *src, *dst;
 	int ret, i;
 
-	ret = i915_gem_object_pin_pages_unlocked(obj);
-	if (ret)
-		goto err;
-
 	/* Copy sg so that we make an independent mapping */
 	st = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
 	if (st == NULL) {
 		ret = -ENOMEM;
-		goto err_unpin_pages;
+		goto err;
 	}
 
 	ret = sg_alloc_table(st, obj->mm.pages->nents, GFP_KERNEL);
@@ -58,8 +56,6 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme
 	sg_free_table(st);
 err_free:
 	kfree(st);
-err_unpin_pages:
-	i915_gem_object_unpin_pages(obj);
 err:
 	return ERR_PTR(ret);
 }
@@ -68,13 +64,9 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
 				   struct sg_table *sg,
 				   enum dma_data_direction dir)
 {
-	struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf);
-
 	dma_unmap_sgtable(attachment->dev, sg, dir, DMA_ATTR_SKIP_CPU_SYNC);
 	sg_free_table(sg);
 	kfree(sg);
-
-	i915_gem_object_unpin_pages(obj);
 }
 
 static int i915_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct dma_buf_map *map)
@@ -168,7 +160,31 @@ static int i915_gem_end_cpu_access(struct dma_buf *dma_buf, enum dma_data_direct
 	return err;
 }
 
+/**
+ * i915_gem_dmabuf_attach - Do any extra attach work necessary
+ * @dmabuf: imported dma-buf
+ * @attach: new attach to do work on
+ *
+ */
+static int i915_gem_dmabuf_attach(struct dma_buf *dmabuf,
+				  struct dma_buf_attachment *attach)
+{
+	struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf);
+
+	return i915_gem_object_pin_pages_unlocked(obj);
+}
+
+static void i915_gem_dmabuf_detach(struct dma_buf *dmabuf,
+				   struct dma_buf_attachment *attach)
+{
+	struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf);
+
+	i915_gem_object_unpin_pages(obj);
+}
+
 static const struct dma_buf_ops i915_dmabuf_ops =  {
+	.attach = i915_gem_dmabuf_attach,
+	.detach = i915_gem_dmabuf_detach,
 	.map_dma_buf = i915_gem_map_dma_buf,
 	.unmap_dma_buf = i915_gem_unmap_dma_buf,
 	.release = drm_gem_dmabuf_release,
@@ -204,6 +220,8 @@ static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
 	struct sg_table *pages;
 	unsigned int sg_page_sizes;
 
+	assert_object_held(obj);
+
 	pages = dma_buf_map_attachment(obj->base.import_attach,
 				       DMA_BIDIRECTIONAL);
 	if (IS_ERR(pages))
@@ -241,7 +259,8 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
 	if (dma_buf->ops == &i915_dmabuf_ops) {
 		obj = dma_buf_to_obj(dma_buf);
 		/* is it from our device? */
-		if (obj->base.dev == dev) {
+		if (obj->base.dev == dev &&
+		    !I915_SELFTEST_ONLY(force_different_devices)) {
 			/*
 			 * Importing dmabuf exported from out own gem increases
 			 * refcount on gem itself instead of f_count of dmabuf.
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
index dd74bc09ec88d..4451bbb4917e4 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
@@ -35,7 +35,7 @@ static int igt_dmabuf_export(void *arg)
 static int igt_dmabuf_import_self(void *arg)
 {
 	struct drm_i915_private *i915 = arg;
-	struct drm_i915_gem_object *obj;
+	struct drm_i915_gem_object *obj, *import_obj;
 	struct drm_gem_object *import;
 	struct dma_buf *dmabuf;
 	int err;
@@ -65,14 +65,112 @@ static int igt_dmabuf_import_self(void *arg)
 		err = -EINVAL;
 		goto out_import;
 	}
+	import_obj = to_intel_bo(import);
+
+	i915_gem_object_lock(import_obj, NULL);
+	err = ____i915_gem_object_get_pages(import_obj);
+	i915_gem_object_unlock(import_obj);
+	if (err) {
+		pr_err("Same object dma-buf get_pages failed!\n");
+		goto out_import;
+	}
 
 	err = 0;
 out_import:
-	i915_gem_object_put(to_intel_bo(import));
+	i915_gem_object_put(import_obj);
+out_dmabuf:
+	dma_buf_put(dmabuf);
+out:
+	i915_gem_object_put(obj);
+	return err;
+}
+
+static int igt_dmabuf_import_same_driver(void *arg)
+{
+	struct drm_i915_private *i915 = arg;
+	struct drm_i915_gem_object *obj, *import_obj;
+	struct drm_gem_object *import;
+	struct dma_buf *dmabuf;
+	struct dma_buf_attachment *import_attach;
+	struct sg_table *st;
+	long timeout;
+	int err;
+
+	force_different_devices = true;
+	obj = i915_gem_object_create_shmem(i915, PAGE_SIZE);
+	if (IS_ERR(obj))
+		goto out_ret;
+
+	dmabuf = i915_gem_prime_export(&obj->base, 0);
+	if (IS_ERR(dmabuf)) {
+		pr_err("i915_gem_prime_export failed with err=%d\n",
+		       (int)PTR_ERR(dmabuf));
+		err = PTR_ERR(dmabuf);
+		goto out;
+	}
+
+	import = i915_gem_prime_import(&i915->drm, dmabuf);
+	if (IS_ERR(import)) {
+		pr_err("i915_gem_prime_import failed with err=%d\n",
+		       (int)PTR_ERR(import));
+		err = PTR_ERR(import);
+		goto out_dmabuf;
+	}
+
+	if (import == &obj->base) {
+		pr_err("i915_gem_prime_import reused gem object!\n");
+		err = -EINVAL;
+		goto out_import;
+	}
+
+	import_obj = to_intel_bo(import);
+
+	i915_gem_object_lock(import_obj, NULL);
+	err = ____i915_gem_object_get_pages(import_obj);
+	if (err) {
+		pr_err("Different objects dma-buf get_pages failed!\n");
+		i915_gem_object_unlock(import_obj);
+		goto out_import;
+	}
+
+	/*
+	 * If the exported object is not in system memory, something
+	 * weird is going on. TODO: When p2p is supported, this is no
+	 * longer considered weird.
+	 */
+	if (obj->mm.region != i915->mm.regions[INTEL_REGION_SMEM]) {
+		pr_err("Exported dma-buf is not in system memory\n");
+		err = -EINVAL;
+	}
+
+	i915_gem_object_unlock(import_obj);
+
+	/* Now try a fake an importer */
+	import_attach = dma_buf_attach(dmabuf, obj->base.dev->dev);
+	if (IS_ERR(import_attach))
+		goto out_import;
+
+	st = dma_buf_map_attachment(import_attach, DMA_BIDIRECTIONAL);
+	if (IS_ERR(st))
+		goto out_detach;
+
+	timeout = dma_resv_wait_timeout(dmabuf->resv, false, true, 5 * HZ);
+	if (!timeout) {
+		pr_err("dmabuf wait for exclusive fence timed out.\n");
+		timeout = -ETIME;
+	}
+	err = timeout > 0 ? 0 : timeout;
+	dma_buf_unmap_attachment(import_attach, st, DMA_BIDIRECTIONAL);
+out_detach:
+	dma_buf_detach(dmabuf, import_attach);
+out_import:
+	i915_gem_object_put(import_obj);
 out_dmabuf:
 	dma_buf_put(dmabuf);
 out:
 	i915_gem_object_put(obj);
+out_ret:
+	force_different_devices = false;
 	return err;
 }
 
@@ -286,6 +384,7 @@ int i915_gem_dmabuf_live_selftests(struct drm_i915_private *i915)
 {
 	static const struct i915_subtest tests[] = {
 		SUBTEST(igt_dmabuf_export),
+		SUBTEST(igt_dmabuf_import_same_driver),
 	};
 
 	return i915_subtests(tests, i915);
-- 
2.31.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 7/7] drm/i915/gem: Migrate to system at dma-buf attach time (v6)

[Jason Ekstrand]

From: Thomas Hellström <thomas.hellstrom@linux.intel.com>

Until we support p2p dma or as a complement to that, migrate data
to system memory at dma-buf attach time if possible.

v2:
- Rebase on dynamic exporter. Update the igt_dmabuf_import_same_driver
  selftest to migrate if we are LMEM capable.
v3:
- Migrate also in the pin() callback.
v4:
- Migrate in attach
v5: (jason)
- Lock around the migration
v6: (jason)
- Move the can_migrate check outside the lock
- Rework the selftests to test more migration conditions.  In
  particular, SMEM, LMEM, and LMEM+SMEM are all checked.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
---
 drivers/gpu/drm/i915/gem/i915_gem_create.c    |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c    | 23 ++++-
 drivers/gpu/drm/i915/gem/i915_gem_object.h    |  4 +
 .../drm/i915/gem/selftests/i915_gem_dmabuf.c  | 89 ++++++++++++++++++-
 4 files changed, 112 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c
index 69bf9ec777642..ed6b0d75ff801 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
@@ -82,7 +82,7 @@ static int i915_gem_publish(struct drm_i915_gem_object *obj,
 	return 0;
 }
 
-static struct drm_i915_gem_object *
+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)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
index 9a655f69a0671..5d438b95826b9 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
@@ -170,8 +170,29 @@ static int i915_gem_dmabuf_attach(struct dma_buf *dmabuf,
 				  struct dma_buf_attachment *attach)
 {
 	struct drm_i915_gem_object *obj = dma_buf_to_obj(dmabuf);
+	struct i915_gem_ww_ctx ww;
+	int err;
+
+	if (!i915_gem_object_can_migrate(obj, INTEL_REGION_SMEM))
+		return -EOPNOTSUPP;
+
+	for_i915_gem_ww(&ww, err, true) {
+		err = i915_gem_object_lock(obj, &ww);
+		if (err)
+			continue;
+
+		err = i915_gem_object_migrate(obj, &ww, INTEL_REGION_SMEM);
+		if (err)
+			continue;
 
-	return i915_gem_object_pin_pages_unlocked(obj);
+		err = i915_gem_object_wait_migration(obj, 0);
+		if (err)
+			continue;
+
+		err = i915_gem_object_pin_pages(obj);
+	}
+
+	return err;
 }
 
 static void i915_gem_dmabuf_detach(struct dma_buf *dmabuf,
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 8be4fadeee487..fbae53bd46384 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -61,6 +61,10 @@ i915_gem_object_create_shmem(struct drm_i915_private *i915,
 struct drm_i915_gem_object *
 i915_gem_object_create_shmem_from_data(struct drm_i915_private *i915,
 				       const void *data, resource_size_t size);
+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);
 
 extern const struct drm_i915_gem_object_ops i915_gem_shmem_ops;
 
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
index 4451bbb4917e4..7b7647e7e220a 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c
@@ -85,9 +85,62 @@ static int igt_dmabuf_import_self(void *arg)
 	return err;
 }
 
-static int igt_dmabuf_import_same_driver(void *arg)
+static int igt_dmabuf_import_same_driver_lmem(void *arg)
 {
 	struct drm_i915_private *i915 = arg;
+	struct intel_memory_region *lmem = i915->mm.regions[INTEL_REGION_LMEM];
+	struct drm_i915_gem_object *obj;
+	struct drm_gem_object *import;
+	struct dma_buf *dmabuf;
+	int err;
+
+	if (!i915->mm.regions[INTEL_REGION_LMEM])
+		return 0;
+
+	force_different_devices = true;
+
+	obj = i915_gem_object_create_user(i915, PAGE_SIZE, &lmem, 1);
+	if (IS_ERR(obj)) {
+		pr_err("i915_gem_object_create_user failed with err=%d\n",
+		       (int)PTR_ERR(dmabuf));
+		err = PTR_ERR(obj);
+		goto out_ret;
+	}
+
+	dmabuf = i915_gem_prime_export(&obj->base, 0);
+	if (IS_ERR(dmabuf)) {
+		pr_err("i915_gem_prime_export failed with err=%d\n",
+		       (int)PTR_ERR(dmabuf));
+		err = PTR_ERR(dmabuf);
+		goto out;
+	}
+
+	/* We expect an import of an LMEM-only object to fail with
+	 * -EOPNOTSUPP because it can't be migrated to SMEM.
+	 */
+	import = i915_gem_prime_import(&i915->drm, dmabuf);
+	if (!IS_ERR(import)) {
+		drm_gem_object_put(import);
+		pr_err("i915_gem_prime_import succeeded when it shouldn't have\n");
+		err = -EINVAL;
+	} else if (PTR_ERR(import) != -EOPNOTSUPP) {
+		pr_err("i915_gem_prime_import failed with the wrong err=%d\n",
+		       (int)PTR_ERR(import));
+		err = PTR_ERR(import);
+	}
+
+	dma_buf_put(dmabuf);
+out:
+	i915_gem_object_put(obj);
+out_ret:
+	force_different_devices = false;
+	return err;
+}
+
+static int igt_dmabuf_import_same_driver(struct drm_i915_private *i915,
+					 struct intel_memory_region **regions,
+					 unsigned int num_regions)
+{
 	struct drm_i915_gem_object *obj, *import_obj;
 	struct drm_gem_object *import;
 	struct dma_buf *dmabuf;
@@ -97,9 +150,15 @@ static int igt_dmabuf_import_same_driver(void *arg)
 	int err;
 
 	force_different_devices = true;
-	obj = i915_gem_object_create_shmem(i915, PAGE_SIZE);
-	if (IS_ERR(obj))
+
+	obj = i915_gem_object_create_user(i915, PAGE_SIZE,
+					  regions, num_regions);
+	if (IS_ERR(obj)) {
+		pr_err("i915_gem_object_create_user failed with err=%d\n",
+		       (int)PTR_ERR(dmabuf));
+		err = PTR_ERR(obj);
 		goto out_ret;
+	}
 
 	dmabuf = i915_gem_prime_export(&obj->base, 0);
 	if (IS_ERR(dmabuf)) {
@@ -174,6 +233,26 @@ static int igt_dmabuf_import_same_driver(void *arg)
 	return err;
 }
 
+static int igt_dmabuf_import_same_driver_smem(void *arg)
+{
+	struct drm_i915_private *i915 = arg;
+	struct intel_memory_region *smem = i915->mm.regions[INTEL_REGION_SMEM];
+	return igt_dmabuf_import_same_driver(i915, &smem, 1);
+}
+
+static int igt_dmabuf_import_same_driver_lmem_smem(void *arg)
+{
+	struct drm_i915_private *i915 = arg;
+	struct intel_memory_region *regions[2];
+
+	if (!i915->mm.regions[INTEL_REGION_LMEM])
+		return 0;
+
+	regions[0] = i915->mm.regions[INTEL_REGION_LMEM];
+	regions[1] = i915->mm.regions[INTEL_REGION_SMEM];
+	return igt_dmabuf_import_same_driver(i915, regions, 2);
+}
+
 static int igt_dmabuf_import(void *arg)
 {
 	struct drm_i915_private *i915 = arg;
@@ -384,7 +463,9 @@ int i915_gem_dmabuf_live_selftests(struct drm_i915_private *i915)
 {
 	static const struct i915_subtest tests[] = {
 		SUBTEST(igt_dmabuf_export),
-		SUBTEST(igt_dmabuf_import_same_driver),
+		SUBTEST(igt_dmabuf_import_same_driver_lmem),
+		SUBTEST(igt_dmabuf_import_same_driver_smem),
+		SUBTEST(igt_dmabuf_import_same_driver_lmem_smem),
 	};
 
 	return i915_subtests(tests, i915);
-- 
2.31.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/7] drm/i915/gem: Refactor placement setup for i915_gem_object_create*

[Matthew Auld]

On Thu, 15 Jul 2021 at 23:39, Jason Ekstrand <jason@jlekstrand.net> wrote:
>
> Since we don't allow changing the set of regions after creation, we can
> make ext_set_placements() build up the region set directly in the
> create_ext and assign it to the object later.  This is similar to what
> we did for contexts with the proto-context only simpler because there's
> no funny object shuffling.  This will be used in the next patch to allow
> us to de-duplicate a bunch of code.  Also, since we know the maximum
> number of regions up-front, we can use a fixed-size temporary array for
> the regions.  This simplifies memory management a bit for this new
> delayed approach.
>
> Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>

I think the plan was to have each extension directly hang/apply their
state on the vanilla/proto object, or at least that was the idea
behind: 357814f878f9 ("drm/i915: rework gem_create flow for upcoming
extensions"). A previous version looked similar to this IIRC.

> ---
>  drivers/gpu/drm/i915/gem/i915_gem_create.c | 75 +++++++++++++---------
>  1 file changed, 45 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> index 51f92e4b1a69d..391c8c4a12172 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> @@ -27,10 +27,13 @@ static u32 object_max_page_size(struct drm_i915_gem_object *obj)
>         return max_page_size;
>  }
>
> -static void object_set_placements(struct drm_i915_gem_object *obj,
> -                                 struct intel_memory_region **placements,
> -                                 unsigned int n_placements)
> +static int object_set_placements(struct drm_i915_gem_object *obj,
> +                                struct intel_memory_region **placements,
> +                                unsigned int n_placements)
>  {
> +       struct intel_memory_region **arr;
> +       unsigned int i;
> +
>         GEM_BUG_ON(!n_placements);
>
>         /*
> @@ -44,9 +47,20 @@ static void object_set_placements(struct drm_i915_gem_object *obj,
>                 obj->mm.placements = &i915->mm.regions[mr->id];
>                 obj->mm.n_placements = 1;
>         } else {
> -               obj->mm.placements = placements;
> +               arr = kmalloc_array(n_placements,
> +                                   sizeof(struct intel_memory_region *),
> +                                   GFP_KERNEL);
> +               if (!arr)
> +                       return -ENOMEM;
> +
> +               for (i = 0; i < n_placements; i++)
> +                       arr[i] = placements[i];
> +
> +               obj->mm.placements = arr;
>                 obj->mm.n_placements = n_placements;
>         }
> +
> +       return 0;
>  }
>
>  static int i915_gem_publish(struct drm_i915_gem_object *obj,
> @@ -148,7 +162,9 @@ i915_gem_dumb_create(struct drm_file *file,
>                 return -ENOMEM;
>
>         mr = intel_memory_region_by_type(to_i915(dev), mem_type);
> -       object_set_placements(obj, &mr, 1);
> +       ret = object_set_placements(obj, &mr, 1);
> +       if (ret)
> +               goto object_free;
>
>         ret = i915_gem_setup(obj, args->size);
>         if (ret)
> @@ -184,7 +200,9 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
>                 return -ENOMEM;
>
>         mr = intel_memory_region_by_type(i915, INTEL_MEMORY_SYSTEM);
> -       object_set_placements(obj, &mr, 1);
> +       ret = object_set_placements(obj, &mr, 1);
> +       if (ret)
> +               goto object_free;
>
>         ret = i915_gem_setup(obj, args->size);
>         if (ret)
> @@ -197,9 +215,12 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
>         return ret;
>  }
>
> +#define MAX_N_PLACEMENTS = (INTEL_REGION_UNKNOWN + 1)

Unused?

> +
>  struct create_ext {
>         struct drm_i915_private *i915;
> -       struct drm_i915_gem_object *vanilla_object;
> +       struct intel_memory_region *placements[INTEL_REGION_UNKNOWN];
> +       unsigned int n_placements;
>  };
>
>  static void repr_placements(char *buf, size_t size,
> @@ -230,8 +251,7 @@ static int set_placements(struct drm_i915_gem_create_ext_memory_regions *args,
>         struct drm_i915_private *i915 = ext_data->i915;
>         struct drm_i915_gem_memory_class_instance __user *uregions =
>                 u64_to_user_ptr(args->regions);
> -       struct drm_i915_gem_object *obj = ext_data->vanilla_object;
> -       struct intel_memory_region **placements;
> +       struct intel_memory_region *placements[INTEL_REGION_UNKNOWN];
>         u32 mask;
>         int i, ret = 0;
>
> @@ -245,6 +265,8 @@ static int set_placements(struct drm_i915_gem_create_ext_memory_regions *args,
>                 ret = -EINVAL;
>         }
>
> +       BUILD_BUG_ON(ARRAY_SIZE(i915->mm.regions) != ARRAY_SIZE(placements));
> +       BUILD_BUG_ON(ARRAY_SIZE(ext_data->placements) != ARRAY_SIZE(placements));
>         if (args->num_regions > ARRAY_SIZE(i915->mm.regions)) {
>                 drm_dbg(&i915->drm, "num_regions is too large\n");
>                 ret = -EINVAL;
> @@ -253,12 +275,6 @@ static int set_placements(struct drm_i915_gem_create_ext_memory_regions *args,
>         if (ret)
>                 return ret;
>
> -       placements = kmalloc_array(args->num_regions,
> -                                  sizeof(struct intel_memory_region *),
> -                                  GFP_KERNEL);
> -       if (!placements)
> -               return -ENOMEM;
> -
>         mask = 0;
>         for (i = 0; i < args->num_regions; i++) {
>                 struct drm_i915_gem_memory_class_instance region;
> @@ -293,14 +309,13 @@ static int set_placements(struct drm_i915_gem_create_ext_memory_regions *args,
>                 ++uregions;
>         }
>
> -       if (obj->mm.placements) {
> +       if (ext_data->n_placements) {
>                 ret = -EINVAL;
>                 goto out_dump;
>         }
>
> -       object_set_placements(obj, placements, args->num_regions);
> -       if (args->num_regions == 1)
> -               kfree(placements);
> +       for (i = 0; i < args->num_regions; i++)
> +               ext_data->placements[i] = placements[i];

kfree(placements) is still in the error unwind?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/7] drm/i915/gem: Unify user object creation

[Matthew Auld]

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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/7] drm/i915/gem/ttm: Place new BOs in the requested region

[Matthew Auld]

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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/7] drm/i915/gem/ttm: Place new BOs in the requested region

[Jason Ekstrand]

On Fri, Jul 16, 2021 at 8:18 AM Matthew Auld
<matthew.william.auld@gmail.com> wrote:
>
> 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.

I'm happy to drop this patch.  It doesn't actually fix anything AFAIK.
It makes the behavior more what I expected but my expectations are not
to be trusted here.

The other TTM patch does fix a real bug AFAIK.

--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 | 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
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/7] drm/i915/gem: Refactor placement setup for i915_gem_object_create*

[Jason Ekstrand]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj

[Matthew Auld]

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?

>
> 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/7] drm/i915/gem: Unify user object creation

[Jason Ekstrand]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj

[Jason Ekstrand]

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
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj

[Matthew Auld]

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?

>
> 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
> > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj

[Matthew Auld]

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
> > > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj

[Jason Ekstrand]

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.

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

--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
> > > > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj

[Matthew Auld]

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
> > > > > >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj

[Jason Ekstrand]

On Fri, Jul 16, 2021 at 1:45 PM Matthew Auld
<matthew.william.auld@gmail.com> wrote:
>
> 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".

I've switched the ____four_underscores call to a __two_underscores in
the selftests and it had no effect, good or bad.  But, still, probably
better to call that one.

> >
> > > > >
> > > > > 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().

Did a bit of experimenting along those lines and added the following
to the self-test BEFORE the export/import:

    i915_gem_object_lock(obj, NULL);
    err = __i915_gem_object_get_pages(obj);
    __i915_gem_object_unpin_pages(obj);
    i915_gem_object_unlock(obj);
    if (err) {
        pr_err("__i915_gem_object_get_pages failed with err=%d\n", err);
        goto out_ret;
    }

This seems to make the migration happen as expected without this
patch.  So it seems the problem only exists on buffers that haven't
gotten any backing storage yet (if I'm understanding get_pages
correctly).

One potential work-around (not sure if this is a good idea or not!)
would be to do this inside dmabuf_attach().  Is this reliable?  Once
it has pages will it always have pages?  Or are there crazy races I
need to be worried about here?

--Jason
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj

[Matthew Auld]

On Fri, 16 Jul 2021 at 20:49, Jason Ekstrand <jason@jlekstrand.net> wrote:
>
> On Fri, Jul 16, 2021 at 1:45 PM Matthew Auld
> <matthew.william.auld@gmail.com> wrote:
> >
> > 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".
>
> I've switched the ____four_underscores call to a __two_underscores in
> the selftests and it had no effect, good or bad.  But, still, probably
> better to call that one.
>
> > >
> > > > > >
> > > > > > 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().
>
> Did a bit of experimenting along those lines and added the following
> to the self-test BEFORE the export/import:
>
>     i915_gem_object_lock(obj, NULL);
>     err = __i915_gem_object_get_pages(obj);
>     __i915_gem_object_unpin_pages(obj);
>     i915_gem_object_unlock(obj);
>     if (err) {
>         pr_err("__i915_gem_object_get_pages failed with err=%d\n", err);
>         goto out_ret;
>     }
>
> This seems to make the migration happen as expected without this
> patch.  So it seems the problem only exists on buffers that haven't
> gotten any backing storage yet (if I'm understanding get_pages
> correctly).
>
> One potential work-around (not sure if this is a good idea or not!)
> would be to do this inside dmabuf_attach().  Is this reliable?  Once
> it has pages will it always have pages?  Or are there crazy races I
> need to be worried about here?

It turns out that the i915_ttm_adjust_gem_after_move() call in
ttm_object_init will always update the mm.region to system memory(so
that it matches the ttm resource), which seems reasonable given the
default system placeholder thing, but does seem slightly iffy since we
haven't actually moved/allocated anything.

So effectively i915_ttm_migrate(SYSTEM) becomes a noop here since
mm.region == mr. Which ofc means when we actually call get_pages() all
that happens is that we allocate the pages in system memory(or without
this patch placements[0]). Also with this patch lmem+smem, will always
be placed in smem first, regardless of the placements ordering.

For now we could maybe just split i915_ttm_adjust_gem_after_move() so
we skip the part which updates the mm.region here in the init portion,
since that should only happen when we try to place the object for
real?

>
> --Jason
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/7] drm/i915/gem: Unify user object creation

[Matthew Auld]

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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/7] drm/i915/gem: Unify user object creation

[Jason Ekstrand]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/7] drm/i915/gem: Unify user object creation

[Matthew Auld]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/7] drm/i915/gem: Unify user object creation

[Jason Ekstrand]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/7] drm/i915/gem: Unify user object creation

[Matthew Auld]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj

[Jason Ekstrand]

On Mon, Jul 19, 2021 at 8:35 AM Matthew Auld
<matthew.william.auld@gmail.com> wrote:
>
> On Fri, 16 Jul 2021 at 20:49, Jason Ekstrand <jason@jlekstrand.net> wrote:
> >
> > On Fri, Jul 16, 2021 at 1:45 PM Matthew Auld
> > <matthew.william.auld@gmail.com> wrote:
> > >
> > > 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".
> >
> > I've switched the ____four_underscores call to a __two_underscores in
> > the selftests and it had no effect, good or bad.  But, still, probably
> > better to call that one.
> >
> > > >
> > > > > > >
> > > > > > > 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().
> >
> > Did a bit of experimenting along those lines and added the following
> > to the self-test BEFORE the export/import:
> >
> >     i915_gem_object_lock(obj, NULL);
> >     err = __i915_gem_object_get_pages(obj);
> >     __i915_gem_object_unpin_pages(obj);
> >     i915_gem_object_unlock(obj);
> >     if (err) {
> >         pr_err("__i915_gem_object_get_pages failed with err=%d\n", err);
> >         goto out_ret;
> >     }
> >
> > This seems to make the migration happen as expected without this
> > patch.  So it seems the problem only exists on buffers that haven't
> > gotten any backing storage yet (if I'm understanding get_pages
> > correctly).
> >
> > One potential work-around (not sure if this is a good idea or not!)
> > would be to do this inside dmabuf_attach().  Is this reliable?  Once
> > it has pages will it always have pages?  Or are there crazy races I
> > need to be worried about here?
>
> It turns out that the i915_ttm_adjust_gem_after_move() call in
> ttm_object_init will always update the mm.region to system memory(so
> that it matches the ttm resource), which seems reasonable given the
> default system placeholder thing, but does seem slightly iffy since we
> haven't actually moved/allocated anything.
>
> So effectively i915_ttm_migrate(SYSTEM) becomes a noop here since
> mm.region == mr. Which ofc means when we actually call get_pages() all
> that happens is that we allocate the pages in system memory(or without
> this patch placements[0]). Also with this patch lmem+smem, will always
> be placed in smem first, regardless of the placements ordering.
>
> For now we could maybe just split i915_ttm_adjust_gem_after_move() so
> we skip the part which updates the mm.region here in the init portion,
> since that should only happen when we try to place the object for
> real?

Doesn't that mean we would end up with obj->mm.region and
obj->mm.res->mem_type are out-of-sync?  That seems bad.  I would think
we'd want the two in sync at all times.

It seems like the fundamental problem here is that, when it's created,
the object isn't really in any memory region at all.  While I don't
think obj->mm.region == NULL is allowed or a good idea, it does seem
closer to the ground truth.

Perhaps what we really want is for i915_gem_object_migrate to
get_pages before it does the migration to ensure that pages exist.
The only call to i915_gem_object_migrate in the code-base today is in
the display code and it's immediately followed by pin_pages().  For
that matter, maybe the call we actually want is
i915_object_migrate_and_pin that does the whole lot.

Thoughts?

--Jason

P.S.  I'm going to go ahead and send another version with your other
comments addressed.  We can keep this discussion going here for now.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj

[Daniel Vetter]

On Wed, Jul 21, 2021 at 10:11 PM Jason Ekstrand <jason@jlekstrand.net> wrote:
>
> On Mon, Jul 19, 2021 at 8:35 AM Matthew Auld
> <matthew.william.auld@gmail.com> wrote:
> >
> > On Fri, 16 Jul 2021 at 20:49, Jason Ekstrand <jason@jlekstrand.net> wrote:
> > >
> > > On Fri, Jul 16, 2021 at 1:45 PM Matthew Auld
> > > <matthew.william.auld@gmail.com> wrote:
> > > >
> > > > 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".
> > >
> > > I've switched the ____four_underscores call to a __two_underscores in
> > > the selftests and it had no effect, good or bad.  But, still, probably
> > > better to call that one.
> > >
> > > > >
> > > > > > > >
> > > > > > > > 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().
> > >
> > > Did a bit of experimenting along those lines and added the following
> > > to the self-test BEFORE the export/import:
> > >
> > >     i915_gem_object_lock(obj, NULL);
> > >     err = __i915_gem_object_get_pages(obj);
> > >     __i915_gem_object_unpin_pages(obj);
> > >     i915_gem_object_unlock(obj);
> > >     if (err) {
> > >         pr_err("__i915_gem_object_get_pages failed with err=%d\n", err);
> > >         goto out_ret;
> > >     }
> > >
> > > This seems to make the migration happen as expected without this
> > > patch.  So it seems the problem only exists on buffers that haven't
> > > gotten any backing storage yet (if I'm understanding get_pages
> > > correctly).
> > >
> > > One potential work-around (not sure if this is a good idea or not!)
> > > would be to do this inside dmabuf_attach().  Is this reliable?  Once
> > > it has pages will it always have pages?  Or are there crazy races I
> > > need to be worried about here?
> >
> > It turns out that the i915_ttm_adjust_gem_after_move() call in
> > ttm_object_init will always update the mm.region to system memory(so
> > that it matches the ttm resource), which seems reasonable given the
> > default system placeholder thing, but does seem slightly iffy since we
> > haven't actually moved/allocated anything.
> >
> > So effectively i915_ttm_migrate(SYSTEM) becomes a noop here since
> > mm.region == mr. Which ofc means when we actually call get_pages() all
> > that happens is that we allocate the pages in system memory(or without
> > this patch placements[0]). Also with this patch lmem+smem, will always
> > be placed in smem first, regardless of the placements ordering.
> >
> > For now we could maybe just split i915_ttm_adjust_gem_after_move() so
> > we skip the part which updates the mm.region here in the init portion,
> > since that should only happen when we try to place the object for
> > real?
>
> Doesn't that mean we would end up with obj->mm.region and
> obj->mm.res->mem_type are out-of-sync?  That seems bad.  I would think
> we'd want the two in sync at all times.
>
> It seems like the fundamental problem here is that, when it's created,
> the object isn't really in any memory region at all.  While I don't
> think obj->mm.region == NULL is allowed or a good idea, it does seem
> closer to the ground truth.
>
> Perhaps what we really want is for i915_gem_object_migrate to
> get_pages before it does the migration to ensure that pages exist.
> The only call to i915_gem_object_migrate in the code-base today is in
> the display code and it's immediately followed by pin_pages().  For
> that matter, maybe the call we actually want is
> i915_object_migrate_and_pin that does the whole lot.

I think long term at least we should track the curren region in the
ttm_resource struct (which can now be subclassed, yay, so we can stuff
anything we want into that one). And maybe make our regions proper
subclassss of ttm_resource_manager.

Even on platforms where ttm isn't used, where we will simply use the
same fields until the code is more unified.

With that there should be only the invairant placement list from
create_ext and the current region allocation left, and nothing else.
Also this would give us a very clear design for the objects which
change their type on igfx (like the shmem->phys_object stuff, which
currently just punches out the ->ops table and hopes for the best).

Also, get_pages is just a transition crutch too, we really want to
more explicit manage placement:
- for migration in dma-buf or display and other places where we must
limit the list of placements beyond what the user specified (and fail
it the intersection is empty)
- for execbuf, where we want to limit migration to avoid thrashing,
i.e. get_pages shouldn't just blindly try to move the buffer (but also
should not just never try to move the buffer, either is suboptimal).

All of this should be orthogonal to pin, which just nails something in
place wherever it is. Which is also extremely not what we currently
have, because right now pin is what allows to to say where you need
the object to be - in the old design only holding a pin prevented the
object from slipping away, now we'll move over to dma_resv_lock and
explicit migration, instead of smashing it all into the one pin
function call.

> Thoughts?

I think aside from starting to embed the ttm objects in the right
places and starting to use only those fields were we duplicate I think
we should leave things as-is for now. It's not pretty, but we need a
pile more ttm things in place first.
-Daniel

>
> --Jason
>
> P.S.  I'm going to go ahead and send another version with your other
> comments addressed.  We can keep this discussion going here for now.
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj

[Matthew Auld]

On Wed, 21 Jul 2021 at 21:11, Jason Ekstrand <jason@jlekstrand.net> wrote:
>
> On Mon, Jul 19, 2021 at 8:35 AM Matthew Auld
> <matthew.william.auld@gmail.com> wrote:
> >
> > On Fri, 16 Jul 2021 at 20:49, Jason Ekstrand <jason@jlekstrand.net> wrote:
> > >
> > > On Fri, Jul 16, 2021 at 1:45 PM Matthew Auld
> > > <matthew.william.auld@gmail.com> wrote:
> > > >
> > > > 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".
> > >
> > > I've switched the ____four_underscores call to a __two_underscores in
> > > the selftests and it had no effect, good or bad.  But, still, probably
> > > better to call that one.
> > >
> > > > >
> > > > > > > >
> > > > > > > > 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().
> > >
> > > Did a bit of experimenting along those lines and added the following
> > > to the self-test BEFORE the export/import:
> > >
> > >     i915_gem_object_lock(obj, NULL);
> > >     err = __i915_gem_object_get_pages(obj);
> > >     __i915_gem_object_unpin_pages(obj);
> > >     i915_gem_object_unlock(obj);
> > >     if (err) {
> > >         pr_err("__i915_gem_object_get_pages failed with err=%d\n", err);
> > >         goto out_ret;
> > >     }
> > >
> > > This seems to make the migration happen as expected without this
> > > patch.  So it seems the problem only exists on buffers that haven't
> > > gotten any backing storage yet (if I'm understanding get_pages
> > > correctly).
> > >
> > > One potential work-around (not sure if this is a good idea or not!)
> > > would be to do this inside dmabuf_attach().  Is this reliable?  Once
> > > it has pages will it always have pages?  Or are there crazy races I
> > > need to be worried about here?
> >
> > It turns out that the i915_ttm_adjust_gem_after_move() call in
> > ttm_object_init will always update the mm.region to system memory(so
> > that it matches the ttm resource), which seems reasonable given the
> > default system placeholder thing, but does seem slightly iffy since we
> > haven't actually moved/allocated anything.
> >
> > So effectively i915_ttm_migrate(SYSTEM) becomes a noop here since
> > mm.region == mr. Which ofc means when we actually call get_pages() all
> > that happens is that we allocate the pages in system memory(or without
> > this patch placements[0]). Also with this patch lmem+smem, will always
> > be placed in smem first, regardless of the placements ordering.
> >
> > For now we could maybe just split i915_ttm_adjust_gem_after_move() so
> > we skip the part which updates the mm.region here in the init portion,
> > since that should only happen when we try to place the object for
> > real?
>
> Doesn't that mean we would end up with obj->mm.region and
> obj->mm.res->mem_type are out-of-sync?  That seems bad.  I would think
> we'd want the two in sync at all times.

It likely doesn't matter since all roads lead to i915_ttm_get_pages()
when we need to actually use the object?

Also updating the mm.region in ttm_object_init() to reflect the dummy
ttm resource seems a little scary, since any existing is_lmem() check
now needs to happen after we place the object. Or at least the
existing callers(for kernel internal objects) might not have expected
that behaviour. Not sure if we checked all the callers.

>
> It seems like the fundamental problem here is that, when it's created,
> the object isn't really in any memory region at all.  While I don't
> think obj->mm.region == NULL is allowed or a good idea, it does seem
> closer to the ground truth.

Yeah, seems reasonable, especially for create_user where we don't know
the placement until we actually call get_pages(). I think for internal
users like with create_lmem() setting the mm.region early still makes
some sense?

>
> Perhaps what we really want is for i915_gem_object_migrate to
> get_pages before it does the migration to ensure that pages exist.
> The only call to i915_gem_object_migrate in the code-base today is in
> the display code and it's immediately followed by pin_pages().  For
> that matter, maybe the call we actually want is
> i915_object_migrate_and_pin that does the whole lot.

I guess the only downside is that we might end up doing a real
migration, with mempy or the blitter vs just changing the preferred
placement for later? I think just go with whatever you feel is the
simplest for now.

>
> Thoughts?
>
> --Jason
>
> P.S.  I'm going to go ahead and send another version with your other
> comments addressed.  We can keep this discussion going here for now.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj

[Matthew Auld]

On Thu, 22 Jul 2021 at 10:49, Matthew Auld
<matthew.william.auld@gmail.com> wrote:
>
> On Wed, 21 Jul 2021 at 21:11, Jason Ekstrand <jason@jlekstrand.net> wrote:
> >
> > On Mon, Jul 19, 2021 at 8:35 AM Matthew Auld
> > <matthew.william.auld@gmail.com> wrote:
> > >
> > > On Fri, 16 Jul 2021 at 20:49, Jason Ekstrand <jason@jlekstrand.net> wrote:
> > > >
> > > > On Fri, Jul 16, 2021 at 1:45 PM Matthew Auld
> > > > <matthew.william.auld@gmail.com> wrote:
> > > > >
> > > > > 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".
> > > >
> > > > I've switched the ____four_underscores call to a __two_underscores in
> > > > the selftests and it had no effect, good or bad.  But, still, probably
> > > > better to call that one.
> > > >
> > > > > >
> > > > > > > > >
> > > > > > > > > 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().
> > > >
> > > > Did a bit of experimenting along those lines and added the following
> > > > to the self-test BEFORE the export/import:
> > > >
> > > >     i915_gem_object_lock(obj, NULL);
> > > >     err = __i915_gem_object_get_pages(obj);
> > > >     __i915_gem_object_unpin_pages(obj);
> > > >     i915_gem_object_unlock(obj);
> > > >     if (err) {
> > > >         pr_err("__i915_gem_object_get_pages failed with err=%d\n", err);
> > > >         goto out_ret;
> > > >     }
> > > >
> > > > This seems to make the migration happen as expected without this
> > > > patch.  So it seems the problem only exists on buffers that haven't
> > > > gotten any backing storage yet (if I'm understanding get_pages
> > > > correctly).
> > > >
> > > > One potential work-around (not sure if this is a good idea or not!)
> > > > would be to do this inside dmabuf_attach().  Is this reliable?  Once
> > > > it has pages will it always have pages?  Or are there crazy races I
> > > > need to be worried about here?
> > >
> > > It turns out that the i915_ttm_adjust_gem_after_move() call in
> > > ttm_object_init will always update the mm.region to system memory(so
> > > that it matches the ttm resource), which seems reasonable given the
> > > default system placeholder thing, but does seem slightly iffy since we
> > > haven't actually moved/allocated anything.
> > >
> > > So effectively i915_ttm_migrate(SYSTEM) becomes a noop here since
> > > mm.region == mr. Which ofc means when we actually call get_pages() all
> > > that happens is that we allocate the pages in system memory(or without
> > > this patch placements[0]). Also with this patch lmem+smem, will always
> > > be placed in smem first, regardless of the placements ordering.
> > >
> > > For now we could maybe just split i915_ttm_adjust_gem_after_move() so
> > > we skip the part which updates the mm.region here in the init portion,
> > > since that should only happen when we try to place the object for
> > > real?
> >
> > Doesn't that mean we would end up with obj->mm.region and
> > obj->mm.res->mem_type are out-of-sync?  That seems bad.  I would think
> > we'd want the two in sync at all times.
>
> It likely doesn't matter since all roads lead to i915_ttm_get_pages()
> when we need to actually use the object?
>
> Also updating the mm.region in ttm_object_init() to reflect the dummy
> ttm resource seems a little scary, since any existing is_lmem() check
> now needs to happen after we place the object. Or at least the
> existing callers(for kernel internal objects) might not have expected
> that behaviour. Not sure if we checked all the callers.
>
> >
> > It seems like the fundamental problem here is that, when it's created,
> > the object isn't really in any memory region at all.  While I don't
> > think obj->mm.region == NULL is allowed or a good idea, it does seem
> > closer to the ground truth.
>
> Yeah, seems reasonable, especially for create_user where we don't know
> the placement until we actually call get_pages(). I think for internal
> users like with create_lmem() setting the mm.region early still makes
> some sense?
>
> >
> > Perhaps what we really want is for i915_gem_object_migrate to
> > get_pages before it does the migration to ensure that pages exist.
> > The only call to i915_gem_object_migrate in the code-base today is in
> > the display code and it's immediately followed by pin_pages().  For
> > that matter, maybe the call we actually want is
> > i915_object_migrate_and_pin that does the whole lot.
>
> I guess the only downside is that we might end up doing a real
> migration, with mempy or the blitter vs just changing the preferred
> placement for later? I think just go with whatever you feel is the
> simplest for now.

Another cheapo could be to drop the mr == mm.region noop, and just try
to place the object at mr anyway?

>
> >
> > Thoughts?
> >
> > --Jason
> >
> > P.S.  I'm going to go ahead and send another version with your other
> > comments addressed.  We can keep this discussion going here for now.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/7] drm/i915/gem/ttm: Place new BOs in the requested region

[Thomas Hellström]

Hi, Jason,

On 7/16/21 12:38 AM, Jason Ekstrand 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().

This is done on purpose. When objects are initially created in SMEM, 
they are created in "Limbo", meaning they have no pages and costly 
allocation and clearing is deferred to first get_pages().

So we shouldn't be doing this.

/Thomas

Re: [Intel-gfx] [PATCH 4/7] drm/i915/gem/ttm: Place new BOs in the requested region

[Thomas Hellström]

On 8/4/21 8:49 AM, Thomas Hellström wrote:
> Hi, Jason,
>
> On 7/16/21 12:38 AM, Jason Ekstrand 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().
>
> This is done on purpose. When objects are initially created in SMEM, 
> they are created in "Limbo", meaning they have no pages and costly 
> allocation and clearing is deferred to first get_pages().
>
> So we shouldn't be doing this.

Ah, I see Matthew already responded on this. Sorry for the noise.

/Thomas

Re: [Intel-gfx] [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj

[Thomas Hellström]

Hi,

On 7/22/21 11:59 AM, Matthew Auld wrote:
> On Thu, 22 Jul 2021 at 10:49, Matthew Auld
> <matthew.william.auld@gmail.com> wrote:
>> On Wed, 21 Jul 2021 at 21:11, Jason Ekstrand <jason@jlekstrand.net> wrote:
>>> On Mon, Jul 19, 2021 at 8:35 AM Matthew Auld
>>> <matthew.william.auld@gmail.com> wrote:
>>>> On Fri, 16 Jul 2021 at 20:49, Jason Ekstrand <jason@jlekstrand.net> wrote:
>>>>> On Fri, Jul 16, 2021 at 1:45 PM Matthew Auld
>>>>> <matthew.william.auld@gmail.com> wrote:
>>>>>> 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".
>>>>> I've switched the ____four_underscores call to a __two_underscores in
>>>>> the selftests and it had no effect, good or bad.  But, still, probably
>>>>> better to call that one.
>>>>>
>>>>>>>>>> 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().
>>>>> Did a bit of experimenting along those lines and added the following
>>>>> to the self-test BEFORE the export/import:
>>>>>
>>>>>      i915_gem_object_lock(obj, NULL);
>>>>>      err = __i915_gem_object_get_pages(obj);
>>>>>      __i915_gem_object_unpin_pages(obj);
>>>>>      i915_gem_object_unlock(obj);
>>>>>      if (err) {
>>>>>          pr_err("__i915_gem_object_get_pages failed with err=%d\n", err);
>>>>>          goto out_ret;
>>>>>      }
>>>>>
>>>>> This seems to make the migration happen as expected without this
>>>>> patch.  So it seems the problem only exists on buffers that haven't
>>>>> gotten any backing storage yet (if I'm understanding get_pages
>>>>> correctly).
>>>>>
>>>>> One potential work-around (not sure if this is a good idea or not!)
>>>>> would be to do this inside dmabuf_attach().  Is this reliable?  Once
>>>>> it has pages will it always have pages?  Or are there crazy races I
>>>>> need to be worried about here?
>>>> It turns out that the i915_ttm_adjust_gem_after_move() call in
>>>> ttm_object_init will always update the mm.region to system memory(so
>>>> that it matches the ttm resource), which seems reasonable given the
>>>> default system placeholder thing, but does seem slightly iffy since we
>>>> haven't actually moved/allocated anything.
>>>>
>>>> So effectively i915_ttm_migrate(SYSTEM) becomes a noop here since
>>>> mm.region == mr. Which ofc means when we actually call get_pages() all
>>>> that happens is that we allocate the pages in system memory(or without
>>>> this patch placements[0]). Also with this patch lmem+smem, will always
>>>> be placed in smem first, regardless of the placements ordering.
>>>>
>>>> For now we could maybe just split i915_ttm_adjust_gem_after_move() so
>>>> we skip the part which updates the mm.region here in the init portion,
>>>> since that should only happen when we try to place the object for
>>>> real?
>>> Doesn't that mean we would end up with obj->mm.region and
>>> obj->mm.res->mem_type are out-of-sync?  That seems bad.  I would think
>>> we'd want the two in sync at all times.
>> It likely doesn't matter since all roads lead to i915_ttm_get_pages()
>> when we need to actually use the object?
>>
>> Also updating the mm.region in ttm_object_init() to reflect the dummy
>> ttm resource seems a little scary, since any existing is_lmem() check
>> now needs to happen after we place the object. Or at least the
>> existing callers(for kernel internal objects) might not have expected
>> that behaviour. Not sure if we checked all the callers.
>>
>>> It seems like the fundamental problem here is that, when it's created,
>>> the object isn't really in any memory region at all.  While I don't
>>> think obj->mm.region == NULL is allowed or a good idea, it does seem
>>> closer to the ground truth.
>> Yeah, seems reasonable, especially for create_user where we don't know
>> the placement until we actually call get_pages(). I think for internal
>> users like with create_lmem() setting the mm.region early still makes
>> some sense?
>>
>>> Perhaps what we really want is for i915_gem_object_migrate to
>>> get_pages before it does the migration to ensure that pages exist.
>>> The only call to i915_gem_object_migrate in the code-base today is in
>>> the display code and it's immediately followed by pin_pages().  For
>>> that matter, maybe the call we actually want is
>>> i915_object_migrate_and_pin that does the whole lot.
>> I guess the only downside is that we might end up doing a real
>> migration, with mempy or the blitter vs just changing the preferred
>> placement for later? I think just go with whatever you feel is the
>> simplest for now.
> Another cheapo could be to drop the mr == mm.region noop, and just try
> to place the object at mr anyway?
>
There are a number of things to consider here,

First, as Jason found out what's keeping thing from working as intended 
is that we actually call into TTM get_pages() after migration, since the 
object isn't populated with pages yet. That's indeed a bug.

We should probably have migrate be migrate_and_populate(): Whatever 
kernel code decides to migrate needs to hold the object lock over the 
operation where data needs to be migrated or in the worst case call 
pin() under the lock which currently needs to be the case for dma-buf 
and display.

If we blindly just look at obj->mm.region() in get_pages() then if an 
object with allowable placements in lmem and smem initially gets placed 
in lmem, and then evicted to smem it will never migrate back to lmem 
unless if there is an explicit i915_gem_object_migrate(), but again, 
that's perhaps what we want? I guess we need to more clearly define the 
migration policies; for example should we attempt to migrate evicted 
buffers back to lmem on each execbuf where they are referenced, even if 
they haven't lost their pages?

On region dicrepance between gem and TTM there is a short DOC: section 
in i915_gem_ttm.c

/Thomas


>>> Thoughts?
>>>
>>> --Jason
>>>
>>> P.S.  I'm going to go ahead and send another version with your other
>>> comments addressed.  We can keep this discussion going here for now.

Re: [Intel-gfx] [PATCH 5/7] drm/i915/gem/ttm: Respect the objection region in placement_from_obj

[Daniel Vetter]

On Wed, Aug 4, 2021 at 10:00 AM Thomas Hellström
<thomas.hellstrom@linux.intel.com> wrote:
>
> Hi,
>
> On 7/22/21 11:59 AM, Matthew Auld wrote:
> > On Thu, 22 Jul 2021 at 10:49, Matthew Auld
> > <matthew.william.auld@gmail.com> wrote:
> >> On Wed, 21 Jul 2021 at 21:11, Jason Ekstrand <jason@jlekstrand.net> wrote:
> >>> On Mon, Jul 19, 2021 at 8:35 AM Matthew Auld
> >>> <matthew.william.auld@gmail.com> wrote:
> >>>> On Fri, 16 Jul 2021 at 20:49, Jason Ekstrand <jason@jlekstrand.net> wrote:
> >>>>> On Fri, Jul 16, 2021 at 1:45 PM Matthew Auld
> >>>>> <matthew.william.auld@gmail.com> wrote:
> >>>>>> 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".
> >>>>> I've switched the ____four_underscores call to a __two_underscores in
> >>>>> the selftests and it had no effect, good or bad.  But, still, probably
> >>>>> better to call that one.
> >>>>>
> >>>>>>>>>> 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().
> >>>>> Did a bit of experimenting along those lines and added the following
> >>>>> to the self-test BEFORE the export/import:
> >>>>>
> >>>>>      i915_gem_object_lock(obj, NULL);
> >>>>>      err = __i915_gem_object_get_pages(obj);
> >>>>>      __i915_gem_object_unpin_pages(obj);
> >>>>>      i915_gem_object_unlock(obj);
> >>>>>      if (err) {
> >>>>>          pr_err("__i915_gem_object_get_pages failed with err=%d\n", err);
> >>>>>          goto out_ret;
> >>>>>      }
> >>>>>
> >>>>> This seems to make the migration happen as expected without this
> >>>>> patch.  So it seems the problem only exists on buffers that haven't
> >>>>> gotten any backing storage yet (if I'm understanding get_pages
> >>>>> correctly).
> >>>>>
> >>>>> One potential work-around (not sure if this is a good idea or not!)
> >>>>> would be to do this inside dmabuf_attach().  Is this reliable?  Once
> >>>>> it has pages will it always have pages?  Or are there crazy races I
> >>>>> need to be worried about here?
> >>>> It turns out that the i915_ttm_adjust_gem_after_move() call in
> >>>> ttm_object_init will always update the mm.region to system memory(so
> >>>> that it matches the ttm resource), which seems reasonable given the
> >>>> default system placeholder thing, but does seem slightly iffy since we
> >>>> haven't actually moved/allocated anything.
> >>>>
> >>>> So effectively i915_ttm_migrate(SYSTEM) becomes a noop here since
> >>>> mm.region == mr. Which ofc means when we actually call get_pages() all
> >>>> that happens is that we allocate the pages in system memory(or without
> >>>> this patch placements[0]). Also with this patch lmem+smem, will always
> >>>> be placed in smem first, regardless of the placements ordering.
> >>>>
> >>>> For now we could maybe just split i915_ttm_adjust_gem_after_move() so
> >>>> we skip the part which updates the mm.region here in the init portion,
> >>>> since that should only happen when we try to place the object for
> >>>> real?
> >>> Doesn't that mean we would end up with obj->mm.region and
> >>> obj->mm.res->mem_type are out-of-sync?  That seems bad.  I would think
> >>> we'd want the two in sync at all times.
> >> It likely doesn't matter since all roads lead to i915_ttm_get_pages()
> >> when we need to actually use the object?
> >>
> >> Also updating the mm.region in ttm_object_init() to reflect the dummy
> >> ttm resource seems a little scary, since any existing is_lmem() check
> >> now needs to happen after we place the object. Or at least the
> >> existing callers(for kernel internal objects) might not have expected
> >> that behaviour. Not sure if we checked all the callers.
> >>
> >>> It seems like the fundamental problem here is that, when it's created,
> >>> the object isn't really in any memory region at all.  While I don't
> >>> think obj->mm.region == NULL is allowed or a good idea, it does seem
> >>> closer to the ground truth.
> >> Yeah, seems reasonable, especially for create_user where we don't know
> >> the placement until we actually call get_pages(). I think for internal
> >> users like with create_lmem() setting the mm.region early still makes
> >> some sense?
> >>
> >>> Perhaps what we really want is for i915_gem_object_migrate to
> >>> get_pages before it does the migration to ensure that pages exist.
> >>> The only call to i915_gem_object_migrate in the code-base today is in
> >>> the display code and it's immediately followed by pin_pages().  For
> >>> that matter, maybe the call we actually want is
> >>> i915_object_migrate_and_pin that does the whole lot.
> >> I guess the only downside is that we might end up doing a real
> >> migration, with mempy or the blitter vs just changing the preferred
> >> placement for later? I think just go with whatever you feel is the
> >> simplest for now.
> > Another cheapo could be to drop the mr == mm.region noop, and just try
> > to place the object at mr anyway?
> >
> There are a number of things to consider here,
>
> First, as Jason found out what's keeping thing from working as intended
> is that we actually call into TTM get_pages() after migration, since the
> object isn't populated with pages yet. That's indeed a bug.
>
> We should probably have migrate be migrate_and_populate(): Whatever
> kernel code decides to migrate needs to hold the object lock over the
> operation where data needs to be migrated or in the worst case call
> pin() under the lock which currently needs to be the case for dma-buf
> and display.
>
> If we blindly just look at obj->mm.region() in get_pages() then if an
> object with allowable placements in lmem and smem initially gets placed
> in lmem, and then evicted to smem it will never migrate back to lmem
> unless if there is an explicit i915_gem_object_migrate(), but again,
> that's perhaps what we want? I guess we need to more clearly define the
> migration policies; for example should we attempt to migrate evicted
> buffers back to lmem on each execbuf where they are referenced, even if
> they haven't lost their pages?

Looking at amdgpu things are indeed complicated:
- mmap adds some hints that cpu access is preferred (iirc at least) so
that the unmappable vram problems aren't too awful
- execbuf adds vram to the non-evict placement list whenever that
makes sense (i.e. preferred place and no inferred hint like mmap
access countering that)
- for eviction there's a ratelimit, to make sure we're not thrashing
terribly and spending all the gpu time moving buffers around with the
copy engine

Maybe another interim strategy would be to only evict non-busy
buffers, not sure ttm supports that already. We definitely don't want
to unconditionally force all buffers into lmem on every execbuf.
-Daniel


> On region dicrepance between gem and TTM there is a short DOC: section
> in i915_gem_ttm.c
>
> /Thomas
>
>
> >>> Thoughts?
> >>>
> >>> --Jason
> >>>
> >>> P.S.  I'm going to go ahead and send another version with your other
> >>> comments addressed.  We can keep this discussion going here for now.



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[Intel-gfx] [PATCH 1/7] drm/i915/gem: Check object_can_migrate from object_migrate

[Jason Ekstrand]

We don't roll them together entirely because there are still a couple
cases where we want a separate can_migrate check.  For instance, the
display code checks that you can migrate a buffer to LMEM before it
accepts it in fb_create.  The dma-buf import code also uses it to do an
early check and return a different error code if someone tries to attach
a LMEM-only dma-buf to another driver.

However, no one actually wants to call object_migrate when can_migrate
has failed.  The stated intention is for self-tests but none of those
actually take advantage of this unsafe migration.

Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
Cc: Daniel Vetter <daniel@ffwll.ch>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_object.c        | 13 ++-----------
 .../gpu/drm/i915/gem/selftests/i915_gem_migrate.c | 15 ---------------
 2 files changed, 2 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 9da7b288b7ede..f2244ae09a613 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -584,12 +584,6 @@ bool i915_gem_object_can_migrate(struct drm_i915_gem_object *obj,
  * completed yet, and to accomplish that, i915_gem_object_wait_migration()
  * must be called.
  *
- * This function is a bit more permissive than i915_gem_object_can_migrate()
- * to allow for migrating objects where the caller knows exactly what is
- * happening. For example within selftests. More specifically this
- * function allows migrating I915_BO_ALLOC_USER objects to regions
- * that are not in the list of allowable regions.
- *
  * Note: the @ww parameter is not used yet, but included to make sure
  * callers put some effort into obtaining a valid ww ctx if one is
  * available.
@@ -616,11 +610,8 @@ int i915_gem_object_migrate(struct drm_i915_gem_object *obj,
 	if (obj->mm.region == mr)
 		return 0;
 
-	if (!i915_gem_object_evictable(obj))
-		return -EBUSY;
-
-	if (!obj->ops->migrate)
-		return -EOPNOTSUPP;
+	if (!i915_gem_object_can_migrate(obj, id))
+		return -EINVAL;
 
 	return obj->ops->migrate(obj, mr);
 }
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c
index 0b7144d2991ca..28a700f08b49a 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c
@@ -61,11 +61,6 @@ static int igt_create_migrate(struct intel_gt *gt, enum intel_region_id src,
 		if (err)
 			continue;
 
-		if (!i915_gem_object_can_migrate(obj, dst)) {
-			err = -EINVAL;
-			continue;
-		}
-
 		err = i915_gem_object_migrate(obj, &ww, dst);
 		if (err)
 			continue;
@@ -114,11 +109,6 @@ static int lmem_pages_migrate_one(struct i915_gem_ww_ctx *ww,
 		return err;
 
 	if (i915_gem_object_is_lmem(obj)) {
-		if (!i915_gem_object_can_migrate(obj, INTEL_REGION_SMEM)) {
-			pr_err("object can't migrate to smem.\n");
-			return -EINVAL;
-		}
-
 		err = i915_gem_object_migrate(obj, ww, INTEL_REGION_SMEM);
 		if (err) {
 			pr_err("Object failed migration to smem\n");
@@ -137,11 +127,6 @@ static int lmem_pages_migrate_one(struct i915_gem_ww_ctx *ww,
 		}
 
 	} else {
-		if (!i915_gem_object_can_migrate(obj, INTEL_REGION_LMEM)) {
-			pr_err("object can't migrate to lmem.\n");
-			return -EINVAL;
-		}
-
 		err = i915_gem_object_migrate(obj, ww, INTEL_REGION_LMEM);
 		if (err) {
 			pr_err("Object failed migration to lmem\n");
-- 
2.31.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/7] drm/i915/gem: Check object_can_migrate from object_migrate

[Jason Ekstrand]

We don't roll them together entirely because there are still a couple
cases where we want a separate can_migrate check.  For instance, the
display code checks that you can migrate a buffer to LMEM before it
accepts it in fb_create.  The dma-buf import code also uses it to do an
early check and return a different error code if someone tries to attach
a LMEM-only dma-buf to another driver.

However, no one actually wants to call object_migrate when can_migrate
has failed.  The stated intention is for self-tests but none of those
actually take advantage of this unsafe migration.

Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
Cc: Daniel Vetter <daniel@ffwll.ch>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_object.c        | 13 ++-----------
 .../gpu/drm/i915/gem/selftests/i915_gem_migrate.c | 15 ---------------
 2 files changed, 2 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 9da7b288b7ede..f2244ae09a613 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -584,12 +584,6 @@ bool i915_gem_object_can_migrate(struct drm_i915_gem_object *obj,
  * completed yet, and to accomplish that, i915_gem_object_wait_migration()
  * must be called.
  *
- * This function is a bit more permissive than i915_gem_object_can_migrate()
- * to allow for migrating objects where the caller knows exactly what is
- * happening. For example within selftests. More specifically this
- * function allows migrating I915_BO_ALLOC_USER objects to regions
- * that are not in the list of allowable regions.
- *
  * Note: the @ww parameter is not used yet, but included to make sure
  * callers put some effort into obtaining a valid ww ctx if one is
  * available.
@@ -616,11 +610,8 @@ int i915_gem_object_migrate(struct drm_i915_gem_object *obj,
 	if (obj->mm.region == mr)
 		return 0;
 
-	if (!i915_gem_object_evictable(obj))
-		return -EBUSY;
-
-	if (!obj->ops->migrate)
-		return -EOPNOTSUPP;
+	if (!i915_gem_object_can_migrate(obj, id))
+		return -EINVAL;
 
 	return obj->ops->migrate(obj, mr);
 }
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c
index 0b7144d2991ca..28a700f08b49a 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c
@@ -61,11 +61,6 @@ static int igt_create_migrate(struct intel_gt *gt, enum intel_region_id src,
 		if (err)
 			continue;
 
-		if (!i915_gem_object_can_migrate(obj, dst)) {
-			err = -EINVAL;
-			continue;
-		}
-
 		err = i915_gem_object_migrate(obj, &ww, dst);
 		if (err)
 			continue;
@@ -114,11 +109,6 @@ static int lmem_pages_migrate_one(struct i915_gem_ww_ctx *ww,
 		return err;
 
 	if (i915_gem_object_is_lmem(obj)) {
-		if (!i915_gem_object_can_migrate(obj, INTEL_REGION_SMEM)) {
-			pr_err("object can't migrate to smem.\n");
-			return -EINVAL;
-		}
-
 		err = i915_gem_object_migrate(obj, ww, INTEL_REGION_SMEM);
 		if (err) {
 			pr_err("Object failed migration to smem\n");
@@ -137,11 +127,6 @@ static int lmem_pages_migrate_one(struct i915_gem_ww_ctx *ww,
 		}
 
 	} else {
-		if (!i915_gem_object_can_migrate(obj, INTEL_REGION_LMEM)) {
-			pr_err("object can't migrate to lmem.\n");
-			return -EINVAL;
-		}
-
 		err = i915_gem_object_migrate(obj, ww, INTEL_REGION_LMEM);
 		if (err) {
 			pr_err("Object failed migration to lmem\n");
-- 
2.31.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx