[RFC PATCH 1/7] drm/i915: instantiate ttm ranger manager for stolen memory

[RFC PATCH 1/7] drm/i915: instantiate ttm ranger manager for stolen memory

[Robert Beckett]

Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
---
 drivers/gpu/drm/i915/intel_region_ttm.c | 29 +++++++++++++++++++------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c
index 737ef3f4ab54..bb564b830c96 100644
--- a/drivers/gpu/drm/i915/intel_region_ttm.c
+++ b/drivers/gpu/drm/i915/intel_region_ttm.c
@@ -57,11 +57,17 @@ int intel_region_to_ttm_type(const struct intel_memory_region *mem)
 
 	GEM_BUG_ON(mem->type != INTEL_MEMORY_LOCAL &&
 		   mem->type != INTEL_MEMORY_MOCK &&
-		   mem->type != INTEL_MEMORY_SYSTEM);
+		   mem->type != INTEL_MEMORY_SYSTEM &&
+		   mem->type != INTEL_MEMORY_STOLEN_SYSTEM &&
+		   mem->type != INTEL_MEMORY_STOLEN_LOCAL);
 
 	if (mem->type == INTEL_MEMORY_SYSTEM)
 		return TTM_PL_SYSTEM;
 
+	if (mem->type == INTEL_MEMORY_STOLEN_SYSTEM ||
+	    mem->type == INTEL_MEMORY_STOLEN_LOCAL)
+		return TTM_PL_VRAM;
+
 	type = mem->instance + TTM_PL_PRIV;
 	GEM_BUG_ON(type >= TTM_NUM_MEM_TYPES);
 
@@ -85,10 +91,16 @@ int intel_region_ttm_init(struct intel_memory_region *mem)
 	int mem_type = intel_region_to_ttm_type(mem);
 	int ret;
 
-	ret = i915_ttm_buddy_man_init(bdev, mem_type, false,
-				      resource_size(&mem->region),
-				      mem->io_size,
-				      mem->min_page_size, PAGE_SIZE);
+	if (mem_type == TTM_PL_VRAM) {
+		ret = ttm_range_man_init(bdev, mem_type, false,
+					 resource_size(&mem->region) >> PAGE_SHIFT);
+		mem->is_range_manager = true;
+	} else {
+		ret = i915_ttm_buddy_man_init(bdev, mem_type, false,
+					      resource_size(&mem->region),
+					      mem->io_size,
+					      mem->min_page_size, PAGE_SIZE);
+	}
 	if (ret)
 		return ret;
 
@@ -108,6 +120,7 @@ int intel_region_ttm_init(struct intel_memory_region *mem)
 int intel_region_ttm_fini(struct intel_memory_region *mem)
 {
 	struct ttm_resource_manager *man = mem->region_private;
+	int mem_type = intel_region_to_ttm_type(mem);
 	int ret = -EBUSY;
 	int count;
 
@@ -138,8 +151,10 @@ int intel_region_ttm_fini(struct intel_memory_region *mem)
 	if (ret || !man)
 		return ret;
 
-	ret = i915_ttm_buddy_man_fini(&mem->i915->bdev,
-				      intel_region_to_ttm_type(mem));
+	if (mem_type == TTM_PL_VRAM)
+		ret = ttm_range_man_fini(&mem->i915->bdev, mem_type);
+	else
+		ret = i915_ttm_buddy_man_fini(&mem->i915->bdev, mem_type);
 	GEM_WARN_ON(ret);
 	mem->region_private = NULL;
 
-- 
2.25.1

[RFC PATCH 2/7] drm/i915: add ability to create memory region object in place

[Robert Beckett]

Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_region.c | 55 ++++++++++++++
 drivers/gpu/drm/i915/gem/i915_gem_region.h |  6 ++
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c    | 84 ++++++++++++++++++----
 drivers/gpu/drm/i915/intel_memory_region.h |  6 ++
 4 files changed, 136 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c b/drivers/gpu/drm/i915/gem/i915_gem_region.c
index c9b2e8b91053..e25ad0b9b636 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_region.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c
@@ -98,6 +98,61 @@ i915_gem_object_create_region(struct intel_memory_region *mem,
 	return ERR_PTR(err);
 }
 
+struct drm_i915_gem_object *
+i915_gem_object_create_region_in_place(struct intel_memory_region *mem,
+				       resource_size_t size,
+				       resource_size_t page_size,
+				       unsigned int flags,
+				       u64 start, u64 end)
+{
+	struct drm_i915_gem_object *obj;
+	resource_size_t default_page_size;
+	int err;
+
+	/*
+	 * NB: Our use of resource_size_t for the size stems from using struct
+	 * resource for the mem->region. We might need to revisit this in the
+	 * future.
+	 */
+
+	GEM_BUG_ON(flags & ~I915_BO_ALLOC_FLAGS);
+
+	if (!mem)
+		return ERR_PTR(-ENODEV);
+	if (!mem->ops->init_object_in_place)
+		return ERR_PTR(-EINVAL);
+
+	default_page_size = mem->min_page_size;
+	if (page_size)
+		default_page_size = page_size;
+
+	GEM_BUG_ON(!is_power_of_2_u64(default_page_size));
+	GEM_BUG_ON(default_page_size < PAGE_SIZE);
+
+	size = round_up(size, default_page_size);
+
+	GEM_BUG_ON(!size);
+	GEM_BUG_ON(!IS_ALIGNED(size, I915_GTT_MIN_ALIGNMENT));
+
+	if (i915_gem_object_size_2big(size))
+		return ERR_PTR(-E2BIG);
+
+	obj = i915_gem_object_alloc();
+	if (!obj)
+		return ERR_PTR(-ENOMEM);
+
+	err = mem->ops->init_object_in_place(mem, obj, size, page_size, flags, start, end);
+	if (err)
+		goto err_object_free;
+
+	trace_i915_gem_object_create(obj);
+	return obj;
+
+err_object_free:
+	i915_gem_object_free(obj);
+	return ERR_PTR(err);
+}
+
 /**
  * i915_gem_process_region - Iterate over all objects of a region using ops
  * to process and optionally skip objects
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.h b/drivers/gpu/drm/i915/gem/i915_gem_region.h
index fcaa12d657d4..0cad90ac4a92 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_region.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_region.h
@@ -56,6 +56,12 @@ i915_gem_object_create_region(struct intel_memory_region *mem,
 			      resource_size_t size,
 			      resource_size_t page_size,
 			      unsigned int flags);
+struct drm_i915_gem_object *
+i915_gem_object_create_region_in_place(struct intel_memory_region *mem,
+				       resource_size_t size,
+				       resource_size_t page_size,
+				       unsigned int flags,
+				       u64 start, u64 end);
 
 int i915_gem_process_region(struct intel_memory_region *mr,
 			    struct i915_gem_apply_to_region *apply);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 45cc5837ce00..35d1bde19267 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -1131,20 +1131,12 @@ void i915_ttm_bo_destroy(struct ttm_buffer_object *bo)
 	}
 }
 
-/**
- * __i915_gem_ttm_object_init - Initialize a ttm-backed i915 gem object
- * @mem: The initial memory region for the object.
- * @obj: The gem object.
- * @size: Object size in bytes.
- * @flags: gem object flags.
- *
- * Return: 0 on success, negative error code on failure.
- */
-int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
-			       struct drm_i915_gem_object *obj,
-			       resource_size_t size,
-			       resource_size_t page_size,
-			       unsigned int flags)
+static int __i915_gem_ttm_object_init_with_placement(struct intel_memory_region *mem,
+						     struct drm_i915_gem_object *obj,
+						     resource_size_t size,
+						     resource_size_t page_size,
+						     unsigned int flags,
+						     struct ttm_placement *placement)
 {
 	static struct lock_class_key lock_class;
 	struct drm_i915_private *i915 = mem->i915;
@@ -1188,7 +1180,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,
+				   bo_type, placement,
 				   page_size >> PAGE_SHIFT,
 				   &ctx, NULL, NULL, i915_ttm_bo_destroy);
 	if (ret)
@@ -1204,8 +1196,70 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
 	return 0;
 }
 
+/**
+ * __i915_gem_ttm_object_init - Initialize a ttm-backed i915 gem object
+ * @mem: The initial memory region for the object.
+ * @obj: The gem object.
+ * @size: Object size in bytes.
+ * @flags: gem object flags.
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
+			       struct drm_i915_gem_object *obj,
+			       resource_size_t size,
+			       resource_size_t page_size,
+			       unsigned int flags)
+{
+	return __i915_gem_ttm_object_init_with_placement(mem, obj, size,
+							 page_size, flags,
+							 &i915_sys_placement);
+}
+
+/**
+ * i915_gem_ttm_object_init_in_place - Initialize a ttm-backed i915 gem object in place
+ * @mem: The initial memory region for the object.
+ * @obj: The gem object.
+ * @size: Object size in bytes.
+ * @page_size: Required page size.
+ * @flags: gem object flags.
+ * @start: start of range to insert in to.
+ * @end: end of range to insert in to.
+ *
+ * Initializes a ttm-backed i915 gem object with a predefined
+ * placement and range.
+ * Can be used to create an object around a pre-reserved area.
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+static int i915_gem_ttm_object_init_in_place(struct intel_memory_region *mem,
+					     struct drm_i915_gem_object *obj,
+					     resource_size_t size,
+					     resource_size_t page_size,
+					     unsigned int flags,
+					     u64 start,
+					     u64 end)
+{
+	struct ttm_place place;
+	struct ttm_placement placement = {
+		.num_placement = 1,
+		.placement = &place,
+		.num_busy_placement = 1,
+		.busy_placement = &place,
+	};
+
+	i915_ttm_place_from_region(mem, &place, flags);
+	place.fpfn = PFN_DOWN(start);
+	place.lpfn = PFN_UP(end);
+
+	return __i915_gem_ttm_object_init_with_placement(mem, obj, size,
+							 page_size, flags,
+							 &placement);
+}
+
 static const struct intel_memory_region_ops ttm_system_region_ops = {
 	.init_object = __i915_gem_ttm_object_init,
+	.init_object_in_place = i915_gem_ttm_object_init_in_place,
 	.release = intel_region_ttm_fini,
 };
 
diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h
index 21dcbd620758..eb72997df8f7 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.h
+++ b/drivers/gpu/drm/i915/intel_memory_region.h
@@ -57,6 +57,12 @@ struct intel_memory_region_ops {
 			   resource_size_t size,
 			   resource_size_t page_size,
 			   unsigned int flags);
+	int (*init_object_in_place)(struct intel_memory_region *mem,
+				    struct drm_i915_gem_object *obj,
+				    resource_size_t size,
+				    resource_size_t page_size,
+				    unsigned int flags,
+				    u64 start, u64 end);
 };
 
 struct intel_memory_region {
-- 
2.25.1

[RFC PATCH 3/7] drm/i915: use gem objects to track stolen nodes

[Robert Beckett]

Construct gem objects around stolen nodes.
This stops the abuse of interfaces and aids future patches that done use
drm nodes for stolen areas.

Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
---
 drivers/gpu/drm/i915/display/intel_fbc.c   | 72 ++++++++++++----------
 drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 60 ++++++++++++++++++
 drivers/gpu/drm/i915/gem/i915_gem_stolen.h |  7 +++
 3 files changed, 106 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index 142280b6ce6d..9df64ecab70e 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -92,8 +92,8 @@ struct intel_fbc {
 	unsigned int possible_framebuffer_bits;
 	unsigned int busy_bits;
 
-	struct drm_mm_node compressed_fb;
-	struct drm_mm_node compressed_llb;
+	struct drm_i915_gem_object *compressed_fb;
+	struct drm_i915_gem_object *compressed_llb;
 
 	enum intel_fbc_id id;
 
@@ -331,16 +331,18 @@ static void i8xx_fbc_nuke(struct intel_fbc *fbc)
 static void i8xx_fbc_program_cfb(struct intel_fbc *fbc)
 {
 	struct drm_i915_private *i915 = fbc->i915;
+	u64 fb_offset = i915_gem_object_stolen_offset(fbc->compressed_fb);
+	u64 llb_offset = i915_gem_object_stolen_offset(fbc->compressed_llb);
 
 	GEM_BUG_ON(range_overflows_end_t(u64, i915->dsm.start,
-					 fbc->compressed_fb.start, U32_MAX));
+					 fb_offset, U32_MAX));
 	GEM_BUG_ON(range_overflows_end_t(u64, i915->dsm.start,
-					 fbc->compressed_llb.start, U32_MAX));
+					 llb_offset, U32_MAX));
 
 	intel_de_write(i915, FBC_CFB_BASE,
-		       i915->dsm.start + fbc->compressed_fb.start);
+		       i915->dsm.start + fb_offset);
 	intel_de_write(i915, FBC_LL_BASE,
-		       i915->dsm.start + fbc->compressed_llb.start);
+		       i915->dsm.start + llb_offset);
 }
 
 static const struct intel_fbc_funcs i8xx_fbc_funcs = {
@@ -449,7 +451,8 @@ static void g4x_fbc_program_cfb(struct intel_fbc *fbc)
 {
 	struct drm_i915_private *i915 = fbc->i915;
 
-	intel_de_write(i915, DPFC_CB_BASE, fbc->compressed_fb.start);
+	intel_de_write(i915, DPFC_CB_BASE,
+		       i915_gem_object_stolen_offset(fbc->compressed_fb));
 }
 
 static const struct intel_fbc_funcs g4x_fbc_funcs = {
@@ -500,7 +503,8 @@ static void ilk_fbc_program_cfb(struct intel_fbc *fbc)
 {
 	struct drm_i915_private *i915 = fbc->i915;
 
-	intel_de_write(i915, ILK_DPFC_CB_BASE(fbc->id), fbc->compressed_fb.start);
+	intel_de_write(i915, ILK_DPFC_CB_BASE(fbc->id),
+			i915_gem_object_stolen_offset(fbc->compressed_fb));
 }
 
 static const struct intel_fbc_funcs ilk_fbc_funcs = {
@@ -740,21 +744,24 @@ static int find_compression_limit(struct intel_fbc *fbc,
 {
 	struct drm_i915_private *i915 = fbc->i915;
 	u64 end = intel_fbc_stolen_end(i915);
-	int ret, limit = min_limit;
+	int limit = min_limit;
+	struct drm_i915_gem_object *obj;
 
 	size /= limit;
 
 	/* Try to over-allocate to reduce reallocations and fragmentation. */
-	ret = i915_gem_stolen_insert_node_in_range(i915, &fbc->compressed_fb,
-						   size <<= 1, 4096, 0, end);
-	if (ret == 0)
+	obj = i915_gem_object_create_stolen_in_range(i915, size <<= 1, 4096, 0, end);
+	if (!IS_ERR(obj)) {
+		fbc->compressed_fb = obj;
 		return limit;
+	}
 
 	for (; limit <= intel_fbc_max_limit(i915); limit <<= 1) {
-		ret = i915_gem_stolen_insert_node_in_range(i915, &fbc->compressed_fb,
-							   size >>= 1, 4096, 0, end);
-		if (ret == 0)
+		obj = i915_gem_object_create_stolen_in_range(i915, size >>= 1, 4096, 0, end);
+		if (!IS_ERR(obj)) {
+			fbc->compressed_fb = obj;
 			return limit;
+		}
 	}
 
 	return 0;
@@ -765,17 +772,19 @@ static int intel_fbc_alloc_cfb(struct intel_fbc *fbc,
 {
 	struct drm_i915_private *i915 = fbc->i915;
 	int ret;
+	struct drm_i915_gem_object *obj;
 
-	drm_WARN_ON(&i915->drm,
-		    drm_mm_node_allocated(&fbc->compressed_fb));
-	drm_WARN_ON(&i915->drm,
-		    drm_mm_node_allocated(&fbc->compressed_llb));
+	drm_WARN_ON(&i915->drm, fbc->compressed_fb);
+	drm_WARN_ON(&i915->drm, fbc->compressed_llb);
 
 	if (DISPLAY_VER(i915) < 5 && !IS_G4X(i915)) {
-		ret = i915_gem_stolen_insert_node(i915, &fbc->compressed_llb,
-						  4096, 4096);
-		if (ret)
+		obj = i915_gem_object_create_stolen_in_range(i915, 4096, 4096,
+							     I915_GEM_STOLEN_BIAS, U64_MAX);
+		if (IS_ERR(obj)) {
+			ret = PTR_ERR(obj);
 			goto err;
+		}
+		fbc->compressed_llb = obj;
 	}
 
 	ret = find_compression_limit(fbc, size, min_limit);
@@ -788,14 +797,13 @@ static int intel_fbc_alloc_cfb(struct intel_fbc *fbc,
 	fbc->limit = ret;
 
 	drm_dbg_kms(&i915->drm,
-		    "reserved %llu bytes of contiguous stolen space for FBC, limit: %d\n",
-		    fbc->compressed_fb.size, fbc->limit);
+		    "reserved %lu bytes of contiguous stolen space for FBC, limit: %d\n",
+		    fbc->compressed_fb->base.size, fbc->limit);
 
 	return 0;
 
 err_llb:
-	if (drm_mm_node_allocated(&fbc->compressed_llb))
-		i915_gem_stolen_remove_node(i915, &fbc->compressed_llb);
+	i915_gem_object_put(fetch_and_zero(&fbc->compressed_llb));
 err:
 	if (drm_mm_initialized(&i915->mm.stolen))
 		drm_info_once(&i915->drm, "not enough stolen space for compressed buffer (need %d more bytes), disabling. Hint: you may be able to increase stolen memory size in the BIOS to avoid this.\n", size);
@@ -809,15 +817,13 @@ static void intel_fbc_program_cfb(struct intel_fbc *fbc)
 
 static void __intel_fbc_cleanup_cfb(struct intel_fbc *fbc)
 {
-	struct drm_i915_private *i915 = fbc->i915;
-
 	if (WARN_ON(intel_fbc_hw_is_active(fbc)))
 		return;
 
-	if (drm_mm_node_allocated(&fbc->compressed_llb))
-		i915_gem_stolen_remove_node(i915, &fbc->compressed_llb);
-	if (drm_mm_node_allocated(&fbc->compressed_fb))
-		i915_gem_stolen_remove_node(i915, &fbc->compressed_fb);
+	if (fbc->compressed_llb)
+		i915_gem_object_put(fetch_and_zero(&fbc->compressed_llb));
+	if (fbc->compressed_fb)
+		i915_gem_object_put(fetch_and_zero(&fbc->compressed_fb));
 }
 
 void intel_fbc_cleanup(struct drm_i915_private *i915)
@@ -1023,7 +1029,7 @@ static bool intel_fbc_is_cfb_ok(const struct intel_plane_state *plane_state)
 	struct intel_fbc *fbc = plane->fbc;
 
 	return intel_fbc_min_limit(plane_state) <= fbc->limit &&
-		intel_fbc_cfb_size(plane_state) <= fbc->compressed_fb.size * fbc->limit;
+		intel_fbc_cfb_size(plane_state) <= fbc->compressed_fb->base.size * fbc->limit;
 }
 
 static bool intel_fbc_is_ok(const struct intel_plane_state *plane_state)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
index 0bf8f61134af..265133cb2a47 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
@@ -907,6 +907,66 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_i915_private *i915,
 	return ERR_PTR(ret);
 }
 
+struct drm_i915_gem_object *
+i915_gem_object_create_stolen_in_range(struct drm_i915_private *i915,
+				       resource_size_t size,
+				       resource_size_t alignment,
+				       u64 start, u64 end)
+{
+	struct intel_memory_region *mem = i915->mm.stolen_region;
+	struct drm_i915_gem_object *obj;
+	struct drm_mm_node *stolen;
+	int ret;
+
+	if (!drm_mm_initialized(&i915->mm.stolen))
+		return ERR_PTR(-ENODEV);
+
+	/* KISS and expect everything to be page-aligned */
+	if (GEM_WARN_ON(size == 0) ||
+	    GEM_WARN_ON(!IS_ALIGNED(size, mem->min_page_size)) ||
+	    GEM_WARN_ON(!IS_ALIGNED(alignment, mem->min_page_size)))
+		return ERR_PTR(-EINVAL);
+
+	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
+	if (!stolen)
+		return ERR_PTR(-ENOMEM);
+
+	ret = i915_gem_stolen_insert_node_in_range(i915, stolen, size,
+						   alignment, start, end);
+	if (ret)
+		goto err_free;
+
+	obj = i915_gem_object_alloc();
+	if (!obj) {
+		ret = -ENOMEM;
+		goto err_stolen;
+	}
+
+	ret = __i915_gem_object_create_stolen(mem, obj, stolen);
+	if (ret)
+		goto err_object_free;
+
+	i915_gem_object_set_cache_coherency(obj, I915_CACHE_NONE);
+	return obj;
+
+err_object_free:
+	i915_gem_object_free(obj);
+err_stolen:
+	i915_gem_stolen_remove_node(i915, stolen);
+err_free:
+	kfree(stolen);
+	return ERR_PTR(ret);
+}
+
+u64 i915_gem_object_stolen_offset(const struct drm_i915_gem_object *obj)
+{
+	if (!obj || !i915_gem_object_is_stolen(obj))
+		return 0;
+
+	return obj->stolen->start;
+}
+
+
 bool i915_gem_object_is_stolen(const struct drm_i915_gem_object *obj)
 {
 	return obj->ops == &i915_gem_object_stolen_ops;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.h b/drivers/gpu/drm/i915/gem/i915_gem_stolen.h
index ccdf7befc571..494e90f130f4 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.h
@@ -35,6 +35,13 @@ struct drm_i915_gem_object *
 i915_gem_object_create_stolen_for_preallocated(struct drm_i915_private *dev_priv,
 					       resource_size_t stolen_offset,
 					       resource_size_t size);
+struct drm_i915_gem_object *
+i915_gem_object_create_stolen_in_range(struct drm_i915_private *i915,
+				       resource_size_t size,
+				       resource_size_t alignment,
+				       u64 start, u64 end);
+u64 i915_gem_object_stolen_offset(const struct drm_i915_gem_object *obj);
+
 
 bool i915_gem_object_is_stolen(const struct drm_i915_gem_object *obj);
 
-- 
2.25.1

[RFC PATCH 4/7] drm/i915: stolen memory use ttm backend

[Robert Beckett]

Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 385 ++-------------------
 drivers/gpu/drm/i915/gem/i915_gem_stolen.h |   9 -
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c    |  14 +-
 drivers/gpu/drm/i915/gem/i915_gem_ttm.h    |   7 +
 4 files changed, 40 insertions(+), 375 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
index 265133cb2a47..e58f9902ef47 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
@@ -4,19 +4,22 @@
  * Copyright © 2008-2012 Intel Corporation
  */
 
+#include "drm/ttm/ttm_placement.h"
+#include "gem/i915_gem_object_types.h"
 #include <linux/errno.h>
 #include <linux/mutex.h>
 
-#include <drm/drm_mm.h>
 #include <drm/i915_drm.h>
 
 #include "gem/i915_gem_lmem.h"
 #include "gem/i915_gem_region.h"
+#include "gem/i915_gem_ttm.h"
 #include "i915_drv.h"
 #include "i915_gem_stolen.h"
 #include "i915_reg.h"
 #include "i915_vgpu.h"
 #include "intel_mchbar_regs.h"
+#include "intel_region_ttm.h"
 
 /*
  * The BIOS typically reserves some of the system's memory for the exclusive
@@ -30,46 +33,6 @@
  * for is a boon.
  */
 
-int i915_gem_stolen_insert_node_in_range(struct drm_i915_private *i915,
-					 struct drm_mm_node *node, u64 size,
-					 unsigned alignment, u64 start, u64 end)
-{
-	int ret;
-
-	if (!drm_mm_initialized(&i915->mm.stolen))
-		return -ENODEV;
-
-	/* WaSkipStolenMemoryFirstPage:bdw+ */
-	if (GRAPHICS_VER(i915) >= 8 && start < 4096)
-		start = 4096;
-
-	mutex_lock(&i915->mm.stolen_lock);
-	ret = drm_mm_insert_node_in_range(&i915->mm.stolen, node,
-					  size, alignment, 0,
-					  start, end, DRM_MM_INSERT_BEST);
-	mutex_unlock(&i915->mm.stolen_lock);
-
-	return ret;
-}
-
-int i915_gem_stolen_insert_node(struct drm_i915_private *i915,
-				struct drm_mm_node *node, u64 size,
-				unsigned alignment)
-{
-	return i915_gem_stolen_insert_node_in_range(i915, node,
-						    size, alignment,
-						    I915_GEM_STOLEN_BIAS,
-						    U64_MAX);
-}
-
-void i915_gem_stolen_remove_node(struct drm_i915_private *i915,
-				 struct drm_mm_node *node)
-{
-	mutex_lock(&i915->mm.stolen_lock);
-	drm_mm_remove_node(node);
-	mutex_unlock(&i915->mm.stolen_lock);
-}
-
 static int i915_adjust_stolen(struct drm_i915_private *i915,
 			      struct resource *dsm)
 {
@@ -170,14 +133,6 @@ static int i915_adjust_stolen(struct drm_i915_private *i915,
 	return 0;
 }
 
-static void i915_gem_cleanup_stolen(struct drm_i915_private *i915)
-{
-	if (!drm_mm_initialized(&i915->mm.stolen))
-		return;
-
-	drm_mm_takedown(&i915->mm.stolen);
-}
-
 static void g4x_get_stolen_reserved(struct drm_i915_private *i915,
 				    struct intel_uncore *uncore,
 				    resource_size_t *base,
@@ -510,216 +465,15 @@ static int i915_gem_init_stolen(struct intel_memory_region *mem)
 		return 0;
 
 	/* Basic memrange allocator for stolen space. */
-	drm_mm_init(&i915->mm.stolen, 0, i915->stolen_usable_size);
-
-	return 0;
-}
-
-static void dbg_poison(struct i915_ggtt *ggtt,
-		       dma_addr_t addr, resource_size_t size,
-		       u8 x)
-{
-#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
-	if (!drm_mm_node_allocated(&ggtt->error_capture))
-		return;
-
-	if (ggtt->vm.bind_async_flags & I915_VMA_GLOBAL_BIND)
-		return; /* beware stop_machine() inversion */
-
-	GEM_BUG_ON(!IS_ALIGNED(size, PAGE_SIZE));
-
-	mutex_lock(&ggtt->error_mutex);
-	while (size) {
-		void __iomem *s;
-
-		ggtt->vm.insert_page(&ggtt->vm, addr,
-				     ggtt->error_capture.start,
-				     I915_CACHE_NONE, 0);
-		mb();
-
-		s = io_mapping_map_wc(&ggtt->iomap,
-				      ggtt->error_capture.start,
-				      PAGE_SIZE);
-		memset_io(s, x, PAGE_SIZE);
-		io_mapping_unmap(s);
-
-		addr += PAGE_SIZE;
-		size -= PAGE_SIZE;
-	}
-	mb();
-	ggtt->vm.clear_range(&ggtt->vm, ggtt->error_capture.start, PAGE_SIZE);
-	mutex_unlock(&ggtt->error_mutex);
-#endif
-}
-
-static struct sg_table *
-i915_pages_create_for_stolen(struct drm_device *dev,
-			     resource_size_t offset, resource_size_t size)
-{
-	struct drm_i915_private *i915 = to_i915(dev);
-	struct sg_table *st;
-	struct scatterlist *sg;
-
-	GEM_BUG_ON(range_overflows(offset, size, resource_size(&i915->dsm)));
-
-	/* We hide that we have no struct page backing our stolen object
-	 * by wrapping the contiguous physical allocation with a fake
-	 * dma mapping in a single scatterlist.
-	 */
-
-	st = kmalloc(sizeof(*st), GFP_KERNEL);
-	if (st == NULL)
-		return ERR_PTR(-ENOMEM);
-
-	if (sg_alloc_table(st, 1, GFP_KERNEL)) {
-		kfree(st);
-		return ERR_PTR(-ENOMEM);
-	}
-
-	sg = st->sgl;
-	sg->offset = 0;
-	sg->length = size;
-
-	sg_dma_address(sg) = (dma_addr_t)i915->dsm.start + offset;
-	sg_dma_len(sg) = size;
-
-	return st;
-}
-
-static int i915_gem_object_get_pages_stolen(struct drm_i915_gem_object *obj)
-{
-	struct drm_i915_private *i915 = to_i915(obj->base.dev);
-	struct sg_table *pages =
-		i915_pages_create_for_stolen(obj->base.dev,
-					     obj->stolen->start,
-					     obj->stolen->size);
-	if (IS_ERR(pages))
-		return PTR_ERR(pages);
-
-	dbg_poison(to_gt(i915)->ggtt,
-		   sg_dma_address(pages->sgl),
-		   sg_dma_len(pages->sgl),
-		   POISON_INUSE);
-
-	__i915_gem_object_set_pages(obj, pages, obj->stolen->size);
-
-	return 0;
-}
-
-static void i915_gem_object_put_pages_stolen(struct drm_i915_gem_object *obj,
-					     struct sg_table *pages)
-{
-	struct drm_i915_private *i915 = to_i915(obj->base.dev);
-	/* Should only be called from i915_gem_object_release_stolen() */
-
-	dbg_poison(to_gt(i915)->ggtt,
-		   sg_dma_address(pages->sgl),
-		   sg_dma_len(pages->sgl),
-		   POISON_FREE);
-
-	sg_free_table(pages);
-	kfree(pages);
-}
-
-static void
-i915_gem_object_release_stolen(struct drm_i915_gem_object *obj)
-{
-	struct drm_i915_private *i915 = to_i915(obj->base.dev);
-	struct drm_mm_node *stolen = fetch_and_zero(&obj->stolen);
-
-	GEM_BUG_ON(!stolen);
-	i915_gem_stolen_remove_node(i915, stolen);
-	kfree(stolen);
-
-	i915_gem_object_release_memory_region(obj);
-}
-
-static const struct drm_i915_gem_object_ops i915_gem_object_stolen_ops = {
-	.name = "i915_gem_object_stolen",
-	.get_pages = i915_gem_object_get_pages_stolen,
-	.put_pages = i915_gem_object_put_pages_stolen,
-	.release = i915_gem_object_release_stolen,
-};
-
-static int __i915_gem_object_create_stolen(struct intel_memory_region *mem,
-					   struct drm_i915_gem_object *obj,
-					   struct drm_mm_node *stolen)
-{
-	static struct lock_class_key lock_class;
-	unsigned int cache_level;
-	unsigned int flags;
-	int err;
-
-	/*
-	 * Stolen objects are always physically contiguous since we just
-	 * allocate one big block underneath using the drm_mm range allocator.
-	 */
-	flags = I915_BO_ALLOC_CONTIGUOUS;
-
-	drm_gem_private_object_init(&mem->i915->drm, &obj->base, stolen->size);
-	i915_gem_object_init(obj, &i915_gem_object_stolen_ops, &lock_class, flags);
-
-	obj->stolen = stolen;
-	obj->read_domains = I915_GEM_DOMAIN_CPU | I915_GEM_DOMAIN_GTT;
-	cache_level = HAS_LLC(mem->i915) ? I915_CACHE_LLC : I915_CACHE_NONE;
-	i915_gem_object_set_cache_coherency(obj, cache_level);
-
-	if (WARN_ON(!i915_gem_object_trylock(obj, NULL)))
-		return -EBUSY;
-
-	i915_gem_object_init_memory_region(obj, mem);
-
-	err = i915_gem_object_pin_pages(obj);
-	if (err)
-		i915_gem_object_release_memory_region(obj);
-	i915_gem_object_unlock(obj);
-
-	return err;
-}
-
-static int _i915_gem_object_stolen_init(struct intel_memory_region *mem,
-					struct drm_i915_gem_object *obj,
-					resource_size_t size,
-					resource_size_t page_size,
-					unsigned int flags)
-{
-	struct drm_i915_private *i915 = mem->i915;
-	struct drm_mm_node *stolen;
-	int ret;
-
-	if (!drm_mm_initialized(&i915->mm.stolen))
-		return -ENODEV;
-
-	if (size == 0)
-		return -EINVAL;
-
-	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
-	if (!stolen)
-		return -ENOMEM;
-
-	ret = i915_gem_stolen_insert_node(i915, stolen, size,
-					  mem->min_page_size);
-	if (ret)
-		goto err_free;
-
-	ret = __i915_gem_object_create_stolen(mem, obj, stolen);
-	if (ret)
-		goto err_remove;
-
-	return 0;
-
-err_remove:
-	i915_gem_stolen_remove_node(i915, stolen);
-err_free:
-	kfree(stolen);
-	return ret;
+	return intel_region_ttm_init(mem);
 }
 
 struct drm_i915_gem_object *
 i915_gem_object_create_stolen(struct drm_i915_private *i915,
 			      resource_size_t size)
 {
-	return i915_gem_object_create_region(i915->mm.stolen_region, size, 0, 0);
+	return i915_gem_object_create_region(i915->mm.stolen_region, size, 0,
+					     I915_BO_ALLOC_CONTIGUOUS);
 }
 
 static int init_stolen_smem(struct intel_memory_region *mem)
@@ -731,16 +485,11 @@ static int init_stolen_smem(struct intel_memory_region *mem)
 	return i915_gem_init_stolen(mem);
 }
 
-static int release_stolen_smem(struct intel_memory_region *mem)
-{
-	i915_gem_cleanup_stolen(mem->i915);
-	return 0;
-}
-
 static const struct intel_memory_region_ops i915_region_stolen_smem_ops = {
 	.init = init_stolen_smem,
-	.release = release_stolen_smem,
-	.init_object = _i915_gem_object_stolen_init,
+	.release = intel_region_ttm_fini,
+	.init_object = __i915_gem_ttm_object_init,
+	.init_object_in_place = i915_gem_ttm_object_init_in_place,
 };
 
 static int init_stolen_lmem(struct intel_memory_region *mem)
@@ -774,14 +523,14 @@ static int init_stolen_lmem(struct intel_memory_region *mem)
 static int release_stolen_lmem(struct intel_memory_region *mem)
 {
 	io_mapping_fini(&mem->iomap);
-	i915_gem_cleanup_stolen(mem->i915);
-	return 0;
+	return intel_region_ttm_fini(mem);
 }
 
 static const struct intel_memory_region_ops i915_region_stolen_lmem_ops = {
 	.init = init_stolen_lmem,
 	.release = release_stolen_lmem,
-	.init_object = _i915_gem_object_stolen_init,
+	.init_object = __i915_gem_ttm_object_init,
+	.init_object_in_place = i915_gem_ttm_object_init_in_place,
 };
 
 struct intel_memory_region *
@@ -855,56 +604,10 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_i915_private *i915,
 					       resource_size_t stolen_offset,
 					       resource_size_t size)
 {
-	struct intel_memory_region *mem = i915->mm.stolen_region;
-	struct drm_i915_gem_object *obj;
-	struct drm_mm_node *stolen;
-	int ret;
-
-	if (!drm_mm_initialized(&i915->mm.stolen))
-		return ERR_PTR(-ENODEV);
-
-	drm_dbg(&i915->drm,
-		"creating preallocated stolen object: stolen_offset=%pa, size=%pa\n",
-		&stolen_offset, &size);
-
-	/* KISS and expect everything to be page-aligned */
-	if (GEM_WARN_ON(size == 0) ||
-	    GEM_WARN_ON(!IS_ALIGNED(size, mem->min_page_size)) ||
-	    GEM_WARN_ON(!IS_ALIGNED(stolen_offset, mem->min_page_size)))
-		return ERR_PTR(-EINVAL);
-
-	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
-	if (!stolen)
-		return ERR_PTR(-ENOMEM);
-
-	stolen->start = stolen_offset;
-	stolen->size = size;
-	mutex_lock(&i915->mm.stolen_lock);
-	ret = drm_mm_reserve_node(&i915->mm.stolen, stolen);
-	mutex_unlock(&i915->mm.stolen_lock);
-	if (ret)
-		goto err_free;
-
-	obj = i915_gem_object_alloc();
-	if (!obj) {
-		ret = -ENOMEM;
-		goto err_stolen;
-	}
-
-	ret = __i915_gem_object_create_stolen(mem, obj, stolen);
-	if (ret)
-		goto err_object_free;
-
-	i915_gem_object_set_cache_coherency(obj, I915_CACHE_NONE);
-	return obj;
-
-err_object_free:
-	i915_gem_object_free(obj);
-err_stolen:
-	i915_gem_stolen_remove_node(i915, stolen);
-err_free:
-	kfree(stolen);
-	return ERR_PTR(ret);
+	return i915_gem_object_create_region_in_place(i915->mm.stolen_region, size, 0,
+						      I915_BO_ALLOC_CONTIGUOUS,
+						      stolen_offset,
+						      stolen_offset + size);
 }
 
 struct drm_i915_gem_object *
@@ -913,61 +616,25 @@ i915_gem_object_create_stolen_in_range(struct drm_i915_private *i915,
 				       resource_size_t alignment,
 				       u64 start, u64 end)
 {
-	struct intel_memory_region *mem = i915->mm.stolen_region;
-	struct drm_i915_gem_object *obj;
-	struct drm_mm_node *stolen;
-	int ret;
-
-	if (!drm_mm_initialized(&i915->mm.stolen))
-		return ERR_PTR(-ENODEV);
-
-	/* KISS and expect everything to be page-aligned */
-	if (GEM_WARN_ON(size == 0) ||
-	    GEM_WARN_ON(!IS_ALIGNED(size, mem->min_page_size)) ||
-	    GEM_WARN_ON(!IS_ALIGNED(alignment, mem->min_page_size)))
-		return ERR_PTR(-EINVAL);
-
-	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
-	if (!stolen)
-		return ERR_PTR(-ENOMEM);
-
-	ret = i915_gem_stolen_insert_node_in_range(i915, stolen, size,
-						   alignment, start, end);
-	if (ret)
-		goto err_free;
-
-	obj = i915_gem_object_alloc();
-	if (!obj) {
-		ret = -ENOMEM;
-		goto err_stolen;
-	}
-
-	ret = __i915_gem_object_create_stolen(mem, obj, stolen);
-	if (ret)
-		goto err_object_free;
-
-	i915_gem_object_set_cache_coherency(obj, I915_CACHE_NONE);
-	return obj;
-
-err_object_free:
-	i915_gem_object_free(obj);
-err_stolen:
-	i915_gem_stolen_remove_node(i915, stolen);
-err_free:
-	kfree(stolen);
-	return ERR_PTR(ret);
+	return i915_gem_object_create_region_in_place(i915->mm.stolen_region, size, 0,
+						      I915_BO_ALLOC_CONTIGUOUS,
+						      start, end);
 }
 
 u64 i915_gem_object_stolen_offset(const struct drm_i915_gem_object *obj)
 {
+	struct ttm_buffer_object *ttm_obj;
 	if (!obj || !i915_gem_object_is_stolen(obj))
 		return 0;
 
-	return obj->stolen->start;
+	ttm_obj = i915_gem_to_ttm((struct drm_i915_gem_object *)obj);
+
+	return ttm_obj->resource->start;
 }
 
 
 bool i915_gem_object_is_stolen(const struct drm_i915_gem_object *obj)
 {
-	return obj->ops == &i915_gem_object_stolen_ops;
+	return obj->mm.region->type == INTEL_MEMORY_STOLEN_SYSTEM ||
+	       obj->mm.region->type == INTEL_MEMORY_STOLEN_LOCAL;
 }
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.h b/drivers/gpu/drm/i915/gem/i915_gem_stolen.h
index 494e90f130f4..921e51a5bbc4 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.h
@@ -12,15 +12,6 @@ struct drm_i915_private;
 struct drm_mm_node;
 struct drm_i915_gem_object;
 
-int i915_gem_stolen_insert_node(struct drm_i915_private *dev_priv,
-				struct drm_mm_node *node, u64 size,
-				unsigned alignment);
-int i915_gem_stolen_insert_node_in_range(struct drm_i915_private *dev_priv,
-					 struct drm_mm_node *node, u64 size,
-					 unsigned alignment, u64 start,
-					 u64 end);
-void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv,
-				 struct drm_mm_node *node);
 struct intel_memory_region *
 i915_gem_stolen_smem_setup(struct drm_i915_private *i915, u16 type,
 			   u16 instance);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 35d1bde19267..b26bde6a4bb9 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -1232,13 +1232,13 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
  *
  * Return: 0 on success, negative error code on failure.
  */
-static int i915_gem_ttm_object_init_in_place(struct intel_memory_region *mem,
-					     struct drm_i915_gem_object *obj,
-					     resource_size_t size,
-					     resource_size_t page_size,
-					     unsigned int flags,
-					     u64 start,
-					     u64 end)
+int i915_gem_ttm_object_init_in_place(struct intel_memory_region *mem,
+				      struct drm_i915_gem_object *obj,
+				      resource_size_t size,
+				      resource_size_t page_size,
+				      unsigned int flags,
+				      u64 start,
+				      u64 end)
 {
 	struct ttm_place place;
 	struct ttm_placement placement = {
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.h b/drivers/gpu/drm/i915/gem/i915_gem_ttm.h
index 9d698ad00853..f8ff52d81072 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.h
@@ -48,6 +48,13 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
 			       resource_size_t size,
 			       resource_size_t page_size,
 			       unsigned int flags);
+int i915_gem_ttm_object_init_in_place(struct intel_memory_region *mem,
+				      struct drm_i915_gem_object *obj,
+				      resource_size_t size,
+				      resource_size_t page_size,
+				      unsigned int flags,
+				      u64 start,
+				      u64 end);
 
 /* Internal I915 TTM declarations and definitions below. */
 
-- 
2.25.1

[RFC PATCH 5/7] drm/ttm: add range busy check for range manager

[Robert Beckett]

RFC: do we want this to become a generic interface in
ttm_resource_manager_func?

RFC: would we prefer a different interface? e.g.
for_each_resource_in_range or for_each_bo_in_range

Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
---
 drivers/gpu/drm/ttm/ttm_range_manager.c | 21 +++++++++++++++++++++
 include/drm/ttm/ttm_range_manager.h     |  3 +++
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c b/drivers/gpu/drm/ttm/ttm_range_manager.c
index 8cd4f3fb9f79..5662627bb933 100644
--- a/drivers/gpu/drm/ttm/ttm_range_manager.c
+++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
@@ -206,3 +206,24 @@ int ttm_range_man_fini_nocheck(struct ttm_device *bdev,
 	return 0;
 }
 EXPORT_SYMBOL(ttm_range_man_fini_nocheck);
+
+/**
+ * ttm_range_man_range_busy - Check whether anything is allocated with a range
+ *
+ * @man: memory manager to check
+ * @fpfn: first page number to check
+ * @lpfn: last page number to check
+ *
+ * Return: true if anything allocated within the range, false otherwise.
+ */
+bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
+			      unsigned fpfn, unsigned lpfn)
+{
+	struct ttm_range_manager *rman = to_range_manager(man);
+	struct drm_mm *mm = &rman->mm;
+
+	if (__drm_mm_interval_first(mm, PFN_PHYS(fpfn), PFN_PHYS(lpfn + 1) - 1))
+		return true;
+	return false;
+}
+EXPORT_SYMBOL(ttm_range_man_range_busy);
diff --git a/include/drm/ttm/ttm_range_manager.h b/include/drm/ttm/ttm_range_manager.h
index 7963b957e9ef..86794a3f9101 100644
--- a/include/drm/ttm/ttm_range_manager.h
+++ b/include/drm/ttm/ttm_range_manager.h
@@ -53,4 +53,7 @@ static __always_inline int ttm_range_man_fini(struct ttm_device *bdev,
 	BUILD_BUG_ON(__builtin_constant_p(type) && type >= TTM_NUM_MEM_TYPES);
 	return ttm_range_man_fini_nocheck(bdev, type);
 }
+
+bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
+			      unsigned fpfn, unsigned lpfn);
 #endif
-- 
2.25.1

Re: [RFC PATCH 5/7] drm/ttm: add range busy check for range manager

[Christian König]

Am 15.03.22 um 19:04 schrieb Robert Beckett:
> RFC: do we want this to become a generic interface in
> ttm_resource_manager_func?
>
> RFC: would we prefer a different interface? e.g.
> for_each_resource_in_range or for_each_bo_in_range

Well completely NAK to that. Why do you need that?

The long term goal is to completely remove the range checks from TTM 
instead.

Regards,
Christian.

>
> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
> ---
>   drivers/gpu/drm/ttm/ttm_range_manager.c | 21 +++++++++++++++++++++
>   include/drm/ttm/ttm_range_manager.h     |  3 +++
>   2 files changed, 24 insertions(+)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c b/drivers/gpu/drm/ttm/ttm_range_manager.c
> index 8cd4f3fb9f79..5662627bb933 100644
> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
> @@ -206,3 +206,24 @@ int ttm_range_man_fini_nocheck(struct ttm_device *bdev,
>   	return 0;
>   }
>   EXPORT_SYMBOL(ttm_range_man_fini_nocheck);
> +
> +/**
> + * ttm_range_man_range_busy - Check whether anything is allocated with a range
> + *
> + * @man: memory manager to check
> + * @fpfn: first page number to check
> + * @lpfn: last page number to check
> + *
> + * Return: true if anything allocated within the range, false otherwise.
> + */
> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
> +			      unsigned fpfn, unsigned lpfn)
> +{
> +	struct ttm_range_manager *rman = to_range_manager(man);
> +	struct drm_mm *mm = &rman->mm;
> +
> +	if (__drm_mm_interval_first(mm, PFN_PHYS(fpfn), PFN_PHYS(lpfn + 1) - 1))
> +		return true;
> +	return false;
> +}
> +EXPORT_SYMBOL(ttm_range_man_range_busy);
> diff --git a/include/drm/ttm/ttm_range_manager.h b/include/drm/ttm/ttm_range_manager.h
> index 7963b957e9ef..86794a3f9101 100644
> --- a/include/drm/ttm/ttm_range_manager.h
> +++ b/include/drm/ttm/ttm_range_manager.h
> @@ -53,4 +53,7 @@ static __always_inline int ttm_range_man_fini(struct ttm_device *bdev,
>   	BUILD_BUG_ON(__builtin_constant_p(type) && type >= TTM_NUM_MEM_TYPES);
>   	return ttm_range_man_fini_nocheck(bdev, type);
>   }
> +
> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
> +			      unsigned fpfn, unsigned lpfn);
>   #endif

Re: [RFC PATCH 5/7] drm/ttm: add range busy check for range manager

[Robert Beckett]

On 16/03/2022 09:54, Christian König wrote:
> Am 15.03.22 um 19:04 schrieb Robert Beckett:
>> RFC: do we want this to become a generic interface in
>> ttm_resource_manager_func?
>>
>> RFC: would we prefer a different interface? e.g.
>> for_each_resource_in_range or for_each_bo_in_range
> 
> Well completely NAK to that. Why do you need that?
> 
> The long term goal is to completely remove the range checks from TTM 
> instead.

ah, I did not know that.
I wanted it just to enable parity with a selftest that checks whether a 
range is allocated before initializing a given range with test data 
behind the allocator's back. It needs to check the range so that it 
doesn't destroy in use data.

I suppose we could add another drm_mm range tracker just for testing and 
shadow track each allocation in the range, but that seemed like a lot of 
extra infrastructure for no general runtime use.

would you mind explaining the rationale for removing range checks? It 
seems to me like a natural fit for a memory manager

> 
> Regards,
> Christian.
> 
>>
>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>> ---
>>   drivers/gpu/drm/ttm/ttm_range_manager.c | 21 +++++++++++++++++++++
>>   include/drm/ttm/ttm_range_manager.h     |  3 +++
>>   2 files changed, 24 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c 
>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>> index 8cd4f3fb9f79..5662627bb933 100644
>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>> @@ -206,3 +206,24 @@ int ttm_range_man_fini_nocheck(struct ttm_device 
>> *bdev,
>>       return 0;
>>   }
>>   EXPORT_SYMBOL(ttm_range_man_fini_nocheck);
>> +
>> +/**
>> + * ttm_range_man_range_busy - Check whether anything is allocated 
>> with a range
>> + *
>> + * @man: memory manager to check
>> + * @fpfn: first page number to check
>> + * @lpfn: last page number to check
>> + *
>> + * Return: true if anything allocated within the range, false otherwise.
>> + */
>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
>> +                  unsigned fpfn, unsigned lpfn)
>> +{
>> +    struct ttm_range_manager *rman = to_range_manager(man);
>> +    struct drm_mm *mm = &rman->mm;
>> +
>> +    if (__drm_mm_interval_first(mm, PFN_PHYS(fpfn), PFN_PHYS(lpfn + 
>> 1) - 1))
>> +        return true;
>> +    return false;
>> +}
>> +EXPORT_SYMBOL(ttm_range_man_range_busy);
>> diff --git a/include/drm/ttm/ttm_range_manager.h 
>> b/include/drm/ttm/ttm_range_manager.h
>> index 7963b957e9ef..86794a3f9101 100644
>> --- a/include/drm/ttm/ttm_range_manager.h
>> +++ b/include/drm/ttm/ttm_range_manager.h
>> @@ -53,4 +53,7 @@ static __always_inline int ttm_range_man_fini(struct 
>> ttm_device *bdev,
>>       BUILD_BUG_ON(__builtin_constant_p(type) && type >= 
>> TTM_NUM_MEM_TYPES);
>>       return ttm_range_man_fini_nocheck(bdev, type);
>>   }
>> +
>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
>> +                  unsigned fpfn, unsigned lpfn);
>>   #endif
> 

Re: [RFC PATCH 5/7] drm/ttm: add range busy check for range manager

[Christian König]

Am 16.03.22 um 14:19 schrieb Robert Beckett:
>
>
> On 16/03/2022 09:54, Christian König wrote:
>> Am 15.03.22 um 19:04 schrieb Robert Beckett:
>>> RFC: do we want this to become a generic interface in
>>> ttm_resource_manager_func?
>>>
>>> RFC: would we prefer a different interface? e.g.
>>> for_each_resource_in_range or for_each_bo_in_range
>>
>> Well completely NAK to that. Why do you need that?
>>
>> The long term goal is to completely remove the range checks from TTM 
>> instead.
>
> ah, I did not know that.
> I wanted it just to enable parity with a selftest that checks whether 
> a range is allocated before initializing a given range with test data 
> behind the allocator's back. It needs to check the range so that it 
> doesn't destroy in use data.

Mhm, of hand that doesn't sounds like a valid test case. Do you have the 
code at hand?

>
> I suppose we could add another drm_mm range tracker just for testing 
> and shadow track each allocation in the range, but that seemed like a 
> lot of extra infrastructure for no general runtime use.

I have no idea what you mean with that.

>
> would you mind explaining the rationale for removing range checks? It 
> seems to me like a natural fit for a memory manager

TTM manages buffer objects and resources, not address space. The 
lpfn/fpfn parameter for the resource allocators are actually used as 
just two independent parameters and not define any range. We just keep 
the names for historical reasons.

The only places we still use and compare them as ranges are 
ttm_resource_compat() and ttm_bo_eviction_valuable() and I already have 
patches to clean up those and move them into the backend resource handling.

Regards,
Christian.

>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>>> ---
>>>   drivers/gpu/drm/ttm/ttm_range_manager.c | 21 +++++++++++++++++++++
>>>   include/drm/ttm/ttm_range_manager.h     |  3 +++
>>>   2 files changed, 24 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c 
>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>> index 8cd4f3fb9f79..5662627bb933 100644
>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>> @@ -206,3 +206,24 @@ int ttm_range_man_fini_nocheck(struct 
>>> ttm_device *bdev,
>>>       return 0;
>>>   }
>>>   EXPORT_SYMBOL(ttm_range_man_fini_nocheck);
>>> +
>>> +/**
>>> + * ttm_range_man_range_busy - Check whether anything is allocated 
>>> with a range
>>> + *
>>> + * @man: memory manager to check
>>> + * @fpfn: first page number to check
>>> + * @lpfn: last page number to check
>>> + *
>>> + * Return: true if anything allocated within the range, false 
>>> otherwise.
>>> + */
>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
>>> +                  unsigned fpfn, unsigned lpfn)
>>> +{
>>> +    struct ttm_range_manager *rman = to_range_manager(man);
>>> +    struct drm_mm *mm = &rman->mm;
>>> +
>>> +    if (__drm_mm_interval_first(mm, PFN_PHYS(fpfn), PFN_PHYS(lpfn + 
>>> 1) - 1))
>>> +        return true;
>>> +    return false;
>>> +}
>>> +EXPORT_SYMBOL(ttm_range_man_range_busy);
>>> diff --git a/include/drm/ttm/ttm_range_manager.h 
>>> b/include/drm/ttm/ttm_range_manager.h
>>> index 7963b957e9ef..86794a3f9101 100644
>>> --- a/include/drm/ttm/ttm_range_manager.h
>>> +++ b/include/drm/ttm/ttm_range_manager.h
>>> @@ -53,4 +53,7 @@ static __always_inline int 
>>> ttm_range_man_fini(struct ttm_device *bdev,
>>>       BUILD_BUG_ON(__builtin_constant_p(type) && type >= 
>>> TTM_NUM_MEM_TYPES);
>>>       return ttm_range_man_fini_nocheck(bdev, type);
>>>   }
>>> +
>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
>>> +                  unsigned fpfn, unsigned lpfn);
>>>   #endif
>>

Re: [RFC PATCH 5/7] drm/ttm: add range busy check for range manager

[Robert Beckett]

On 16/03/2022 13:43, Christian König wrote:
> Am 16.03.22 um 14:19 schrieb Robert Beckett:
>>
>>
>> On 16/03/2022 09:54, Christian König wrote:
>>> Am 15.03.22 um 19:04 schrieb Robert Beckett:
>>>> RFC: do we want this to become a generic interface in
>>>> ttm_resource_manager_func?
>>>>
>>>> RFC: would we prefer a different interface? e.g.
>>>> for_each_resource_in_range or for_each_bo_in_range
>>>
>>> Well completely NAK to that. Why do you need that?
>>>
>>> The long term goal is to completely remove the range checks from TTM 
>>> instead.
>>
>> ah, I did not know that.
>> I wanted it just to enable parity with a selftest that checks whether 
>> a range is allocated before initializing a given range with test data 
>> behind the allocator's back. It needs to check the range so that it 
>> doesn't destroy in use data.
> 
> Mhm, of hand that doesn't sounds like a valid test case. Do you have the 
> code at hand?

https://patchwork.freedesktop.org/patch/478347/?series=101396&rev=1
this is where I replace an existing range check via drm_mm with the 
range check I added in this patch.

> 
>>
>> I suppose we could add another drm_mm range tracker just for testing 
>> and shadow track each allocation in the range, but that seemed like a 
>> lot of extra infrastructure for no general runtime use.
> 
> I have no idea what you mean with that.

I meant as a potential solution to tracking allocations without a range 
check, we would need to add something external. e.g. adding a shadow 
drm_mm range tracker, or a bitmask across the range, or stick objects in 
a list etc.

> 
>>
>> would you mind explaining the rationale for removing range checks? It 
>> seems to me like a natural fit for a memory manager
> 
> TTM manages buffer objects and resources, not address space. The 
> lpfn/fpfn parameter for the resource allocators are actually used as 
> just two independent parameters and not define any range. We just keep 
> the names for historical reasons.
> 
> The only places we still use and compare them as ranges are 
> ttm_resource_compat() and ttm_bo_eviction_valuable() and I already have 
> patches to clean up those and move them into the backend resource handling.

except the ttm_range_manager seems to still use them as a range specifier.

If the general design going forward is to not consider ranges, how would 
you recommend constructing buffers around pre-allocated regions e.g. 
uefi frame buffers who's range is dictated externally?

> 
> Regards,
> Christian.
> 
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>>>> ---
>>>>   drivers/gpu/drm/ttm/ttm_range_manager.c | 21 +++++++++++++++++++++
>>>>   include/drm/ttm/ttm_range_manager.h     |  3 +++
>>>>   2 files changed, 24 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c 
>>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>> index 8cd4f3fb9f79..5662627bb933 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>> @@ -206,3 +206,24 @@ int ttm_range_man_fini_nocheck(struct 
>>>> ttm_device *bdev,
>>>>       return 0;
>>>>   }
>>>>   EXPORT_SYMBOL(ttm_range_man_fini_nocheck);
>>>> +
>>>> +/**
>>>> + * ttm_range_man_range_busy - Check whether anything is allocated 
>>>> with a range
>>>> + *
>>>> + * @man: memory manager to check
>>>> + * @fpfn: first page number to check
>>>> + * @lpfn: last page number to check
>>>> + *
>>>> + * Return: true if anything allocated within the range, false 
>>>> otherwise.
>>>> + */
>>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
>>>> +                  unsigned fpfn, unsigned lpfn)
>>>> +{
>>>> +    struct ttm_range_manager *rman = to_range_manager(man);
>>>> +    struct drm_mm *mm = &rman->mm;
>>>> +
>>>> +    if (__drm_mm_interval_first(mm, PFN_PHYS(fpfn), PFN_PHYS(lpfn + 
>>>> 1) - 1))
>>>> +        return true;
>>>> +    return false;
>>>> +}
>>>> +EXPORT_SYMBOL(ttm_range_man_range_busy);
>>>> diff --git a/include/drm/ttm/ttm_range_manager.h 
>>>> b/include/drm/ttm/ttm_range_manager.h
>>>> index 7963b957e9ef..86794a3f9101 100644
>>>> --- a/include/drm/ttm/ttm_range_manager.h
>>>> +++ b/include/drm/ttm/ttm_range_manager.h
>>>> @@ -53,4 +53,7 @@ static __always_inline int 
>>>> ttm_range_man_fini(struct ttm_device *bdev,
>>>>       BUILD_BUG_ON(__builtin_constant_p(type) && type >= 
>>>> TTM_NUM_MEM_TYPES);
>>>>       return ttm_range_man_fini_nocheck(bdev, type);
>>>>   }
>>>> +
>>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
>>>> +                  unsigned fpfn, unsigned lpfn);
>>>>   #endif
>>>
> 

Re: [RFC PATCH 5/7] drm/ttm: add range busy check for range manager

[Christian König]

Am 16.03.22 um 15:26 schrieb Robert Beckett:
>
> [SNIP]
> this is where I replace an existing range check via drm_mm with the 
> range check I added in this patch.

Mhm, I still don't get the use case from the code, but I don't think it 
matters any more.

>>> I suppose we could add another drm_mm range tracker just for testing 
>>> and shadow track each allocation in the range, but that seemed like 
>>> a lot of extra infrastructure for no general runtime use.
>>
>> I have no idea what you mean with that.
>
> I meant as a potential solution to tracking allocations without a 
> range check, we would need to add something external. e.g. adding a 
> shadow drm_mm range tracker, or a bitmask across the range, or stick 
> objects in a list etc.

Ah! So you are trying to get access to the drm_mm inside the 
ttm_range_manager and not add some additional range check function! Now 
I got your use case.

>>> would you mind explaining the rationale for removing range checks? 
>>> It seems to me like a natural fit for a memory manager
>>
>> TTM manages buffer objects and resources, not address space. The 
>> lpfn/fpfn parameter for the resource allocators are actually used as 
>> just two independent parameters and not define any range. We just 
>> keep the names for historical reasons.
>>
>> The only places we still use and compare them as ranges are 
>> ttm_resource_compat() and ttm_bo_eviction_valuable() and I already 
>> have patches to clean up those and move them into the backend 
>> resource handling.
>
> except the ttm_range_manager seems to still use them as a range specifier.

Yeah, because the range manager is the backend which handles ranges 
using the drm_mm :)

> If the general design going forward is to not consider ranges, how 
> would you recommend constructing buffers around pre-allocated regions 
> e.g. uefi frame buffers who's range is dictated externally?

Call ttm_bo_mem_space() with the fpfn/lpfn filled in as required. See 
function amdgpu_bo_create_kernel_at() for an example.

Regards,
Christian.

>
>>
>> Regards,
>> Christian.
>>
>>>
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>
>>>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>>>>> ---
>>>>>   drivers/gpu/drm/ttm/ttm_range_manager.c | 21 +++++++++++++++++++++
>>>>>   include/drm/ttm/ttm_range_manager.h     |  3 +++
>>>>>   2 files changed, 24 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c 
>>>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>> index 8cd4f3fb9f79..5662627bb933 100644
>>>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>> @@ -206,3 +206,24 @@ int ttm_range_man_fini_nocheck(struct 
>>>>> ttm_device *bdev,
>>>>>       return 0;
>>>>>   }
>>>>>   EXPORT_SYMBOL(ttm_range_man_fini_nocheck);
>>>>> +
>>>>> +/**
>>>>> + * ttm_range_man_range_busy - Check whether anything is allocated 
>>>>> with a range
>>>>> + *
>>>>> + * @man: memory manager to check
>>>>> + * @fpfn: first page number to check
>>>>> + * @lpfn: last page number to check
>>>>> + *
>>>>> + * Return: true if anything allocated within the range, false 
>>>>> otherwise.
>>>>> + */
>>>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
>>>>> +                  unsigned fpfn, unsigned lpfn)
>>>>> +{
>>>>> +    struct ttm_range_manager *rman = to_range_manager(man);
>>>>> +    struct drm_mm *mm = &rman->mm;
>>>>> +
>>>>> +    if (__drm_mm_interval_first(mm, PFN_PHYS(fpfn), PFN_PHYS(lpfn 
>>>>> + 1) - 1))
>>>>> +        return true;
>>>>> +    return false;
>>>>> +}
>>>>> +EXPORT_SYMBOL(ttm_range_man_range_busy);
>>>>> diff --git a/include/drm/ttm/ttm_range_manager.h 
>>>>> b/include/drm/ttm/ttm_range_manager.h
>>>>> index 7963b957e9ef..86794a3f9101 100644
>>>>> --- a/include/drm/ttm/ttm_range_manager.h
>>>>> +++ b/include/drm/ttm/ttm_range_manager.h
>>>>> @@ -53,4 +53,7 @@ static __always_inline int 
>>>>> ttm_range_man_fini(struct ttm_device *bdev,
>>>>>       BUILD_BUG_ON(__builtin_constant_p(type) && type >= 
>>>>> TTM_NUM_MEM_TYPES);
>>>>>       return ttm_range_man_fini_nocheck(bdev, type);
>>>>>   }
>>>>> +
>>>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
>>>>> +                  unsigned fpfn, unsigned lpfn);
>>>>>   #endif
>>>>
>>

Re: [RFC PATCH 5/7] drm/ttm: add range busy check for range manager

[Robert Beckett]

On 16/03/2022 14:39, Christian König wrote:
> Am 16.03.22 um 15:26 schrieb Robert Beckett:
>>
>> [SNIP]
>> this is where I replace an existing range check via drm_mm with the 
>> range check I added in this patch.
> 
> Mhm, I still don't get the use case from the code, but I don't think it 
> matters any more.
> 
>>>> I suppose we could add another drm_mm range tracker just for testing 
>>>> and shadow track each allocation in the range, but that seemed like 
>>>> a lot of extra infrastructure for no general runtime use.
>>>
>>> I have no idea what you mean with that.
>>
>> I meant as a potential solution to tracking allocations without a 
>> range check, we would need to add something external. e.g. adding a 
>> shadow drm_mm range tracker, or a bitmask across the range, or stick 
>> objects in a list etc.
> 
> Ah! So you are trying to get access to the drm_mm inside the 
> ttm_range_manager and not add some additional range check function! Now 
> I got your use case.

well, specifically I was trying to avoid having to get access to the drm_mm.
I wanted to maintain an abstract interface at the resource manager 
level, hence the rfc to ask if we could add a range check to 
ttm_resource_manager_func.

I don't like the idea of code external to ttm having to poke in to the 
implementation details of the manager to get it's underlying drm_mm.

> 
>>>> would you mind explaining the rationale for removing range checks? 
>>>> It seems to me like a natural fit for a memory manager
>>>
>>> TTM manages buffer objects and resources, not address space. The 
>>> lpfn/fpfn parameter for the resource allocators are actually used as 
>>> just two independent parameters and not define any range. We just 
>>> keep the names for historical reasons.
>>>
>>> The only places we still use and compare them as ranges are 
>>> ttm_resource_compat() and ttm_bo_eviction_valuable() and I already 
>>> have patches to clean up those and move them into the backend 
>>> resource handling.
>>
>> except the ttm_range_manager seems to still use them as a range 
>> specifier.
> 
> Yeah, because the range manager is the backend which handles ranges 
> using the drm_mm :)
> 
>> If the general design going forward is to not consider ranges, how 
>> would you recommend constructing buffers around pre-allocated regions 
>> e.g. uefi frame buffers who's range is dictated externally?
> 
> Call ttm_bo_mem_space() with the fpfn/lpfn filled in as required. See 
> function amdgpu_bo_create_kernel_at() for an example.

ah, I see, thanks.

To allow similar code to before, which was conceptually just trying to 
see if a range was currently free, would you be okay with a new 
ttm_bo_mem_try_space, which does not do the force to evict, but instead 
returns -EBUSY?

If so, the test can try to alloc, and immediately free if successful 
which would imply it was free.

> 
> Regards,
> Christian.
> 
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/ttm/ttm_range_manager.c | 21 +++++++++++++++++++++
>>>>>>   include/drm/ttm/ttm_range_manager.h     |  3 +++
>>>>>>   2 files changed, 24 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c 
>>>>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>> index 8cd4f3fb9f79..5662627bb933 100644
>>>>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>> @@ -206,3 +206,24 @@ int ttm_range_man_fini_nocheck(struct 
>>>>>> ttm_device *bdev,
>>>>>>       return 0;
>>>>>>   }
>>>>>>   EXPORT_SYMBOL(ttm_range_man_fini_nocheck);
>>>>>> +
>>>>>> +/**
>>>>>> + * ttm_range_man_range_busy - Check whether anything is allocated 
>>>>>> with a range
>>>>>> + *
>>>>>> + * @man: memory manager to check
>>>>>> + * @fpfn: first page number to check
>>>>>> + * @lpfn: last page number to check
>>>>>> + *
>>>>>> + * Return: true if anything allocated within the range, false 
>>>>>> otherwise.
>>>>>> + */
>>>>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
>>>>>> +                  unsigned fpfn, unsigned lpfn)
>>>>>> +{
>>>>>> +    struct ttm_range_manager *rman = to_range_manager(man);
>>>>>> +    struct drm_mm *mm = &rman->mm;
>>>>>> +
>>>>>> +    if (__drm_mm_interval_first(mm, PFN_PHYS(fpfn), PFN_PHYS(lpfn 
>>>>>> + 1) - 1))
>>>>>> +        return true;
>>>>>> +    return false;
>>>>>> +}
>>>>>> +EXPORT_SYMBOL(ttm_range_man_range_busy);
>>>>>> diff --git a/include/drm/ttm/ttm_range_manager.h 
>>>>>> b/include/drm/ttm/ttm_range_manager.h
>>>>>> index 7963b957e9ef..86794a3f9101 100644
>>>>>> --- a/include/drm/ttm/ttm_range_manager.h
>>>>>> +++ b/include/drm/ttm/ttm_range_manager.h
>>>>>> @@ -53,4 +53,7 @@ static __always_inline int 
>>>>>> ttm_range_man_fini(struct ttm_device *bdev,
>>>>>>       BUILD_BUG_ON(__builtin_constant_p(type) && type >= 
>>>>>> TTM_NUM_MEM_TYPES);
>>>>>>       return ttm_range_man_fini_nocheck(bdev, type);
>>>>>>   }
>>>>>> +
>>>>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
>>>>>> +                  unsigned fpfn, unsigned lpfn);
>>>>>>   #endif
>>>>>
>>>
> 

Re: [RFC PATCH 5/7] drm/ttm: add range busy check for range manager

[Christian König]

Am 16.03.22 um 16:28 schrieb Robert Beckett:
>
>
> On 16/03/2022 14:39, Christian König wrote:
>> Am 16.03.22 um 15:26 schrieb Robert Beckett:
>>>
>>> [SNIP]
>>> this is where I replace an existing range check via drm_mm with the 
>>> range check I added in this patch.
>>
>> Mhm, I still don't get the use case from the code, but I don't think 
>> it matters any more.
>>
>>>>> I suppose we could add another drm_mm range tracker just for 
>>>>> testing and shadow track each allocation in the range, but that 
>>>>> seemed like a lot of extra infrastructure for no general runtime use.
>>>>
>>>> I have no idea what you mean with that.
>>>
>>> I meant as a potential solution to tracking allocations without a 
>>> range check, we would need to add something external. e.g. adding a 
>>> shadow drm_mm range tracker, or a bitmask across the range, or stick 
>>> objects in a list etc.
>>
>> Ah! So you are trying to get access to the drm_mm inside the 
>> ttm_range_manager and not add some additional range check function! 
>> Now I got your use case.
>
> well, specifically I was trying to avoid having to get access to the 
> drm_mm.
> I wanted to maintain an abstract interface at the resource manager 
> level, hence the rfc to ask if we could add a range check to 
> ttm_resource_manager_func.
>
> I don't like the idea of code external to ttm having to poke in to the 
> implementation details of the manager to get it's underlying drm_mm.

The purpose of the ttm_range_manager is to implement a base class which 
is then extended by the drivers with more explicit functionality.

I have it on my TODO list to properly export the ttm_range_manager 
functions and use them to simplify the amdgpu_gtt_mgr.c implementation.

So accessing the drm_mm for a test case sounds perfectly fine to me as 
long as you document what is happening. E.g. maybe add a wrapper 
function to get a pointer to the drm_mm.

>
>>
>>>>> would you mind explaining the rationale for removing range checks? 
>>>>> It seems to me like a natural fit for a memory manager
>>>>
>>>> TTM manages buffer objects and resources, not address space. The 
>>>> lpfn/fpfn parameter for the resource allocators are actually used 
>>>> as just two independent parameters and not define any range. We 
>>>> just keep the names for historical reasons.
>>>>
>>>> The only places we still use and compare them as ranges are 
>>>> ttm_resource_compat() and ttm_bo_eviction_valuable() and I already 
>>>> have patches to clean up those and move them into the backend 
>>>> resource handling.
>>>
>>> except the ttm_range_manager seems to still use them as a range 
>>> specifier.
>>
>> Yeah, because the range manager is the backend which handles ranges 
>> using the drm_mm :)
>>
>>> If the general design going forward is to not consider ranges, how 
>>> would you recommend constructing buffers around pre-allocated 
>>> regions e.g. uefi frame buffers who's range is dictated externally?
>>
>> Call ttm_bo_mem_space() with the fpfn/lpfn filled in as required. See 
>> function amdgpu_bo_create_kernel_at() for an example.
>
> ah, I see, thanks.
>
> To allow similar code to before, which was conceptually just trying to 
> see if a range was currently free, would you be okay with a new 
> ttm_bo_mem_try_space, which does not do the force to evict, but 
> instead returns -EBUSY?

You can already do that by setting the num_busy_placement to zero. That 
should prevent any eviction.

Regards,
Christian.


>
> If so, the test can try to alloc, and immediately free if successful 
> which would imply it was free.
>
>>
>> Regards,
>> Christian.
>>
>>>
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/ttm/ttm_range_manager.c | 21 
>>>>>>> +++++++++++++++++++++
>>>>>>>   include/drm/ttm/ttm_range_manager.h     |  3 +++
>>>>>>>   2 files changed, 24 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c 
>>>>>>> b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>> index 8cd4f3fb9f79..5662627bb933 100644
>>>>>>> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
>>>>>>> @@ -206,3 +206,24 @@ int ttm_range_man_fini_nocheck(struct 
>>>>>>> ttm_device *bdev,
>>>>>>>       return 0;
>>>>>>>   }
>>>>>>>   EXPORT_SYMBOL(ttm_range_man_fini_nocheck);
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * ttm_range_man_range_busy - Check whether anything is 
>>>>>>> allocated with a range
>>>>>>> + *
>>>>>>> + * @man: memory manager to check
>>>>>>> + * @fpfn: first page number to check
>>>>>>> + * @lpfn: last page number to check
>>>>>>> + *
>>>>>>> + * Return: true if anything allocated within the range, false 
>>>>>>> otherwise.
>>>>>>> + */
>>>>>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
>>>>>>> +                  unsigned fpfn, unsigned lpfn)
>>>>>>> +{
>>>>>>> +    struct ttm_range_manager *rman = to_range_manager(man);
>>>>>>> +    struct drm_mm *mm = &rman->mm;
>>>>>>> +
>>>>>>> +    if (__drm_mm_interval_first(mm, PFN_PHYS(fpfn), 
>>>>>>> PFN_PHYS(lpfn + 1) - 1))
>>>>>>> +        return true;
>>>>>>> +    return false;
>>>>>>> +}
>>>>>>> +EXPORT_SYMBOL(ttm_range_man_range_busy);
>>>>>>> diff --git a/include/drm/ttm/ttm_range_manager.h 
>>>>>>> b/include/drm/ttm/ttm_range_manager.h
>>>>>>> index 7963b957e9ef..86794a3f9101 100644
>>>>>>> --- a/include/drm/ttm/ttm_range_manager.h
>>>>>>> +++ b/include/drm/ttm/ttm_range_manager.h
>>>>>>> @@ -53,4 +53,7 @@ static __always_inline int 
>>>>>>> ttm_range_man_fini(struct ttm_device *bdev,
>>>>>>>       BUILD_BUG_ON(__builtin_constant_p(type) && type >= 
>>>>>>> TTM_NUM_MEM_TYPES);
>>>>>>>       return ttm_range_man_fini_nocheck(bdev, type);
>>>>>>>   }
>>>>>>> +
>>>>>>> +bool ttm_range_man_range_busy(struct ttm_resource_manager *man,
>>>>>>> +                  unsigned fpfn, unsigned lpfn);
>>>>>>>   #endif
>>>>>>
>>>>
>>

[RFC PATCH 6/7] drm/i915: add range busy check for ttm region

[Robert Beckett]

RFC: should this become a generic interface in intel_memory_region_ops?

RFC: would we prefer an different interface? e.g. for_each_obj_in_range

Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
---
 drivers/gpu/drm/i915/intel_region_ttm.c | 19 +++++++++++++++++++
 drivers/gpu/drm/i915/intel_region_ttm.h |  3 +++
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c b/drivers/gpu/drm/i915/intel_region_ttm.c
index bb564b830c96..2ccefa76348f 100644
--- a/drivers/gpu/drm/i915/intel_region_ttm.c
+++ b/drivers/gpu/drm/i915/intel_region_ttm.c
@@ -256,3 +256,22 @@ void intel_region_ttm_resource_free(struct intel_memory_region *mem,
 
 	man->func->free(man, res);
 }
+
+/**
+ * intel_region_ttm_range_busy - check whether range has any allocations
+ * @mem: The region to check
+ * @start: the start of the range to check
+ * @end: the end of the range to check
+ *
+ * Return: true if something is alloceted within the region, false otherwise.
+ */
+bool intel_region_ttm_range_busy(struct intel_memory_region *mem,
+				 u64 start, u64 end)
+{
+	struct ttm_resource_manager *man = mem->region_private;
+
+	/* currently only supported for range allocator */
+	GEM_BUG_ON(!mem->is_range_manager);
+
+	return ttm_range_man_range_busy(man, PFN_DOWN(start), PFN_UP(end));
+}
diff --git a/drivers/gpu/drm/i915/intel_region_ttm.h b/drivers/gpu/drm/i915/intel_region_ttm.h
index fdee5e7bd46c..670ba9b618f7 100644
--- a/drivers/gpu/drm/i915/intel_region_ttm.h
+++ b/drivers/gpu/drm/i915/intel_region_ttm.h
@@ -29,6 +29,9 @@ intel_region_ttm_resource_to_rsgt(struct intel_memory_region *mem,
 void intel_region_ttm_resource_free(struct intel_memory_region *mem,
 				    struct ttm_resource *res);
 
+bool intel_region_ttm_range_busy(struct intel_memory_region *mem,
+				 u64 start, u64 end);
+
 int intel_region_to_ttm_type(const struct intel_memory_region *mem);
 
 struct ttm_device_funcs *i915_ttm_driver(void);
-- 
2.25.1

[RFC PATCH 7/7] drm/i915: cleanup old stolen state

[Robert Beckett]

remove i915->mm.stolen
remove i915->mm.stolen_lock

they are no longer needed.

Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
---
 drivers/gpu/drm/i915/display/intel_fbc.c   |  4 ++--
 drivers/gpu/drm/i915/gem/i915_gem_stolen.c |  2 --
 drivers/gpu/drm/i915/gt/selftest_reset.c   | 16 +++++++++-------
 drivers/gpu/drm/i915/i915_drv.h            |  5 -----
 4 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index 9df64ecab70e..644bb599eee6 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -805,7 +805,7 @@ static int intel_fbc_alloc_cfb(struct intel_fbc *fbc,
 err_llb:
 	i915_gem_object_put(fetch_and_zero(&fbc->compressed_llb));
 err:
-	if (drm_mm_initialized(&i915->mm.stolen))
+	if (IS_ERR(obj) && (PTR_ERR(obj) == -ENOMEM || PTR_ERR(obj) == -ENXIO))
 		drm_info_once(&i915->drm, "not enough stolen space for compressed buffer (need %d more bytes), disabling. Hint: you may be able to increase stolen memory size in the BIOS to avoid this.\n", size);
 	return -ENOSPC;
 }
@@ -1708,7 +1708,7 @@ void intel_fbc_init(struct drm_i915_private *i915)
 {
 	enum intel_fbc_id fbc_id;
 
-	if (!drm_mm_initialized(&i915->mm.stolen))
+	if (!i915->mm.stolen_region)
 		mkwrite_device_info(i915)->display.fbc_mask = 0;
 
 	if (need_fbc_vtd_wa(i915))
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
index e58f9902ef47..930521a84607 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
@@ -347,8 +347,6 @@ static int i915_gem_init_stolen(struct intel_memory_region *mem)
 	resource_size_t reserved_base, stolen_top;
 	resource_size_t reserved_total, reserved_size;
 
-	mutex_init(&i915->mm.stolen_lock);
-
 	if (intel_vgpu_active(i915)) {
 		drm_notice(&i915->drm,
 			   "%s, disabling use of stolen memory\n",
diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c b/drivers/gpu/drm/i915/gt/selftest_reset.c
index 37c38bdd5f47..ad2ecc582be2 100644
--- a/drivers/gpu/drm/i915/gt/selftest_reset.c
+++ b/drivers/gpu/drm/i915/gt/selftest_reset.c
@@ -6,6 +6,7 @@
 #include <linux/crc32.h>
 
 #include "gem/i915_gem_stolen.h"
+#include "intel_region_ttm.h"
 
 #include "i915_memcpy.h"
 #include "i915_selftest.h"
@@ -83,6 +84,7 @@ __igt_reset_stolen(struct intel_gt *gt,
 		dma_addr_t dma = (dma_addr_t)dsm->start + (page << PAGE_SHIFT);
 		void __iomem *s;
 		void *in;
+		bool busy;
 
 		ggtt->vm.insert_page(&ggtt->vm, dma,
 				     ggtt->error_capture.start,
@@ -93,9 +95,9 @@ __igt_reset_stolen(struct intel_gt *gt,
 				      ggtt->error_capture.start,
 				      PAGE_SIZE);
 
-		if (!__drm_mm_interval_first(&gt->i915->mm.stolen,
-					     page << PAGE_SHIFT,
-					     ((page + 1) << PAGE_SHIFT) - 1))
+		busy = intel_region_ttm_range_busy(gt->i915->mm.stolen_region,
+						   PFN_PHYS(page), PFN_PHYS(page + 1) - 1);
+		if (!busy)
 			memset_io(s, STACK_MAGIC, PAGE_SIZE);
 
 		in = (void __force *)s;
@@ -124,6 +126,7 @@ __igt_reset_stolen(struct intel_gt *gt,
 		void __iomem *s;
 		void *in;
 		u32 x;
+		bool busy;
 
 		ggtt->vm.insert_page(&ggtt->vm, dma,
 				     ggtt->error_capture.start,
@@ -139,10 +142,9 @@ __igt_reset_stolen(struct intel_gt *gt,
 			in = tmp;
 		x = crc32_le(0, in, PAGE_SIZE);
 
-		if (x != crc[page] &&
-		    !__drm_mm_interval_first(&gt->i915->mm.stolen,
-					     page << PAGE_SHIFT,
-					     ((page + 1) << PAGE_SHIFT) - 1)) {
+		busy = intel_region_ttm_range_busy(gt->i915->mm.stolen_region,
+						   PFN_PHYS(page), PFN_PHYS(page + 1) - 1);
+		if (x != crc[page] && !busy) {
 			pr_debug("unused stolen page %pa modified by GPU reset\n",
 				 &page);
 			if (count++ == 0)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7d622d1afe93..1f9fa2d6d198 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -247,11 +247,6 @@ struct i915_gem_mm {
 	 * support stolen.
 	 */
 	struct intel_memory_region *stolen_region;
-	/** Memory allocator for GTT stolen memory */
-	struct drm_mm stolen;
-	/** Protects the usage of the GTT stolen memory allocator. This is
-	 * always the inner lock when overlapping with struct_mutex. */
-	struct mutex stolen_lock;
 
 	/* Protects bound_list/unbound_list and #drm_i915_gem_object.mm.link */
 	spinlock_t obj_lock;
-- 
2.25.1