[PATCH v3 1/6] drm/ttm: Add new callbacks to ttm res mgr

[PATCH v3 1/6] drm/ttm: Add new callbacks to ttm res mgr

[Arunpravin Paneer Selvam]

We are adding two new callbacks to ttm resource manager
function to handle intersection and compatibility of
placement and resources.

v2: move the amdgpu and ttm_range_manager changes to
    separate patches (Christian)
v3: rename "intersect" to "intersects" (Matthew)

Signed-off-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
---
 drivers/gpu/drm/ttm/ttm_resource.c | 59 ++++++++++++++++++++++++++++++
 include/drm/ttm/ttm_resource.h     | 39 ++++++++++++++++++++
 2 files changed, 98 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
index 20f9adcc3235..84a6fe9e976e 100644
--- a/drivers/gpu/drm/ttm/ttm_resource.c
+++ b/drivers/gpu/drm/ttm/ttm_resource.c
@@ -253,6 +253,65 @@ void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res)
 }
 EXPORT_SYMBOL(ttm_resource_free);
 
+/**
+ * ttm_resource_intersect - test for intersection
+ *
+ * @bdev: TTM device structure
+ * @res: The resource to test
+ * @place: The placement to test
+ * @size: How many bytes the new allocation needs.
+ *
+ * Test if @res intersects with @place and @size. Used for testing if evictions
+ * are valueable or not.
+ *
+ * Returns true if the res placement intersects with @place and @size.
+ */
+bool ttm_resource_intersect(struct ttm_device *bdev,
+			    struct ttm_resource *res,
+			    const struct ttm_place *place,
+			    size_t size)
+{
+	struct ttm_resource_manager *man;
+
+	if (!res)
+		return false;
+
+	man = ttm_manager_type(bdev, res->mem_type);
+	if (!place || !man->func->intersects)
+		return true;
+
+	return man->func->intersects(man, res, place, size);
+}
+
+/**
+ * ttm_resource_compatible - test for compatibility
+ *
+ * @bdev: TTM device structure
+ * @res: The resource to test
+ * @place: The placement to test
+ * @size: How many bytes the new allocation needs.
+ *
+ * Test if @res compatible with @place and @size.
+ *
+ * Returns true if the res placement compatible with @place and @size.
+ */
+bool ttm_resource_compatible(struct ttm_device *bdev,
+			     struct ttm_resource *res,
+			     const struct ttm_place *place,
+			     size_t size)
+{
+	struct ttm_resource_manager *man;
+
+	if (!res)
+		return false;
+
+	man = ttm_manager_type(bdev, res->mem_type);
+	if (!place || !man->func->compatible)
+		return true;
+
+	return man->func->compatible(man, res, place, size);
+}
+
 static bool ttm_resource_places_compat(struct ttm_resource *res,
 				       const struct ttm_place *places,
 				       unsigned num_placement)
diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h
index ca89a48c2460..93c0ac338be8 100644
--- a/include/drm/ttm/ttm_resource.h
+++ b/include/drm/ttm/ttm_resource.h
@@ -88,6 +88,37 @@ struct ttm_resource_manager_func {
 	void (*free)(struct ttm_resource_manager *man,
 		     struct ttm_resource *res);
 
+	/**
+	 * struct ttm_resource_manager_func member intersect
+	 *
+	 * @man: Pointer to a memory type manager.
+	 * @res: Pointer to a struct ttm_resource to be checked.
+	 * @place: Placement to check against.
+	 * @size: Size of the check.
+	 *
+	 * Test if @res intersects with @place + @size. Used to judge if
+	 * evictions are valueable or not.
+	 */
+	bool (*intersects)(struct ttm_resource_manager *man,
+			   struct ttm_resource *res,
+			   const struct ttm_place *place,
+			   size_t size);
+
+	/**
+	 * struct ttm_resource_manager_func member compatible
+	 *
+	 * @man: Pointer to a memory type manager.
+	 * @res: Pointer to a struct ttm_resource to be checked.
+	 * @place: Placement to check against.
+	 * @size: Size of the check.
+	 *
+	 * Test if @res compatible with @place + @size.
+	 */
+	bool (*compatible)(struct ttm_resource_manager *man,
+			   struct ttm_resource *res,
+			   const struct ttm_place *place,
+			   size_t size);
+
 	/**
 	 * struct ttm_resource_manager_func member debug
 	 *
@@ -329,6 +360,14 @@ int ttm_resource_alloc(struct ttm_buffer_object *bo,
 		       const struct ttm_place *place,
 		       struct ttm_resource **res);
 void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res);
+bool ttm_resource_intersect(struct ttm_device *bdev,
+			    struct ttm_resource *res,
+			    const struct ttm_place *place,
+			    size_t size);
+bool ttm_resource_compatible(struct ttm_device *bdev,
+			     struct ttm_resource *res,
+			     const struct ttm_place *place,
+			     size_t size);
 bool ttm_resource_compat(struct ttm_resource *res,
 			 struct ttm_placement *placement);
 void ttm_resource_set_bo(struct ttm_resource *res,
-- 
2.25.1

[PATCH v3 2/6] drm/ttm: Implement intersect/compatible functions

[Arunpravin Paneer Selvam]

Implemented a new intersect and compatible callback functions
to ttm range manager fetching start offset from drm mm range
allocator.

Signed-off-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
---
 drivers/gpu/drm/ttm/ttm_range_manager.c | 33 +++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c b/drivers/gpu/drm/ttm/ttm_range_manager.c
index d91666721dc6..19247aaeb5ce 100644
--- a/drivers/gpu/drm/ttm/ttm_range_manager.c
+++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
@@ -113,6 +113,37 @@ static void ttm_range_man_free(struct ttm_resource_manager *man,
 	kfree(node);
 }
 
+static bool ttm_range_man_intersect(struct ttm_resource_manager *man,
+				    struct ttm_resource *res,
+				    const struct ttm_place *place,
+				    size_t size)
+{
+	struct drm_mm_node *node = &to_ttm_range_mgr_node(res)->mm_nodes[0];
+	u32 num_pages = PFN_UP(size);
+
+	/* Don't evict BOs outside of the requested placement range */
+	if (place->fpfn >= (node->start + num_pages) ||
+	    (place->lpfn && place->lpfn <= node->start))
+		return false;
+
+	return true;
+}
+
+static bool ttm_range_man_compatible(struct ttm_resource_manager *man,
+				     struct ttm_resource *res,
+				     const struct ttm_place *place,
+				     size_t size)
+{
+	struct drm_mm_node *node = &to_ttm_range_mgr_node(res)->mm_nodes[0];
+	u32 num_pages = PFN_UP(size);
+
+	if (node->start < place->fpfn ||
+	    (place->lpfn && (node->start + num_pages) > place->lpfn))
+		return false;
+
+	return true;
+}
+
 static void ttm_range_man_debug(struct ttm_resource_manager *man,
 				struct drm_printer *printer)
 {
@@ -126,6 +157,8 @@ static void ttm_range_man_debug(struct ttm_resource_manager *man,
 static const struct ttm_resource_manager_func ttm_range_manager_func = {
 	.alloc = ttm_range_man_alloc,
 	.free = ttm_range_man_free,
+	.intersects = ttm_range_man_intersect,
+	.compatible = ttm_range_man_compatible,
 	.debug = ttm_range_man_debug
 };
 
-- 
2.25.1

[PATCH v3 3/6] drm/amdgpu: Implement intersect/compatible functions

[Arunpravin Paneer Selvam]

Implemented a new intersect and compatible callback function
fetching start offset from backend drm buddy allocator.

Signed-off-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c  | 38 ++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 63 ++++++++++++++++++++
 2 files changed, 101 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
index 8c6b2284cf56..c38752622e9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
@@ -204,6 +204,42 @@ void amdgpu_gtt_mgr_recover(struct amdgpu_gtt_mgr *mgr)
 	amdgpu_gart_invalidate_tlb(adev);
 }
 
+/**
+ * amdgpu_gtt_mgr_intersect - test for intersection
+ *
+ * @man: Our manager object
+ * @res: The resource to test
+ * @place: The place for the new allocation
+ * @size: The size of the new allocation
+ *
+ * Simplified intersection test, only interesting if we need GART or not.
+ */
+static bool amdgpu_gtt_mgr_intersect(struct ttm_resource_manager *man,
+				     struct ttm_resource *res,
+				     const struct ttm_place *place,
+				     size_t size)
+{
+	return !place->lpfn || amdgpu_gtt_mgr_has_gart_addr(res);
+}
+
+/**
+ * amdgpu_gtt_mgr_compatible - test for compatibility
+ *
+ * @man: Our manager object
+ * @res: The resource to test
+ * @place: The place for the new allocation
+ * @size: The size of the new allocation
+ *
+ * Simplified compatibility test.
+ */
+static bool amdgpu_gtt_mgr_compatible(struct ttm_resource_manager *man,
+				      struct ttm_resource *res,
+				      const struct ttm_place *place,
+				      size_t size)
+{
+	return !place->lpfn || amdgpu_gtt_mgr_has_gart_addr(res);
+}
+
 /**
  * amdgpu_gtt_mgr_debug - dump VRAM table
  *
@@ -225,6 +261,8 @@ static void amdgpu_gtt_mgr_debug(struct ttm_resource_manager *man,
 static const struct ttm_resource_manager_func amdgpu_gtt_mgr_func = {
 	.alloc = amdgpu_gtt_mgr_new,
 	.free = amdgpu_gtt_mgr_del,
+	.intersects = amdgpu_gtt_mgr_intersect,
+	.compatible = amdgpu_gtt_mgr_compatible,
 	.debug = amdgpu_gtt_mgr_debug
 };
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index 7a5e8a7b4a1b..0876d370dbb0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -720,6 +720,67 @@ uint64_t amdgpu_vram_mgr_vis_usage(struct amdgpu_vram_mgr *mgr)
 	return atomic64_read(&mgr->vis_usage);
 }
 
+/**
+ * amdgpu_vram_mgr_intersect - test each drm buddy block for intersection
+ *
+ * @man: TTM memory type manager
+ * @res: The resource to test
+ * @place: The place to test against
+ * @size: Size of the new allocation
+ *
+ * Test each drm buddy block for intersection for eviction decision.
+ */
+static bool amdgpu_vram_mgr_intersect(struct ttm_resource_manager *man,
+				      struct ttm_resource *res,
+				      const struct ttm_place *place,
+				      size_t size)
+{
+	struct amdgpu_vram_mgr_resource *mgr = to_amdgpu_vram_mgr_resource(res);
+	u32 start, num_pages = PFN_UP(size);
+	struct drm_buddy_block *block;
+
+	/* Check each drm buddy block individually */
+	list_for_each_entry(block, &mgr->blocks, link) {
+		start = amdgpu_vram_mgr_block_start(block) >> PAGE_SHIFT;
+		/* Don't evict BOs outside of the requested placement range */
+		if (place->fpfn >= (start + num_pages) ||
+		    (place->lpfn && place->lpfn <= start))
+			return false;
+	}
+
+	return true;
+}
+
+/**
+ * amdgpu_vram_mgr_compatible - test each drm buddy block for compatibility
+ *
+ * @man: TTM memory type manager
+ * @res: The resource to test
+ * @place: The place to test against
+ * @size: Size of the new allocation
+ *
+ * Test each drm buddy block for placement compatibility.
+ */
+static bool amdgpu_vram_mgr_compatible(struct ttm_resource_manager *man,
+				       struct ttm_resource *res,
+				       const struct ttm_place *place,
+				       size_t size)
+{
+	struct amdgpu_vram_mgr_resource *mgr = to_amdgpu_vram_mgr_resource(res);
+	u32 start, num_pages = PFN_UP(size);
+	struct drm_buddy_block *block;
+
+	/* Check each drm buddy block individually */
+	list_for_each_entry(block, &mgr->blocks, link) {
+		start = amdgpu_vram_mgr_block_start(block) >> PAGE_SHIFT;
+		if (start < place->fpfn ||
+		    (place->lpfn && (start + num_pages) > place->lpfn))
+			return false;
+	}
+
+	return true;
+}
+
 /**
  * amdgpu_vram_mgr_debug - dump VRAM table
  *
@@ -753,6 +814,8 @@ static void amdgpu_vram_mgr_debug(struct ttm_resource_manager *man,
 static const struct ttm_resource_manager_func amdgpu_vram_mgr_func = {
 	.alloc	= amdgpu_vram_mgr_new,
 	.free	= amdgpu_vram_mgr_del,
+	.intersects = amdgpu_vram_mgr_intersect,
+	.compatible = amdgpu_vram_mgr_compatible,
 	.debug	= amdgpu_vram_mgr_debug
 };
 
-- 
2.25.1

[PATCH v3 4/6] drm/i915: Implement intersect/compatible functions

[Arunpravin Paneer Selvam]

Implemented a new intersect and compatible callback function
fetching start offset from drm buddy allocator.

v2: move the bits that are specific to buddy_man (Matthew)

Signed-off-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c       | 39 +-----------
 drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 62 +++++++++++++++++++
 2 files changed, 64 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 70e2ed4e99df..54eead15d74b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -396,43 +396,8 @@ static bool i915_ttm_eviction_valuable(struct ttm_buffer_object *bo,
 	if (!i915_gem_object_evictable(obj))
 		return false;
 
-	switch (res->mem_type) {
-	case I915_PL_LMEM0: {
-		struct ttm_resource_manager *man =
-			ttm_manager_type(bo->bdev, res->mem_type);
-		struct i915_ttm_buddy_resource *bman_res =
-			to_ttm_buddy_resource(res);
-		struct drm_buddy *mm = bman_res->mm;
-		struct drm_buddy_block *block;
-
-		if (!place->fpfn && !place->lpfn)
-			return true;
-
-		GEM_BUG_ON(!place->lpfn);
-
-		/*
-		 * If we just want something mappable then we can quickly check
-		 * if the current victim resource is using any of the CPU
-		 * visible portion.
-		 */
-		if (!place->fpfn &&
-		    place->lpfn == i915_ttm_buddy_man_visible_size(man))
-			return bman_res->used_visible_size > 0;
-
-		/* Real range allocation */
-		list_for_each_entry(block, &bman_res->blocks, link) {
-			unsigned long fpfn =
-				drm_buddy_block_offset(block) >> PAGE_SHIFT;
-			unsigned long lpfn = fpfn +
-				(drm_buddy_block_size(mm, block) >> PAGE_SHIFT);
-
-			if (place->fpfn < lpfn && place->lpfn > fpfn)
-				return true;
-		}
-		return false;
-	} default:
-		break;
-	}
+	if (res->mem_type == I915_PL_LMEM0)
+		return ttm_bo_eviction_valuable(bo, place);
 
 	return true;
 }
diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
index a5109548abc0..9d2a31154d58 100644
--- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
+++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
@@ -178,6 +178,66 @@ static void i915_ttm_buddy_man_free(struct ttm_resource_manager *man,
 	kfree(bman_res);
 }
 
+static bool i915_ttm_buddy_man_intersect(struct ttm_resource_manager *man,
+					 struct ttm_resource *res,
+					 const struct ttm_place *place,
+					 size_t size)
+{
+	struct i915_ttm_buddy_resource *bman_res = to_ttm_buddy_resource(res);
+	u32 start, num_pages = PFN_UP(size);
+	struct drm_buddy_block *block;
+
+	if (!place->fpfn && !place->lpfn)
+		return true;
+
+	/*
+	 * If we just want something mappable then we can quickly check
+	 * if the current victim resource is using any of the CP
+	 * visible portion.
+	 */
+	if (!place->fpfn &&
+	    place->lpfn == i915_ttm_buddy_man_visible_size(man))
+		return bman_res->used_visible_size > 0;
+
+	/* Check each drm buddy block individually */
+	list_for_each_entry(block, &bman_res->blocks, link) {
+		start = drm_buddy_block_offset(block) >> PAGE_SHIFT;
+		/* Don't evict BOs outside of the requested placement range */
+		if (place->fpfn >= (start + num_pages) ||
+		    (place->lpfn && place->lpfn <= start))
+			return false;
+	}
+
+	return true;
+}
+
+static bool i915_ttm_buddy_man_compatible(struct ttm_resource_manager *man,
+					  struct ttm_resource *res,
+					  const struct ttm_place *place,
+					  size_t size)
+{
+	struct i915_ttm_buddy_resource *bman_res = to_ttm_buddy_resource(res);
+	u32 start, num_pages = PFN_UP(size);
+	struct drm_buddy_block *block;
+
+	if (!place->fpfn && !place->lpfn)
+		return true;
+
+	if (!place->fpfn &&
+	    place->lpfn == i915_ttm_buddy_man_visible_size(man))
+		return bman_res->used_visible_size == res->num_pages;
+
+	/* Check each drm buddy block individually */
+	list_for_each_entry(block, &bman_res->blocks, link) {
+		start = drm_buddy_block_offset(block) >> PAGE_SHIFT;
+		if (start < place->fpfn ||
+		    (place->lpfn && (start + num_pages) > place->lpfn))
+			return false;
+	}
+
+	return true;
+}
+
 static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man,
 				     struct drm_printer *printer)
 {
@@ -205,6 +265,8 @@ static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man,
 static const struct ttm_resource_manager_func i915_ttm_buddy_manager_func = {
 	.alloc = i915_ttm_buddy_man_alloc,
 	.free = i915_ttm_buddy_man_free,
+	.intersects = i915_ttm_buddy_man_intersect,
+	.compatible = i915_ttm_buddy_man_compatible,
 	.debug = i915_ttm_buddy_man_debug,
 };
 
-- 
2.25.1

[PATCH v3 5/6] drm/nouveau: Implement intersect/compatible functions

[Arunpravin Paneer Selvam]

Implemented a new intersect and compatible callback function
fetching the start offset from struct ttm_resource.

Signed-off-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
---
 drivers/gpu/drm/nouveau/nouveau_mem.c | 29 +++++++++++++++++++++++++++
 drivers/gpu/drm/nouveau/nouveau_mem.h |  6 ++++++
 drivers/gpu/drm/nouveau/nouveau_ttm.c | 24 ++++++++++++++++++++++
 3 files changed, 59 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_mem.c b/drivers/gpu/drm/nouveau/nouveau_mem.c
index 2e517cdc24c9..18f1c22fbc2c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_mem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_mem.c
@@ -187,3 +187,32 @@ nouveau_mem_new(struct nouveau_cli *cli, u8 kind, u8 comp,
 	*res = &mem->base;
 	return 0;
 }
+
+bool
+nouveau_mem_intersect(struct ttm_resource *res,
+		      const struct ttm_place *place,
+		      size_t size)
+{
+	u32 num_pages = PFN_UP(size);
+
+	/* Don't evict BOs outside of the requested placement range */
+	if (place->fpfn >= (res->start + num_pages) ||
+	    (place->lpfn && place->lpfn <= res->start))
+		return false;
+
+	return true;
+}
+
+bool
+nouveau_mem_compatible(struct ttm_resource *res,
+		       const struct ttm_place *place,
+		       size_t size)
+{
+	u32 num_pages = PFN_UP(size);
+
+	if (res->start < place->fpfn ||
+	    (place->lpfn && (res->start + num_pages) > place->lpfn))
+		return false;
+
+	return true;
+}
diff --git a/drivers/gpu/drm/nouveau/nouveau_mem.h b/drivers/gpu/drm/nouveau/nouveau_mem.h
index 325551eba5cd..4910e0e992ea 100644
--- a/drivers/gpu/drm/nouveau/nouveau_mem.h
+++ b/drivers/gpu/drm/nouveau/nouveau_mem.h
@@ -25,6 +25,12 @@ int nouveau_mem_new(struct nouveau_cli *, u8 kind, u8 comp,
 		    struct ttm_resource **);
 void nouveau_mem_del(struct ttm_resource_manager *man,
 		     struct ttm_resource *);
+bool nouveau_mem_intersect(struct ttm_resource *res,
+			   const struct ttm_place *place,
+			   size_t size);
+bool nouveau_mem_compatible(struct ttm_resource *res,
+			    const struct ttm_place *place,
+			    size_t size);
 int nouveau_mem_vram(struct ttm_resource *, bool contig, u8 page);
 int nouveau_mem_host(struct ttm_resource *, struct ttm_tt *);
 void nouveau_mem_fini(struct nouveau_mem *);
diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c b/drivers/gpu/drm/nouveau/nouveau_ttm.c
index 85f1f5a0fe5d..5dd6b4d25177 100644
--- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
@@ -42,6 +42,24 @@ nouveau_manager_del(struct ttm_resource_manager *man,
 	nouveau_mem_del(man, reg);
 }
 
+static bool
+nouveau_manager_intersect(struct ttm_resource_manager *man,
+			  struct ttm_resource *res,
+			  const struct ttm_place *place,
+			  size_t size)
+{
+	return nouveau_mem_intersect(res, place, size);
+}
+
+static bool
+nouveau_manager_compatible(struct ttm_resource_manager *man,
+			   struct ttm_resource *res,
+			   const struct ttm_place *place,
+			   size_t size)
+{
+	return nouveau_mem_compatible(res, place, size);
+}
+
 static int
 nouveau_vram_manager_new(struct ttm_resource_manager *man,
 			 struct ttm_buffer_object *bo,
@@ -73,6 +91,8 @@ nouveau_vram_manager_new(struct ttm_resource_manager *man,
 const struct ttm_resource_manager_func nouveau_vram_manager = {
 	.alloc = nouveau_vram_manager_new,
 	.free = nouveau_manager_del,
+	.intersects = nouveau_manager_intersect,
+	.compatible = nouveau_manager_compatible,
 };
 
 static int
@@ -97,6 +117,8 @@ nouveau_gart_manager_new(struct ttm_resource_manager *man,
 const struct ttm_resource_manager_func nouveau_gart_manager = {
 	.alloc = nouveau_gart_manager_new,
 	.free = nouveau_manager_del,
+	.intersects = nouveau_manager_intersect,
+	.compatible = nouveau_manager_compatible,
 };
 
 static int
@@ -130,6 +152,8 @@ nv04_gart_manager_new(struct ttm_resource_manager *man,
 const struct ttm_resource_manager_func nv04_gart_manager = {
 	.alloc = nv04_gart_manager_new,
 	.free = nouveau_manager_del,
+	.intersects = nouveau_manager_intersect,
+	.compatible = nouveau_manager_compatible,
 };
 
 static int
-- 
2.25.1

[PATCH v3 6/6] drm/ttm: Switch to using the new res callback

[Arunpravin Paneer Selvam]

Apply new intersect and compatible callback instead
of having a generic placement range verfications.

v2: Added a separate callback for compatiblilty
    checks (Christian)

Signed-off-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 45 +++++++------------------
 drivers/gpu/drm/ttm/ttm_bo.c            |  9 +++--
 drivers/gpu/drm/ttm/ttm_resource.c      |  5 +--
 3 files changed, 20 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 170935c294f5..7d25a10395c0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1328,11 +1328,12 @@ uint64_t amdgpu_ttm_tt_pte_flags(struct amdgpu_device *adev, struct ttm_tt *ttm,
 static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
 					    const struct ttm_place *place)
 {
-	unsigned long num_pages = bo->resource->num_pages;
 	struct dma_resv_iter resv_cursor;
-	struct amdgpu_res_cursor cursor;
 	struct dma_fence *f;
 
+	if (!amdgpu_bo_is_amdgpu_bo(bo))
+		return ttm_bo_eviction_valuable(bo, place);
+
 	/* Swapout? */
 	if (bo->resource->mem_type == TTM_PL_SYSTEM)
 		return true;
@@ -1351,40 +1352,20 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
 			return false;
 	}
 
-	switch (bo->resource->mem_type) {
-	case AMDGPU_PL_PREEMPT:
-		/* Preemptible BOs don't own system resources managed by the
-		 * driver (pages, VRAM, GART space). They point to resources
-		 * owned by someone else (e.g. pageable memory in user mode
-		 * or a DMABuf). They are used in a preemptible context so we
-		 * can guarantee no deadlocks and good QoS in case of MMU
-		 * notifiers or DMABuf move notifiers from the resource owner.
-		 */
+	/* Preemptible BOs don't own system resources managed by the
+	 * driver (pages, VRAM, GART space). They point to resources
+	 * owned by someone else (e.g. pageable memory in user mode
+	 * or a DMABuf). They are used in a preemptible context so we
+	 * can guarantee no deadlocks and good QoS in case of MMU
+	 * notifiers or DMABuf move notifiers from the resource owner.
+	 */
+	if (bo->resource->mem_type == AMDGPU_PL_PREEMPT)
 		return false;
-	case TTM_PL_TT:
-		if (amdgpu_bo_is_amdgpu_bo(bo) &&
-		    amdgpu_bo_encrypted(ttm_to_amdgpu_bo(bo)))
-			return false;
-		return true;
 
-	case TTM_PL_VRAM:
-		/* Check each drm MM node individually */
-		amdgpu_res_first(bo->resource, 0, (u64)num_pages << PAGE_SHIFT,
-				 &cursor);
-		while (cursor.remaining) {
-			if (place->fpfn < PFN_DOWN(cursor.start + cursor.size)
-			    && !(place->lpfn &&
-				 place->lpfn <= PFN_DOWN(cursor.start)))
-				return true;
-
-			amdgpu_res_next(&cursor, cursor.size);
-		}
+	if (bo->resource->mem_type == TTM_PL_TT &&
+	    amdgpu_bo_encrypted(ttm_to_amdgpu_bo(bo)))
 		return false;
 
-	default:
-		break;
-	}
-
 	return ttm_bo_eviction_valuable(bo, place);
 }
 
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index c1bd006a5525..03409409e43e 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -518,6 +518,9 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
 bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
 			      const struct ttm_place *place)
 {
+	struct ttm_resource *res = bo->resource;
+	struct ttm_device *bdev = bo->bdev;
+
 	dma_resv_assert_held(bo->base.resv);
 	if (bo->resource->mem_type == TTM_PL_SYSTEM)
 		return true;
@@ -525,11 +528,7 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
 	/* Don't evict this BO if it's outside of the
 	 * requested placement range
 	 */
-	if (place->fpfn >= (bo->resource->start + bo->resource->num_pages) ||
-	    (place->lpfn && place->lpfn <= bo->resource->start))
-		return false;
-
-	return true;
+	return ttm_resource_intersect(bdev, res, place, bo->base.size);
 }
 EXPORT_SYMBOL(ttm_bo_eviction_valuable);
 
diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
index 84a6fe9e976e..c745faf72b1a 100644
--- a/drivers/gpu/drm/ttm/ttm_resource.c
+++ b/drivers/gpu/drm/ttm/ttm_resource.c
@@ -316,6 +316,8 @@ static bool ttm_resource_places_compat(struct ttm_resource *res,
 				       const struct ttm_place *places,
 				       unsigned num_placement)
 {
+	struct ttm_buffer_object *bo = res->bo;
+	struct ttm_device *bdev = bo->bdev;
 	unsigned i;
 
 	if (res->placement & TTM_PL_FLAG_TEMPORARY)
@@ -324,8 +326,7 @@ static bool ttm_resource_places_compat(struct ttm_resource *res,
 	for (i = 0; i < num_placement; i++) {
 		const struct ttm_place *heap = &places[i];
 
-		if (res->start < heap->fpfn || (heap->lpfn &&
-		    (res->start + res->num_pages) > heap->lpfn))
+		if (!ttm_resource_compatible(bdev, res, heap, bo->base.size))
 			continue;
 
 		if ((res->mem_type == heap->mem_type) &&
-- 
2.25.1

Re: [PATCH v3 4/6] drm/i915: Implement intersect/compatible functions

[Matthew Auld]

On 28/07/2022 15:33, Arunpravin Paneer Selvam wrote:
> Implemented a new intersect and compatible callback function
> fetching start offset from drm buddy allocator.
> 
> v2: move the bits that are specific to buddy_man (Matthew)
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c       | 39 +-----------
>   drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 62 +++++++++++++++++++
>   2 files changed, 64 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 70e2ed4e99df..54eead15d74b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -396,43 +396,8 @@ static bool i915_ttm_eviction_valuable(struct ttm_buffer_object *bo,
>   	if (!i915_gem_object_evictable(obj))
>   		return false;
>   
> -	switch (res->mem_type) {
> -	case I915_PL_LMEM0: {
> -		struct ttm_resource_manager *man =
> -			ttm_manager_type(bo->bdev, res->mem_type);
> -		struct i915_ttm_buddy_resource *bman_res =
> -			to_ttm_buddy_resource(res);
> -		struct drm_buddy *mm = bman_res->mm;
> -		struct drm_buddy_block *block;
> -
> -		if (!place->fpfn && !place->lpfn)
> -			return true;
> -
> -		GEM_BUG_ON(!place->lpfn);
> -
> -		/*
> -		 * If we just want something mappable then we can quickly check
> -		 * if the current victim resource is using any of the CPU
> -		 * visible portion.
> -		 */
> -		if (!place->fpfn &&
> -		    place->lpfn == i915_ttm_buddy_man_visible_size(man))
> -			return bman_res->used_visible_size > 0;
> -
> -		/* Real range allocation */
> -		list_for_each_entry(block, &bman_res->blocks, link) {
> -			unsigned long fpfn =
> -				drm_buddy_block_offset(block) >> PAGE_SHIFT;
> -			unsigned long lpfn = fpfn +
> -				(drm_buddy_block_size(mm, block) >> PAGE_SHIFT);
> -
> -			if (place->fpfn < lpfn && place->lpfn > fpfn)
> -				return true;
> -		}
> -		return false;
> -	} default:
> -		break;
> -	}
> +	if (res->mem_type == I915_PL_LMEM0)
> +		return ttm_bo_eviction_valuable(bo, place);

We should be able to drop the mem_type == I915_PL_LMEM0 check here I 
think, and just unconditionally do:

return ttm_bo_eviction_valuable(bo, place);

>   
>   	return true;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
> index a5109548abc0..9d2a31154d58 100644
> --- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
> +++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
> @@ -178,6 +178,66 @@ static void i915_ttm_buddy_man_free(struct ttm_resource_manager *man,
>   	kfree(bman_res);
>   }
>   
> +static bool i915_ttm_buddy_man_intersect(struct ttm_resource_manager *man,

Nit: intersects

> +					 struct ttm_resource *res,
> +					 const struct ttm_place *place,
> +					 size_t size)
> +{
> +	struct i915_ttm_buddy_resource *bman_res = to_ttm_buddy_resource(res);
> +	u32 start, num_pages = PFN_UP(size);
> +	struct drm_buddy_block *block;
> +
> +	if (!place->fpfn && !place->lpfn)
> +		return true;
> +
> +	/*
> +	 * If we just want something mappable then we can quickly check
> +	 * if the current victim resource is using any of the CP
> +	 * visible portion.
> +	 */
> +	if (!place->fpfn &&
> +	    place->lpfn == i915_ttm_buddy_man_visible_size(man))
> +		return bman_res->used_visible_size > 0;
> +
> +	/* Check each drm buddy block individually */
> +	list_for_each_entry(block, &bman_res->blocks, link) {
> +		start = drm_buddy_block_offset(block) >> PAGE_SHIFT;
> +		/* Don't evict BOs outside of the requested placement range */
> +		if (place->fpfn >= (start + num_pages) ||
> +		    (place->lpfn && place->lpfn <= start))
> +			return false;
> +	}
> +
> +	return true;

We need to account for the block size somewhere. Also same bug in the 
amdgpu patch it seems. We also need to do this the other way around and 
keep checking until we find something that overlaps, for example if the 
first block doesn't intersect/overlap we will incorrectly return false 
here, even if one of the other blocks does intersect.

list_for_each_entry() {
     fpfn = drm_buddy_block_size(mm, block) >> PAGE_SHIFT;
     lpfn = fpfn + drm_buddy_block_size(mm, block) >> PAGE_SHIFT);

     if (place->fpfn < lpfn && place->lpfn > fpfn)
         return true;
}

return false;

> +}
> +
> +static bool i915_ttm_buddy_man_compatible(struct ttm_resource_manager *man,
> +					  struct ttm_resource *res,
> +					  const struct ttm_place *place,
> +					  size_t size)
> +{
> +	struct i915_ttm_buddy_resource *bman_res = to_ttm_buddy_resource(res);
> +	u32 start, num_pages = PFN_UP(size);
> +	struct drm_buddy_block *block;
> +
> +	if (!place->fpfn && !place->lpfn)
> +		return true;
> +
> +	if (!place->fpfn &&
> +	    place->lpfn == i915_ttm_buddy_man_visible_size(man))
> +		return bman_res->used_visible_size == res->num_pages;
> +
> +	/* Check each drm buddy block individually */
> +	list_for_each_entry(block, &bman_res->blocks, link) {
> +		start = drm_buddy_block_offset(block) >> PAGE_SHIFT;
> +		if (start < place->fpfn ||
> +		    (place->lpfn && (start + num_pages) > place->lpfn))

Same here. We need to consider the block size/range.

Otherwise I think looks good.

> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>   static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man,
>   				     struct drm_printer *printer)
>   {
> @@ -205,6 +265,8 @@ static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man,
>   static const struct ttm_resource_manager_func i915_ttm_buddy_manager_func = {
>   	.alloc = i915_ttm_buddy_man_alloc,
>   	.free = i915_ttm_buddy_man_free,
> +	.intersects = i915_ttm_buddy_man_intersect,
> +	.compatible = i915_ttm_buddy_man_compatible,
>   	.debug = i915_ttm_buddy_man_debug,
>   };
>   

Re: [PATCH v3 6/6] drm/ttm: Switch to using the new res callback

[Matthew Auld]

On 28/07/2022 15:33, Arunpravin Paneer Selvam wrote:
> Apply new intersect and compatible callback instead
> of having a generic placement range verfications.
> 
> v2: Added a separate callback for compatiblilty
>      checks (Christian)
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>

There is also some code at the bottom of i915_ttm_buddy_man_alloc() 
playing games with res->start, which I think can be safely deleted with 
this series (now that we have proper ->compatible() hook).

Also, is the plan to remove res->start completely, or does that still 
have a use?

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 45 +++++++------------------
>   drivers/gpu/drm/ttm/ttm_bo.c            |  9 +++--
>   drivers/gpu/drm/ttm/ttm_resource.c      |  5 +--
>   3 files changed, 20 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 170935c294f5..7d25a10395c0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1328,11 +1328,12 @@ uint64_t amdgpu_ttm_tt_pte_flags(struct amdgpu_device *adev, struct ttm_tt *ttm,
>   static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
>   					    const struct ttm_place *place)
>   {
> -	unsigned long num_pages = bo->resource->num_pages;
>   	struct dma_resv_iter resv_cursor;
> -	struct amdgpu_res_cursor cursor;
>   	struct dma_fence *f;
>   
> +	if (!amdgpu_bo_is_amdgpu_bo(bo))
> +		return ttm_bo_eviction_valuable(bo, place);
> +
>   	/* Swapout? */
>   	if (bo->resource->mem_type == TTM_PL_SYSTEM)
>   		return true;
> @@ -1351,40 +1352,20 @@ static bool amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
>   			return false;
>   	}
>   
> -	switch (bo->resource->mem_type) {
> -	case AMDGPU_PL_PREEMPT:
> -		/* Preemptible BOs don't own system resources managed by the
> -		 * driver (pages, VRAM, GART space). They point to resources
> -		 * owned by someone else (e.g. pageable memory in user mode
> -		 * or a DMABuf). They are used in a preemptible context so we
> -		 * can guarantee no deadlocks and good QoS in case of MMU
> -		 * notifiers or DMABuf move notifiers from the resource owner.
> -		 */
> +	/* Preemptible BOs don't own system resources managed by the
> +	 * driver (pages, VRAM, GART space). They point to resources
> +	 * owned by someone else (e.g. pageable memory in user mode
> +	 * or a DMABuf). They are used in a preemptible context so we
> +	 * can guarantee no deadlocks and good QoS in case of MMU
> +	 * notifiers or DMABuf move notifiers from the resource owner.
> +	 */
> +	if (bo->resource->mem_type == AMDGPU_PL_PREEMPT)
>   		return false;
> -	case TTM_PL_TT:
> -		if (amdgpu_bo_is_amdgpu_bo(bo) &&
> -		    amdgpu_bo_encrypted(ttm_to_amdgpu_bo(bo)))
> -			return false;
> -		return true;
>   
> -	case TTM_PL_VRAM:
> -		/* Check each drm MM node individually */
> -		amdgpu_res_first(bo->resource, 0, (u64)num_pages << PAGE_SHIFT,
> -				 &cursor);
> -		while (cursor.remaining) {
> -			if (place->fpfn < PFN_DOWN(cursor.start + cursor.size)
> -			    && !(place->lpfn &&
> -				 place->lpfn <= PFN_DOWN(cursor.start)))
> -				return true;
> -
> -			amdgpu_res_next(&cursor, cursor.size);
> -		}
> +	if (bo->resource->mem_type == TTM_PL_TT &&
> +	    amdgpu_bo_encrypted(ttm_to_amdgpu_bo(bo)))
>   		return false;
>   
> -	default:
> -		break;
> -	}
> -
>   	return ttm_bo_eviction_valuable(bo, place);
>   }
>   
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index c1bd006a5525..03409409e43e 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -518,6 +518,9 @@ static int ttm_bo_evict(struct ttm_buffer_object *bo,
>   bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
>   			      const struct ttm_place *place)
>   {
> +	struct ttm_resource *res = bo->resource;
> +	struct ttm_device *bdev = bo->bdev;
> +
>   	dma_resv_assert_held(bo->base.resv);
>   	if (bo->resource->mem_type == TTM_PL_SYSTEM)
>   		return true;
> @@ -525,11 +528,7 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
>   	/* Don't evict this BO if it's outside of the
>   	 * requested placement range
>   	 */
> -	if (place->fpfn >= (bo->resource->start + bo->resource->num_pages) ||
> -	    (place->lpfn && place->lpfn <= bo->resource->start))
> -		return false;
> -
> -	return true;
> +	return ttm_resource_intersect(bdev, res, place, bo->base.size);
>   }
>   EXPORT_SYMBOL(ttm_bo_eviction_valuable);
>   
> diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
> index 84a6fe9e976e..c745faf72b1a 100644
> --- a/drivers/gpu/drm/ttm/ttm_resource.c
> +++ b/drivers/gpu/drm/ttm/ttm_resource.c
> @@ -316,6 +316,8 @@ static bool ttm_resource_places_compat(struct ttm_resource *res,
>   				       const struct ttm_place *places,
>   				       unsigned num_placement)
>   {
> +	struct ttm_buffer_object *bo = res->bo;
> +	struct ttm_device *bdev = bo->bdev;
>   	unsigned i;
>   
>   	if (res->placement & TTM_PL_FLAG_TEMPORARY)
> @@ -324,8 +326,7 @@ static bool ttm_resource_places_compat(struct ttm_resource *res,
>   	for (i = 0; i < num_placement; i++) {
>   		const struct ttm_place *heap = &places[i];
>   
> -		if (res->start < heap->fpfn || (heap->lpfn &&
> -		    (res->start + res->num_pages) > heap->lpfn))
> +		if (!ttm_resource_compatible(bdev, res, heap, bo->base.size))
>   			continue;
>   
>   		if ((res->mem_type == heap->mem_type) &&

Re: [PATCH v3 4/6] drm/i915: Implement intersect/compatible functions

[Arunpravin Paneer Selvam]

On 7/28/2022 8:57 PM, Matthew Auld wrote:
> On 28/07/2022 15:33, Arunpravin Paneer Selvam wrote:
>> Implemented a new intersect and compatible callback function
>> fetching start offset from drm buddy allocator.
>>
>> v2: move the bits that are specific to buddy_man (Matthew)
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Arunpravin Paneer Selvam 
>> <Arunpravin.PaneerSelvam@amd.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_ttm.c       | 39 +-----------
>>   drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 62 +++++++++++++++++++
>>   2 files changed, 64 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c 
>> b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> index 70e2ed4e99df..54eead15d74b 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>> @@ -396,43 +396,8 @@ static bool i915_ttm_eviction_valuable(struct 
>> ttm_buffer_object *bo,
>>       if (!i915_gem_object_evictable(obj))
>>           return false;
>>   -    switch (res->mem_type) {
>> -    case I915_PL_LMEM0: {
>> -        struct ttm_resource_manager *man =
>> -            ttm_manager_type(bo->bdev, res->mem_type);
>> -        struct i915_ttm_buddy_resource *bman_res =
>> -            to_ttm_buddy_resource(res);
>> -        struct drm_buddy *mm = bman_res->mm;
>> -        struct drm_buddy_block *block;
>> -
>> -        if (!place->fpfn && !place->lpfn)
>> -            return true;
>> -
>> -        GEM_BUG_ON(!place->lpfn);
>> -
>> -        /*
>> -         * If we just want something mappable then we can quickly check
>> -         * if the current victim resource is using any of the CPU
>> -         * visible portion.
>> -         */
>> -        if (!place->fpfn &&
>> -            place->lpfn == i915_ttm_buddy_man_visible_size(man))
>> -            return bman_res->used_visible_size > 0;
>> -
>> -        /* Real range allocation */
>> -        list_for_each_entry(block, &bman_res->blocks, link) {
>> -            unsigned long fpfn =
>> -                drm_buddy_block_offset(block) >> PAGE_SHIFT;
>> -            unsigned long lpfn = fpfn +
>> -                (drm_buddy_block_size(mm, block) >> PAGE_SHIFT);
>> -
>> -            if (place->fpfn < lpfn && place->lpfn > fpfn)
>> -                return true;
>> -        }
>> -        return false;
>> -    } default:
>> -        break;
>> -    }
>> +    if (res->mem_type == I915_PL_LMEM0)
>> +        return ttm_bo_eviction_valuable(bo, place);
>
> We should be able to drop the mem_type == I915_PL_LMEM0 check here I 
> think, and just unconditionally do:
>
> return ttm_bo_eviction_valuable(bo, place);
okay
>
>>         return true;
>>   }
>> diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c 
>> b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
>> index a5109548abc0..9d2a31154d58 100644
>> --- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
>> +++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
>> @@ -178,6 +178,66 @@ static void i915_ttm_buddy_man_free(struct 
>> ttm_resource_manager *man,
>>       kfree(bman_res);
>>   }
>>   +static bool i915_ttm_buddy_man_intersect(struct 
>> ttm_resource_manager *man,
>
> Nit: intersects
ok
>
>> +                     struct ttm_resource *res,
>> +                     const struct ttm_place *place,
>> +                     size_t size)
>> +{
>> +    struct i915_ttm_buddy_resource *bman_res = 
>> to_ttm_buddy_resource(res);
>> +    u32 start, num_pages = PFN_UP(size);
>> +    struct drm_buddy_block *block;
>> +
>> +    if (!place->fpfn && !place->lpfn)
>> +        return true;
I place bug_on check here
GEM_BUG_ON(!place->lpfn);
>> +
>> +    /*
>> +     * If we just want something mappable then we can quickly check
>> +     * if the current victim resource is using any of the CP
>> +     * visible portion.
>> +     */
>> +    if (!place->fpfn &&
>> +        place->lpfn == i915_ttm_buddy_man_visible_size(man))
>> +        return bman_res->used_visible_size > 0;
>> +
>> +    /* Check each drm buddy block individually */
>> +    list_for_each_entry(block, &bman_res->blocks, link) {
>> +        start = drm_buddy_block_offset(block) >> PAGE_SHIFT;
>> +        /* Don't evict BOs outside of the requested placement range */
>> +        if (place->fpfn >= (start + num_pages) ||
>> +            (place->lpfn && place->lpfn <= start))
>> +            return false;
>> +    }
>> +
>> +    return true;
>
> We need to account for the block size somewhere. Also same bug in the 
> amdgpu patch it seems. We also need to do this the other way around 
> and keep checking until we find something that overlaps, for example 
> if the first block doesn't intersect/overlap we will incorrectly 
> return false here, even if one of the other blocks does intersect.
>
> list_for_each_entry() {
>     fpfn = drm_buddy_block_size(mm, block) >> PAGE_SHIFT;
>     lpfn = fpfn + drm_buddy_block_size(mm, block) >> PAGE_SHIFT);
>
>     if (place->fpfn < lpfn && place->lpfn > fpfn)
>         return true;
> }
>
> return false;
yes, here the final code looks like,
list_for_each_entry(block, &bman_res->blocks, link) {
                 unsigned long fpfn =
                         drm_buddy_block_offset(block) >> PAGE_SHIFT;
                 unsigned long lpfn = fpfn +
                         (drm_buddy_block_size(mm, block) >> PAGE_SHIFT);
                 /* Don't evict BOs outside of the requested placement 
range */
                 if (place->fpfn < lpfn && place->lpfn > fpfn)
                         return true;
         }

return false;

>
>> +}
>> +
>> +static bool i915_ttm_buddy_man_compatible(struct 
>> ttm_resource_manager *man,
>> +                      struct ttm_resource *res,
>> +                      const struct ttm_place *place,
>> +                      size_t size)
>> +{
>> +    struct i915_ttm_buddy_resource *bman_res = 
>> to_ttm_buddy_resource(res);
>> +    u32 start, num_pages = PFN_UP(size);
>> +    struct drm_buddy_block *block;
>> +
>> +    if (!place->fpfn && !place->lpfn)
>> +        return true;
I place bug_on check here
GEM_BUG_ON(!place->lpfn);
>> +
>> +    if (!place->fpfn &&
>> +        place->lpfn == i915_ttm_buddy_man_visible_size(man))
>> +        return bman_res->used_visible_size == res->num_pages;
>> +
>> +    /* Check each drm buddy block individually */
>> +    list_for_each_entry(block, &bman_res->blocks, link) {
>> +        start = drm_buddy_block_offset(block) >> PAGE_SHIFT;
>> +        if (start < place->fpfn ||
>> +            (place->lpfn && (start + num_pages) > place->lpfn))
>
> Same here. We need to consider the block size/range.
ahh somehow missed the block size, here the final code looks like,

/* Check each drm buddy block individually */
     list_for_each_entry(block, &bman_res->blocks, link) {
                 unsigned long fpfn =
                         drm_buddy_block_offset(block) >> PAGE_SHIFT;
                 unsigned long lpfn = fpfn +
                         (drm_buddy_block_size(mm, block) >> PAGE_SHIFT);
                 if (fpfn < place->fpfn || lpfn > place->lpfn)
                         return false;
         }
return true;
>
> Otherwise I think looks good.
>
>> +            return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>   static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man,
>>                        struct drm_printer *printer)
>>   {
>> @@ -205,6 +265,8 @@ static void i915_ttm_buddy_man_debug(struct 
>> ttm_resource_manager *man,
>>   static const struct ttm_resource_manager_func 
>> i915_ttm_buddy_manager_func = {
>>       .alloc = i915_ttm_buddy_man_alloc,
>>       .free = i915_ttm_buddy_man_free,
>> +    .intersects = i915_ttm_buddy_man_intersect,
>> +    .compatible = i915_ttm_buddy_man_compatible,
>>       .debug = i915_ttm_buddy_man_debug,
>>   };

Re: [PATCH v3 6/6] drm/ttm: Switch to using the new res callback

[Arunpravin Paneer Selvam]

On 7/28/2022 9:07 PM, Matthew Auld wrote:
> On 28/07/2022 15:33, Arunpravin Paneer Selvam wrote:
>> Apply new intersect and compatible callback instead
>> of having a generic placement range verfications.
>>
>> v2: Added a separate callback for compatiblilty
>>      checks (Christian)
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Arunpravin Paneer Selvam 
>> <Arunpravin.PaneerSelvam@amd.com>
>
> There is also some code at the bottom of i915_ttm_buddy_man_alloc() 
> playing games with res->start, which I think can be safely deleted 
> with this series (now that we have proper ->compatible() hook).
>
> Also, is the plan to remove res->start completely, or does that still 
> have a use?
yes we should remove res->start completely, I am working on it, planning 
to send in a separate series as amdgpu uses it in many places, and in 
some places we set res->start to AMDGPU_BO_INVALID_OFFSET,
I should find an alternative to indicate the invalid offset BO. Also, 
res->start used in drm/drm_gem_vram_helper.c at drm_gem_vram_pg_offset() 
function. I am removing all the dependencies, I will send the
patches in a separate series. I think i915 doesn't use res->start in its 
own driver code.
>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 45 +++++++------------------
>>   drivers/gpu/drm/ttm/ttm_bo.c            |  9 +++--
>>   drivers/gpu/drm/ttm/ttm_resource.c      |  5 +--
>>   3 files changed, 20 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 170935c294f5..7d25a10395c0 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -1328,11 +1328,12 @@ uint64_t amdgpu_ttm_tt_pte_flags(struct 
>> amdgpu_device *adev, struct ttm_tt *ttm,
>>   static bool amdgpu_ttm_bo_eviction_valuable(struct 
>> ttm_buffer_object *bo,
>>                           const struct ttm_place *place)
>>   {
>> -    unsigned long num_pages = bo->resource->num_pages;
>>       struct dma_resv_iter resv_cursor;
>> -    struct amdgpu_res_cursor cursor;
>>       struct dma_fence *f;
>>   +    if (!amdgpu_bo_is_amdgpu_bo(bo))
>> +        return ttm_bo_eviction_valuable(bo, place);
>> +
>>       /* Swapout? */
>>       if (bo->resource->mem_type == TTM_PL_SYSTEM)
>>           return true;
>> @@ -1351,40 +1352,20 @@ static bool 
>> amdgpu_ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
>>               return false;
>>       }
>>   -    switch (bo->resource->mem_type) {
>> -    case AMDGPU_PL_PREEMPT:
>> -        /* Preemptible BOs don't own system resources managed by the
>> -         * driver (pages, VRAM, GART space). They point to resources
>> -         * owned by someone else (e.g. pageable memory in user mode
>> -         * or a DMABuf). They are used in a preemptible context so we
>> -         * can guarantee no deadlocks and good QoS in case of MMU
>> -         * notifiers or DMABuf move notifiers from the resource owner.
>> -         */
>> +    /* Preemptible BOs don't own system resources managed by the
>> +     * driver (pages, VRAM, GART space). They point to resources
>> +     * owned by someone else (e.g. pageable memory in user mode
>> +     * or a DMABuf). They are used in a preemptible context so we
>> +     * can guarantee no deadlocks and good QoS in case of MMU
>> +     * notifiers or DMABuf move notifiers from the resource owner.
>> +     */
>> +    if (bo->resource->mem_type == AMDGPU_PL_PREEMPT)
>>           return false;
>> -    case TTM_PL_TT:
>> -        if (amdgpu_bo_is_amdgpu_bo(bo) &&
>> -            amdgpu_bo_encrypted(ttm_to_amdgpu_bo(bo)))
>> -            return false;
>> -        return true;
>>   -    case TTM_PL_VRAM:
>> -        /* Check each drm MM node individually */
>> -        amdgpu_res_first(bo->resource, 0, (u64)num_pages << PAGE_SHIFT,
>> -                 &cursor);
>> -        while (cursor.remaining) {
>> -            if (place->fpfn < PFN_DOWN(cursor.start + cursor.size)
>> -                && !(place->lpfn &&
>> -                 place->lpfn <= PFN_DOWN(cursor.start)))
>> -                return true;
>> -
>> -            amdgpu_res_next(&cursor, cursor.size);
>> -        }
>> +    if (bo->resource->mem_type == TTM_PL_TT &&
>> +        amdgpu_bo_encrypted(ttm_to_amdgpu_bo(bo)))
>>           return false;
>>   -    default:
>> -        break;
>> -    }
>> -
>>       return ttm_bo_eviction_valuable(bo, place);
>>   }
>>   diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
>> b/drivers/gpu/drm/ttm/ttm_bo.c
>> index c1bd006a5525..03409409e43e 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -518,6 +518,9 @@ static int ttm_bo_evict(struct ttm_buffer_object 
>> *bo,
>>   bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
>>                     const struct ttm_place *place)
>>   {
>> +    struct ttm_resource *res = bo->resource;
>> +    struct ttm_device *bdev = bo->bdev;
>> +
>>       dma_resv_assert_held(bo->base.resv);
>>       if (bo->resource->mem_type == TTM_PL_SYSTEM)
>>           return true;
>> @@ -525,11 +528,7 @@ bool ttm_bo_eviction_valuable(struct 
>> ttm_buffer_object *bo,
>>       /* Don't evict this BO if it's outside of the
>>        * requested placement range
>>        */
>> -    if (place->fpfn >= (bo->resource->start + 
>> bo->resource->num_pages) ||
>> -        (place->lpfn && place->lpfn <= bo->resource->start))
>> -        return false;
>> -
>> -    return true;
>> +    return ttm_resource_intersect(bdev, res, place, bo->base.size);
>>   }
>>   EXPORT_SYMBOL(ttm_bo_eviction_valuable);
>>   diff --git a/drivers/gpu/drm/ttm/ttm_resource.c 
>> b/drivers/gpu/drm/ttm/ttm_resource.c
>> index 84a6fe9e976e..c745faf72b1a 100644
>> --- a/drivers/gpu/drm/ttm/ttm_resource.c
>> +++ b/drivers/gpu/drm/ttm/ttm_resource.c
>> @@ -316,6 +316,8 @@ static bool ttm_resource_places_compat(struct 
>> ttm_resource *res,
>>                          const struct ttm_place *places,
>>                          unsigned num_placement)
>>   {
>> +    struct ttm_buffer_object *bo = res->bo;
>> +    struct ttm_device *bdev = bo->bdev;
>>       unsigned i;
>>         if (res->placement & TTM_PL_FLAG_TEMPORARY)
>> @@ -324,8 +326,7 @@ static bool ttm_resource_places_compat(struct 
>> ttm_resource *res,
>>       for (i = 0; i < num_placement; i++) {
>>           const struct ttm_place *heap = &places[i];
>>   -        if (res->start < heap->fpfn || (heap->lpfn &&
>> -            (res->start + res->num_pages) > heap->lpfn))
>> +        if (!ttm_resource_compatible(bdev, res, heap, bo->base.size))
>>               continue;
>>             if ((res->mem_type == heap->mem_type) &&