[PATCH v2 1/3] drm/amdgpu: Dirty cleared blocks on free

[PATCH v2 1/3] drm/amdgpu: Dirty cleared blocks on free

[Arunpravin Paneer Selvam]

Set the dirty bit when the memory resource is not cleared
during BO release.

v2(Christian):
  - Drop the cleared flag set to false.
  - Improve the amdgpu_vram_mgr_set_clear_state() function.

Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
Suggested-by: Christian König <christian.koenig@amd.com>
Cc: stable@vger.kernel.org
Fixes: a68c7eaa7a8f ("drm/amdgpu: Enable clear page functionality")
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c      | 1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h | 5 ++++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 9c5df35f05b7..86eb6d47dcc5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -409,7 +409,6 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo,
 		if (r) {
 			goto error;
 		} else if (wipe_fence) {
-			amdgpu_vram_mgr_set_cleared(bo->resource);
 			dma_fence_put(fence);
 			fence = wipe_fence;
 		}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
index b256cbc2bc27..2c88d5fd87da 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
@@ -66,7 +66,10 @@ to_amdgpu_vram_mgr_resource(struct ttm_resource *res)
 
 static inline void amdgpu_vram_mgr_set_cleared(struct ttm_resource *res)
 {
-	to_amdgpu_vram_mgr_resource(res)->flags |= DRM_BUDDY_CLEARED;
+	struct amdgpu_vram_mgr_resource *ares = to_amdgpu_vram_mgr_resource(res);
+
+	WARN_ON(ares->flags & DRM_BUDDY_CLEARED);
+	ares->flags |= DRM_BUDDY_CLEARED;
 }
 
 #endif
-- 
2.43.0

[PATCH v2 2/3] drm/amdgpu: Reset the clear flag in buddy during resume

[Arunpravin Paneer Selvam]

- Added a handler in DRM buddy manager to reset the cleared
  flag for the blocks in the freelist.

- This is necessary because, upon resuming, the VRAM becomes
  cluttered with BIOS data, yet the VRAM backend manager
  believes that everything has been cleared.

v2:
  - Add lock before accessing drm_buddy_clear_reset_blocks()(Matthew Auld)
  - Force merge the two dirty blocks.(Matthew Auld)
  - Add a new unit test case for this issue.(Matthew Auld)
  - Having this function being able to flip the state either way would be
    good. (Matthew Brost)

Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
Suggested-by: Christian König <christian.koenig@amd.com>
Cc: stable@vger.kernel.org
Fixes: a68c7eaa7a8f ("drm/amdgpu: Enable clear page functionality")
Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3812
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c   |  2 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h      |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 17 ++++
 drivers/gpu/drm/drm_buddy.c                  | 90 +++++++++++++++++---
 include/drm/drm_buddy.h                      |  2 +
 5 files changed, 99 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index a59f194e3360..b89e46f29b51 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -5193,6 +5193,8 @@ int amdgpu_device_resume(struct drm_device *dev, bool notify_clients)
 		dev->dev->power.disable_depth--;
 #endif
 	}
+
+	amdgpu_vram_mgr_clear_reset_blocks(adev);
 	adev->in_suspend = false;
 
 	if (amdgpu_acpi_smart_shift_update(dev, AMDGPU_SS_DEV_D0))
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
index 208b7d1d8a27..450e4bf093b7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
@@ -154,6 +154,7 @@ int amdgpu_vram_mgr_reserve_range(struct amdgpu_vram_mgr *mgr,
 				  uint64_t start, uint64_t size);
 int amdgpu_vram_mgr_query_page_status(struct amdgpu_vram_mgr *mgr,
 				      uint64_t start);
+void amdgpu_vram_mgr_clear_reset_blocks(struct amdgpu_device *adev);
 
 bool amdgpu_res_cpu_visible(struct amdgpu_device *adev,
 			    struct ttm_resource *res);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index abdc52b0895a..665656fbc948 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -782,6 +782,23 @@ uint64_t amdgpu_vram_mgr_vis_usage(struct amdgpu_vram_mgr *mgr)
 	return atomic64_read(&mgr->vis_usage);
 }
 
+/**
+ * amdgpu_vram_mgr_clear_reset_blocks - reset clear blocks
+ *
+ * @adev: amdgpu device pointer
+ *
+ * Reset the cleared drm buddy blocks.
+ */
+void amdgpu_vram_mgr_clear_reset_blocks(struct amdgpu_device *adev)
+{
+	struct amdgpu_vram_mgr *mgr = &adev->mman.vram_mgr;
+	struct drm_buddy *mm = &mgr->mm;
+
+	mutex_lock(&mgr->lock);
+	drm_buddy_reset_clear_state(mm, false);
+	mutex_unlock(&mgr->lock);
+}
+
 /**
  * amdgpu_vram_mgr_intersects - test each drm buddy block for intersection
  *
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index a1e652b7631d..436f7e4ee202 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -12,6 +12,9 @@
 
 #include <drm/drm_buddy.h>
 
+#define FORCE_MERGE	BIT(0)
+#define RESET_CLEAR	BIT(1)
+
 static struct kmem_cache *slab_blocks;
 
 static struct drm_buddy_block *drm_block_alloc(struct drm_buddy *mm,
@@ -60,6 +63,16 @@ static void list_insert_sorted(struct drm_buddy *mm,
 	__list_add(&block->link, node->link.prev, &node->link);
 }
 
+static bool is_force_merge_enabled(unsigned int flags)
+{
+	return flags & FORCE_MERGE;
+}
+
+static bool is_reset_clear_enabled(unsigned int flags)
+{
+	return flags & RESET_CLEAR;
+}
+
 static void clear_reset(struct drm_buddy_block *block)
 {
 	block->header &= ~DRM_BUDDY_HEADER_CLEAR;
@@ -122,7 +135,7 @@ __get_buddy(struct drm_buddy_block *block)
 
 static unsigned int __drm_buddy_free(struct drm_buddy *mm,
 				     struct drm_buddy_block *block,
-				     bool force_merge)
+				     unsigned int flags)
 {
 	struct drm_buddy_block *parent;
 	unsigned int order;
@@ -135,7 +148,7 @@ static unsigned int __drm_buddy_free(struct drm_buddy *mm,
 		if (!drm_buddy_block_is_free(buddy))
 			break;
 
-		if (!force_merge) {
+		if (!is_force_merge_enabled(flags)) {
 			/*
 			 * Check the block and its buddy clear state and exit
 			 * the loop if they both have the dissimilar state.
@@ -149,7 +162,9 @@ static unsigned int __drm_buddy_free(struct drm_buddy *mm,
 		}
 
 		list_del(&buddy->link);
-		if (force_merge && drm_buddy_block_is_clear(buddy))
+		if (is_force_merge_enabled(flags) &&
+		    !is_reset_clear_enabled(flags) &&
+		    drm_buddy_block_is_clear(buddy))
 			mm->clear_avail -= drm_buddy_block_size(mm, buddy);
 
 		drm_block_free(mm, block);
@@ -167,7 +182,8 @@ static unsigned int __drm_buddy_free(struct drm_buddy *mm,
 static int __force_merge(struct drm_buddy *mm,
 			 u64 start,
 			 u64 end,
-			 unsigned int min_order)
+			 unsigned int min_order,
+			 unsigned int flags)
 {
 	unsigned int order;
 	int i;
@@ -178,6 +194,8 @@ static int __force_merge(struct drm_buddy *mm,
 	if (min_order > mm->max_order)
 		return -EINVAL;
 
+	flags |= FORCE_MERGE;
+
 	for (i = min_order - 1; i >= 0; i--) {
 		struct drm_buddy_block *block, *prev;
 
@@ -198,7 +216,8 @@ static int __force_merge(struct drm_buddy *mm,
 			if (!drm_buddy_block_is_free(buddy))
 				continue;
 
-			WARN_ON(drm_buddy_block_is_clear(block) ==
+			WARN_ON(!is_reset_clear_enabled(flags) &&
+				drm_buddy_block_is_clear(block) ==
 				drm_buddy_block_is_clear(buddy));
 
 			/*
@@ -210,10 +229,11 @@ static int __force_merge(struct drm_buddy *mm,
 				prev = list_prev_entry(prev, link);
 
 			list_del(&block->link);
-			if (drm_buddy_block_is_clear(block))
+			if (!is_reset_clear_enabled(flags) &&
+			    drm_buddy_block_is_clear(block))
 				mm->clear_avail -= drm_buddy_block_size(mm, block);
 
-			order = __drm_buddy_free(mm, block, true);
+			order = __drm_buddy_free(mm, block, flags);
 			if (order >= min_order)
 				return 0;
 		}
@@ -336,7 +356,7 @@ void drm_buddy_fini(struct drm_buddy *mm)
 	for (i = 0; i < mm->n_roots; ++i) {
 		order = ilog2(size) - ilog2(mm->chunk_size);
 		start = drm_buddy_block_offset(mm->roots[i]);
-		__force_merge(mm, start, start + size, order);
+		__force_merge(mm, start, start + size, order, 0);
 
 		if (WARN_ON(!drm_buddy_block_is_free(mm->roots[i])))
 			kunit_fail_current_test("buddy_fini() root");
@@ -405,6 +425,50 @@ drm_get_buddy(struct drm_buddy_block *block)
 }
 EXPORT_SYMBOL(drm_get_buddy);
 
+/**
+ * drm_buddy_reset_clear_state - reset blocks clear state
+ *
+ * @mm: DRM buddy manager
+ * @is_clear: blocks clear state
+ *
+ * Reset the clear state based on @clear value for each block
+ * in the freelist.
+ */
+void drm_buddy_reset_clear_state(struct drm_buddy *mm, bool is_clear)
+{
+	u64 root_size, size, start;
+	unsigned int order;
+	int i;
+
+	for (i = 0; i <= mm->max_order; ++i) {
+		struct drm_buddy_block *block;
+
+		list_for_each_entry_reverse(block, &mm->free_list[i], link) {
+			if (is_clear != drm_buddy_block_is_clear(block)) {
+				if (is_clear) {
+					mark_cleared(block);
+					mm->clear_avail += drm_buddy_block_size(mm, block);
+				} else {
+					clear_reset(block);
+					mm->clear_avail -= drm_buddy_block_size(mm, block);
+				}
+			}
+		}
+	}
+
+	/* Force merge the two dirty or two cleared blocks */
+	size = mm->size;
+	for (i = 0; i < mm->n_roots; ++i) {
+		order = ilog2(size) - ilog2(mm->chunk_size);
+		start = drm_buddy_block_offset(mm->roots[i]);
+		__force_merge(mm, start, start + size, order, RESET_CLEAR);
+
+		root_size = mm->chunk_size << order;
+		size -= root_size;
+	}
+}
+EXPORT_SYMBOL(drm_buddy_reset_clear_state);
+
 /**
  * drm_buddy_free_block - free a block
  *
@@ -419,7 +483,7 @@ void drm_buddy_free_block(struct drm_buddy *mm,
 	if (drm_buddy_block_is_clear(block))
 		mm->clear_avail += drm_buddy_block_size(mm, block);
 
-	__drm_buddy_free(mm, block, false);
+	__drm_buddy_free(mm, block, 0);
 }
 EXPORT_SYMBOL(drm_buddy_free_block);
 
@@ -566,7 +630,7 @@ __alloc_range_bias(struct drm_buddy *mm,
 	if (buddy &&
 	    (drm_buddy_block_is_free(block) &&
 	     drm_buddy_block_is_free(buddy)))
-		__drm_buddy_free(mm, block, false);
+		__drm_buddy_free(mm, block, 0);
 	return ERR_PTR(err);
 }
 
@@ -684,7 +748,7 @@ alloc_from_freelist(struct drm_buddy *mm,
 
 err_undo:
 	if (tmp != order)
-		__drm_buddy_free(mm, block, false);
+		__drm_buddy_free(mm, block, 0);
 	return ERR_PTR(err);
 }
 
@@ -770,7 +834,7 @@ static int __alloc_range(struct drm_buddy *mm,
 	if (buddy &&
 	    (drm_buddy_block_is_free(block) &&
 	     drm_buddy_block_is_free(buddy)))
-		__drm_buddy_free(mm, block, false);
+		__drm_buddy_free(mm, block, 0);
 
 err_free:
 	if (err == -ENOSPC && total_allocated_on_err) {
@@ -1051,7 +1115,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
 			if (order-- == min_order) {
 				/* Try allocation through force merge method */
 				if (mm->clear_avail &&
-				    !__force_merge(mm, start, end, min_order)) {
+				    !__force_merge(mm, start, end, min_order, 0)) {
 					block = __drm_buddy_alloc_blocks(mm, start,
 									 end,
 									 min_order,
diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h
index 9689a7c5dd36..8b5273d4b2d1 100644
--- a/include/drm/drm_buddy.h
+++ b/include/drm/drm_buddy.h
@@ -160,6 +160,8 @@ int drm_buddy_block_trim(struct drm_buddy *mm,
 			 u64 new_size,
 			 struct list_head *blocks);
 
+void drm_buddy_reset_clear_state(struct drm_buddy *mm, bool is_clear);
+
 void drm_buddy_free_block(struct drm_buddy *mm, struct drm_buddy_block *block);
 
 void drm_buddy_free_list(struct drm_buddy *mm,
-- 
2.43.0

Re: [PATCH v2 2/3] drm/amdgpu: Reset the clear flag in buddy during resume

[Matthew Auld]

On 01/07/2025 20:08, Arunpravin Paneer Selvam wrote:
> - Added a handler in DRM buddy manager to reset the cleared
>    flag for the blocks in the freelist.
> 
> - This is necessary because, upon resuming, the VRAM becomes
>    cluttered with BIOS data, yet the VRAM backend manager
>    believes that everything has been cleared.
> 
> v2:
>    - Add lock before accessing drm_buddy_clear_reset_blocks()(Matthew Auld)
>    - Force merge the two dirty blocks.(Matthew Auld)
>    - Add a new unit test case for this issue.(Matthew Auld)
>    - Having this function being able to flip the state either way would be
>      good. (Matthew Brost)
> 
> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Cc: stable@vger.kernel.org
> Fixes: a68c7eaa7a8f ("drm/amdgpu: Enable clear page functionality")
> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3812
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c   |  2 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h      |  1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 17 ++++
>   drivers/gpu/drm/drm_buddy.c                  | 90 +++++++++++++++++---
>   include/drm/drm_buddy.h                      |  2 +
>   5 files changed, 99 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index a59f194e3360..b89e46f29b51 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -5193,6 +5193,8 @@ int amdgpu_device_resume(struct drm_device *dev, bool notify_clients)
>   		dev->dev->power.disable_depth--;
>   #endif
>   	}
> +
> +	amdgpu_vram_mgr_clear_reset_blocks(adev);
>   	adev->in_suspend = false;
>   
>   	if (amdgpu_acpi_smart_shift_update(dev, AMDGPU_SS_DEV_D0))
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> index 208b7d1d8a27..450e4bf093b7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> @@ -154,6 +154,7 @@ int amdgpu_vram_mgr_reserve_range(struct amdgpu_vram_mgr *mgr,
>   				  uint64_t start, uint64_t size);
>   int amdgpu_vram_mgr_query_page_status(struct amdgpu_vram_mgr *mgr,
>   				      uint64_t start);
> +void amdgpu_vram_mgr_clear_reset_blocks(struct amdgpu_device *adev);
>   
>   bool amdgpu_res_cpu_visible(struct amdgpu_device *adev,
>   			    struct ttm_resource *res);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> index abdc52b0895a..665656fbc948 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> @@ -782,6 +782,23 @@ uint64_t amdgpu_vram_mgr_vis_usage(struct amdgpu_vram_mgr *mgr)
>   	return atomic64_read(&mgr->vis_usage);
>   }
>   
> +/**
> + * amdgpu_vram_mgr_clear_reset_blocks - reset clear blocks
> + *
> + * @adev: amdgpu device pointer
> + *
> + * Reset the cleared drm buddy blocks.
> + */
> +void amdgpu_vram_mgr_clear_reset_blocks(struct amdgpu_device *adev)
> +{
> +	struct amdgpu_vram_mgr *mgr = &adev->mman.vram_mgr;
> +	struct drm_buddy *mm = &mgr->mm;
> +
> +	mutex_lock(&mgr->lock);
> +	drm_buddy_reset_clear_state(mm, false);
> +	mutex_unlock(&mgr->lock);
> +}
> +
>   /**
>    * amdgpu_vram_mgr_intersects - test each drm buddy block for intersection
>    *
> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
> index a1e652b7631d..436f7e4ee202 100644
> --- a/drivers/gpu/drm/drm_buddy.c
> +++ b/drivers/gpu/drm/drm_buddy.c
> @@ -12,6 +12,9 @@
>   
>   #include <drm/drm_buddy.h>
>   
> +#define FORCE_MERGE	BIT(0)
> +#define RESET_CLEAR	BIT(1)
> +
>   static struct kmem_cache *slab_blocks;
>   
>   static struct drm_buddy_block *drm_block_alloc(struct drm_buddy *mm,
> @@ -60,6 +63,16 @@ static void list_insert_sorted(struct drm_buddy *mm,
>   	__list_add(&block->link, node->link.prev, &node->link);
>   }
>   
> +static bool is_force_merge_enabled(unsigned int flags)
> +{
> +	return flags & FORCE_MERGE;
> +}
> +
> +static bool is_reset_clear_enabled(unsigned int flags)
> +{
> +	return flags & RESET_CLEAR;
> +}
> +
>   static void clear_reset(struct drm_buddy_block *block)
>   {
>   	block->header &= ~DRM_BUDDY_HEADER_CLEAR;
> @@ -122,7 +135,7 @@ __get_buddy(struct drm_buddy_block *block)
>   
>   static unsigned int __drm_buddy_free(struct drm_buddy *mm,
>   				     struct drm_buddy_block *block,
> -				     bool force_merge)
> +				     unsigned int flags)
>   {
>   	struct drm_buddy_block *parent;
>   	unsigned int order;
> @@ -135,7 +148,7 @@ static unsigned int __drm_buddy_free(struct drm_buddy *mm,
>   		if (!drm_buddy_block_is_free(buddy))
>   			break;
>   
> -		if (!force_merge) {
> +		if (!is_force_merge_enabled(flags)) {
>   			/*
>   			 * Check the block and its buddy clear state and exit
>   			 * the loop if they both have the dissimilar state.
> @@ -149,7 +162,9 @@ static unsigned int __drm_buddy_free(struct drm_buddy *mm,
>   		}
>   
>   		list_del(&buddy->link);
> -		if (force_merge && drm_buddy_block_is_clear(buddy))
> +		if (is_force_merge_enabled(flags) &&
> +		    !is_reset_clear_enabled(flags) &&
> +		    drm_buddy_block_is_clear(buddy))
>   			mm->clear_avail -= drm_buddy_block_size(mm, buddy);
>   
>   		drm_block_free(mm, block);
> @@ -167,7 +182,8 @@ static unsigned int __drm_buddy_free(struct drm_buddy *mm,
>   static int __force_merge(struct drm_buddy *mm,
>   			 u64 start,
>   			 u64 end,
> -			 unsigned int min_order)
> +			 unsigned int min_order,
> +			 unsigned int flags)
>   {
>   	unsigned int order;
>   	int i;
> @@ -178,6 +194,8 @@ static int __force_merge(struct drm_buddy *mm,
>   	if (min_order > mm->max_order)
>   		return -EINVAL;
>   
> +	flags |= FORCE_MERGE;
> +
>   	for (i = min_order - 1; i >= 0; i--) {
>   		struct drm_buddy_block *block, *prev;
>   
> @@ -198,7 +216,8 @@ static int __force_merge(struct drm_buddy *mm,
>   			if (!drm_buddy_block_is_free(buddy))
>   				continue;
>   
> -			WARN_ON(drm_buddy_block_is_clear(block) ==
> +			WARN_ON(!is_reset_clear_enabled(flags) &&
> +				drm_buddy_block_is_clear(block) ==
>   				drm_buddy_block_is_clear(buddy));
>   
>   			/*
> @@ -210,10 +229,11 @@ static int __force_merge(struct drm_buddy *mm,
>   				prev = list_prev_entry(prev, link);
>   
>   			list_del(&block->link);
> -			if (drm_buddy_block_is_clear(block))
> +			if (!is_reset_clear_enabled(flags) &&
> +			    drm_buddy_block_is_clear(block))
>   				mm->clear_avail -= drm_buddy_block_size(mm, block);
>   
> -			order = __drm_buddy_free(mm, block, true);
> +			order = __drm_buddy_free(mm, block, flags);
>   			if (order >= min_order)
>   				return 0;
>   		}
> @@ -336,7 +356,7 @@ void drm_buddy_fini(struct drm_buddy *mm)
>   	for (i = 0; i < mm->n_roots; ++i) {
>   		order = ilog2(size) - ilog2(mm->chunk_size);
>   		start = drm_buddy_block_offset(mm->roots[i]);
> -		__force_merge(mm, start, start + size, order);
> +		__force_merge(mm, start, start + size, order, 0);
>   
>   		if (WARN_ON(!drm_buddy_block_is_free(mm->roots[i])))
>   			kunit_fail_current_test("buddy_fini() root");
> @@ -405,6 +425,50 @@ drm_get_buddy(struct drm_buddy_block *block)
>   }
>   EXPORT_SYMBOL(drm_get_buddy);
>   
> +/**
> + * drm_buddy_reset_clear_state - reset blocks clear state
> + *
> + * @mm: DRM buddy manager
> + * @is_clear: blocks clear state
> + *
> + * Reset the clear state based on @clear value for each block
> + * in the freelist.
> + */
> +void drm_buddy_reset_clear_state(struct drm_buddy *mm, bool is_clear)
> +{
> +	u64 root_size, size, start;
> +	unsigned int order;
> +	int i;
> +
> +	for (i = 0; i <= mm->max_order; ++i) {
> +		struct drm_buddy_block *block;
> +
> +		list_for_each_entry_reverse(block, &mm->free_list[i], link) {
> +			if (is_clear != drm_buddy_block_is_clear(block)) {
> +				if (is_clear) {
> +					mark_cleared(block);
> +					mm->clear_avail += drm_buddy_block_size(mm, block);
> +				} else {
> +					clear_reset(block);
> +					mm->clear_avail -= drm_buddy_block_size(mm, block);
> +				}
> +			}
> +		}
> +	}

Is it not possible to do the merge step first and then go through 
whatever is left marking at clear/dirty? If we do that then we maybe 
don't need any extra changes or flags outside of reset_clear_state? Or 
am I missing something?

> +
> +	/* Force merge the two dirty or two cleared blocks */
> +	size = mm->size;
> +	for (i = 0; i < mm->n_roots; ++i) {
> +		order = ilog2(size) - ilog2(mm->chunk_size);
> +		start = drm_buddy_block_offset(mm->roots[i]);
> +		__force_merge(mm, start, start + size, order, RESET_CLEAR);
> +
> +		root_size = mm->chunk_size << order;
> +		size -= root_size;
> +	}
> +}
> +EXPORT_SYMBOL(drm_buddy_reset_clear_state);
> +
>   /**
>    * drm_buddy_free_block - free a block
>    *
> @@ -419,7 +483,7 @@ void drm_buddy_free_block(struct drm_buddy *mm,
>   	if (drm_buddy_block_is_clear(block))
>   		mm->clear_avail += drm_buddy_block_size(mm, block);
>   
> -	__drm_buddy_free(mm, block, false);
> +	__drm_buddy_free(mm, block, 0);
>   }
>   EXPORT_SYMBOL(drm_buddy_free_block);
>   
> @@ -566,7 +630,7 @@ __alloc_range_bias(struct drm_buddy *mm,
>   	if (buddy &&
>   	    (drm_buddy_block_is_free(block) &&
>   	     drm_buddy_block_is_free(buddy)))
> -		__drm_buddy_free(mm, block, false);
> +		__drm_buddy_free(mm, block, 0);
>   	return ERR_PTR(err);
>   }
>   
> @@ -684,7 +748,7 @@ alloc_from_freelist(struct drm_buddy *mm,
>   
>   err_undo:
>   	if (tmp != order)
> -		__drm_buddy_free(mm, block, false);
> +		__drm_buddy_free(mm, block, 0);
>   	return ERR_PTR(err);
>   }
>   
> @@ -770,7 +834,7 @@ static int __alloc_range(struct drm_buddy *mm,
>   	if (buddy &&
>   	    (drm_buddy_block_is_free(block) &&
>   	     drm_buddy_block_is_free(buddy)))
> -		__drm_buddy_free(mm, block, false);
> +		__drm_buddy_free(mm, block, 0);
>   
>   err_free:
>   	if (err == -ENOSPC && total_allocated_on_err) {
> @@ -1051,7 +1115,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>   			if (order-- == min_order) {
>   				/* Try allocation through force merge method */
>   				if (mm->clear_avail &&
> -				    !__force_merge(mm, start, end, min_order)) {
> +				    !__force_merge(mm, start, end, min_order, 0)) {
>   					block = __drm_buddy_alloc_blocks(mm, start,
>   									 end,
>   									 min_order,
> diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h
> index 9689a7c5dd36..8b5273d4b2d1 100644
> --- a/include/drm/drm_buddy.h
> +++ b/include/drm/drm_buddy.h
> @@ -160,6 +160,8 @@ int drm_buddy_block_trim(struct drm_buddy *mm,
>   			 u64 new_size,
>   			 struct list_head *blocks);
>   
> +void drm_buddy_reset_clear_state(struct drm_buddy *mm, bool is_clear);
> +
>   void drm_buddy_free_block(struct drm_buddy *mm, struct drm_buddy_block *block);
>   
>   void drm_buddy_free_list(struct drm_buddy *mm,

Re: [PATCH v2 2/3] drm/amdgpu: Reset the clear flag in buddy during resume

[Arunpravin Paneer Selvam]

Hi Matthew,

On 7/4/2025 2:22 PM, Matthew Auld wrote:
> On 01/07/2025 20:08, Arunpravin Paneer Selvam wrote:
>> - Added a handler in DRM buddy manager to reset the cleared
>>    flag for the blocks in the freelist.
>>
>> - This is necessary because, upon resuming, the VRAM becomes
>>    cluttered with BIOS data, yet the VRAM backend manager
>>    believes that everything has been cleared.
>>
>> v2:
>>    - Add lock before accessing drm_buddy_clear_reset_blocks()(Matthew 
>> Auld)
>>    - Force merge the two dirty blocks.(Matthew Auld)
>>    - Add a new unit test case for this issue.(Matthew Auld)
>>    - Having this function being able to flip the state either way 
>> would be
>>      good. (Matthew Brost)
>>
>> Signed-off-by: Arunpravin Paneer Selvam 
>> <Arunpravin.PaneerSelvam@amd.com>
>> Suggested-by: Christian König <christian.koenig@amd.com>
>> Cc: stable@vger.kernel.org
>> Fixes: a68c7eaa7a8f ("drm/amdgpu: Enable clear page functionality")
>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3812
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c   |  2 +
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h      |  1 +
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 17 ++++
>>   drivers/gpu/drm/drm_buddy.c                  | 90 +++++++++++++++++---
>>   include/drm/drm_buddy.h                      |  2 +
>>   5 files changed, 99 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index a59f194e3360..b89e46f29b51 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -5193,6 +5193,8 @@ int amdgpu_device_resume(struct drm_device 
>> *dev, bool notify_clients)
>>           dev->dev->power.disable_depth--;
>>   #endif
>>       }
>> +
>> +    amdgpu_vram_mgr_clear_reset_blocks(adev);
>>       adev->in_suspend = false;
>>         if (amdgpu_acpi_smart_shift_update(dev, AMDGPU_SS_DEV_D0))
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> index 208b7d1d8a27..450e4bf093b7 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> @@ -154,6 +154,7 @@ int amdgpu_vram_mgr_reserve_range(struct 
>> amdgpu_vram_mgr *mgr,
>>                     uint64_t start, uint64_t size);
>>   int amdgpu_vram_mgr_query_page_status(struct amdgpu_vram_mgr *mgr,
>>                         uint64_t start);
>> +void amdgpu_vram_mgr_clear_reset_blocks(struct amdgpu_device *adev);
>>     bool amdgpu_res_cpu_visible(struct amdgpu_device *adev,
>>                   struct ttm_resource *res);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> index abdc52b0895a..665656fbc948 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>> @@ -782,6 +782,23 @@ uint64_t amdgpu_vram_mgr_vis_usage(struct 
>> amdgpu_vram_mgr *mgr)
>>       return atomic64_read(&mgr->vis_usage);
>>   }
>>   +/**
>> + * amdgpu_vram_mgr_clear_reset_blocks - reset clear blocks
>> + *
>> + * @adev: amdgpu device pointer
>> + *
>> + * Reset the cleared drm buddy blocks.
>> + */
>> +void amdgpu_vram_mgr_clear_reset_blocks(struct amdgpu_device *adev)
>> +{
>> +    struct amdgpu_vram_mgr *mgr = &adev->mman.vram_mgr;
>> +    struct drm_buddy *mm = &mgr->mm;
>> +
>> +    mutex_lock(&mgr->lock);
>> +    drm_buddy_reset_clear_state(mm, false);
>> +    mutex_unlock(&mgr->lock);
>> +}
>> +
>>   /**
>>    * amdgpu_vram_mgr_intersects - test each drm buddy block for 
>> intersection
>>    *
>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>> index a1e652b7631d..436f7e4ee202 100644
>> --- a/drivers/gpu/drm/drm_buddy.c
>> +++ b/drivers/gpu/drm/drm_buddy.c
>> @@ -12,6 +12,9 @@
>>     #include <drm/drm_buddy.h>
>>   +#define FORCE_MERGE    BIT(0)
>> +#define RESET_CLEAR    BIT(1)
>> +
>>   static struct kmem_cache *slab_blocks;
>>     static struct drm_buddy_block *drm_block_alloc(struct drm_buddy *mm,
>> @@ -60,6 +63,16 @@ static void list_insert_sorted(struct drm_buddy *mm,
>>       __list_add(&block->link, node->link.prev, &node->link);
>>   }
>>   +static bool is_force_merge_enabled(unsigned int flags)
>> +{
>> +    return flags & FORCE_MERGE;
>> +}
>> +
>> +static bool is_reset_clear_enabled(unsigned int flags)
>> +{
>> +    return flags & RESET_CLEAR;
>> +}
>> +
>>   static void clear_reset(struct drm_buddy_block *block)
>>   {
>>       block->header &= ~DRM_BUDDY_HEADER_CLEAR;
>> @@ -122,7 +135,7 @@ __get_buddy(struct drm_buddy_block *block)
>>     static unsigned int __drm_buddy_free(struct drm_buddy *mm,
>>                        struct drm_buddy_block *block,
>> -                     bool force_merge)
>> +                     unsigned int flags)
>>   {
>>       struct drm_buddy_block *parent;
>>       unsigned int order;
>> @@ -135,7 +148,7 @@ static unsigned int __drm_buddy_free(struct 
>> drm_buddy *mm,
>>           if (!drm_buddy_block_is_free(buddy))
>>               break;
>>   -        if (!force_merge) {
>> +        if (!is_force_merge_enabled(flags)) {
>>               /*
>>                * Check the block and its buddy clear state and exit
>>                * the loop if they both have the dissimilar state.
>> @@ -149,7 +162,9 @@ static unsigned int __drm_buddy_free(struct 
>> drm_buddy *mm,
>>           }
>>             list_del(&buddy->link);
>> -        if (force_merge && drm_buddy_block_is_clear(buddy))
>> +        if (is_force_merge_enabled(flags) &&
>> +            !is_reset_clear_enabled(flags) &&
>> +            drm_buddy_block_is_clear(buddy))
>>               mm->clear_avail -= drm_buddy_block_size(mm, buddy);
>>             drm_block_free(mm, block);
>> @@ -167,7 +182,8 @@ static unsigned int __drm_buddy_free(struct 
>> drm_buddy *mm,
>>   static int __force_merge(struct drm_buddy *mm,
>>                u64 start,
>>                u64 end,
>> -             unsigned int min_order)
>> +             unsigned int min_order,
>> +             unsigned int flags)
>>   {
>>       unsigned int order;
>>       int i;
>> @@ -178,6 +194,8 @@ static int __force_merge(struct drm_buddy *mm,
>>       if (min_order > mm->max_order)
>>           return -EINVAL;
>>   +    flags |= FORCE_MERGE;
>> +
>>       for (i = min_order - 1; i >= 0; i--) {
>>           struct drm_buddy_block *block, *prev;
>>   @@ -198,7 +216,8 @@ static int __force_merge(struct drm_buddy *mm,
>>               if (!drm_buddy_block_is_free(buddy))
>>                   continue;
>>   -            WARN_ON(drm_buddy_block_is_clear(block) ==
>> +            WARN_ON(!is_reset_clear_enabled(flags) &&
>> +                drm_buddy_block_is_clear(block) ==
>>                   drm_buddy_block_is_clear(buddy));
>>                 /*
>> @@ -210,10 +229,11 @@ static int __force_merge(struct drm_buddy *mm,
>>                   prev = list_prev_entry(prev, link);
>>                 list_del(&block->link);
>> -            if (drm_buddy_block_is_clear(block))
>> +            if (!is_reset_clear_enabled(flags) &&
>> +                drm_buddy_block_is_clear(block))
>>                   mm->clear_avail -= drm_buddy_block_size(mm, block);
>>   -            order = __drm_buddy_free(mm, block, true);
>> +            order = __drm_buddy_free(mm, block, flags);
>>               if (order >= min_order)
>>                   return 0;
>>           }
>> @@ -336,7 +356,7 @@ void drm_buddy_fini(struct drm_buddy *mm)
>>       for (i = 0; i < mm->n_roots; ++i) {
>>           order = ilog2(size) - ilog2(mm->chunk_size);
>>           start = drm_buddy_block_offset(mm->roots[i]);
>> -        __force_merge(mm, start, start + size, order);
>> +        __force_merge(mm, start, start + size, order, 0);
>>             if (WARN_ON(!drm_buddy_block_is_free(mm->roots[i])))
>>               kunit_fail_current_test("buddy_fini() root");
>> @@ -405,6 +425,50 @@ drm_get_buddy(struct drm_buddy_block *block)
>>   }
>>   EXPORT_SYMBOL(drm_get_buddy);
>>   +/**
>> + * drm_buddy_reset_clear_state - reset blocks clear state
>> + *
>> + * @mm: DRM buddy manager
>> + * @is_clear: blocks clear state
>> + *
>> + * Reset the clear state based on @clear value for each block
>> + * in the freelist.
>> + */
>> +void drm_buddy_reset_clear_state(struct drm_buddy *mm, bool is_clear)
>> +{
>> +    u64 root_size, size, start;
>> +    unsigned int order;
>> +    int i;
>> +
>> +    for (i = 0; i <= mm->max_order; ++i) {
>> +        struct drm_buddy_block *block;
>> +
>> +        list_for_each_entry_reverse(block, &mm->free_list[i], link) {
>> +            if (is_clear != drm_buddy_block_is_clear(block)) {
>> +                if (is_clear) {
>> +                    mark_cleared(block);
>> +                    mm->clear_avail += drm_buddy_block_size(mm, block);
>> +                } else {
>> +                    clear_reset(block);
>> +                    mm->clear_avail -= drm_buddy_block_size(mm, block);
>> +                }
>> +            }
>> +        }
>> +    }
>
> Is it not possible to do the merge step first and then go through 
> whatever is left marking at clear/dirty? If we do that then we maybe 
> don't need any extra changes or flags outside of reset_clear_state? Or 
> am I missing something?

Yes you are right, I will make the changes accordingly.

Thanks,

Arun.

>
>> +
>> +    /* Force merge the two dirty or two cleared blocks */
>> +    size = mm->size;
>> +    for (i = 0; i < mm->n_roots; ++i) {
>> +        order = ilog2(size) - ilog2(mm->chunk_size);
>> +        start = drm_buddy_block_offset(mm->roots[i]);
>> +        __force_merge(mm, start, start + size, order, RESET_CLEAR);
>> +
>> +        root_size = mm->chunk_size << order;
>> +        size -= root_size;
>> +    }
>> +}
>> +EXPORT_SYMBOL(drm_buddy_reset_clear_state);
>> +
>>   /**
>>    * drm_buddy_free_block - free a block
>>    *
>> @@ -419,7 +483,7 @@ void drm_buddy_free_block(struct drm_buddy *mm,
>>       if (drm_buddy_block_is_clear(block))
>>           mm->clear_avail += drm_buddy_block_size(mm, block);
>>   -    __drm_buddy_free(mm, block, false);
>> +    __drm_buddy_free(mm, block, 0);
>>   }
>>   EXPORT_SYMBOL(drm_buddy_free_block);
>>   @@ -566,7 +630,7 @@ __alloc_range_bias(struct drm_buddy *mm,
>>       if (buddy &&
>>           (drm_buddy_block_is_free(block) &&
>>            drm_buddy_block_is_free(buddy)))
>> -        __drm_buddy_free(mm, block, false);
>> +        __drm_buddy_free(mm, block, 0);
>>       return ERR_PTR(err);
>>   }
>>   @@ -684,7 +748,7 @@ alloc_from_freelist(struct drm_buddy *mm,
>>     err_undo:
>>       if (tmp != order)
>> -        __drm_buddy_free(mm, block, false);
>> +        __drm_buddy_free(mm, block, 0);
>>       return ERR_PTR(err);
>>   }
>>   @@ -770,7 +834,7 @@ static int __alloc_range(struct drm_buddy *mm,
>>       if (buddy &&
>>           (drm_buddy_block_is_free(block) &&
>>            drm_buddy_block_is_free(buddy)))
>> -        __drm_buddy_free(mm, block, false);
>> +        __drm_buddy_free(mm, block, 0);
>>     err_free:
>>       if (err == -ENOSPC && total_allocated_on_err) {
>> @@ -1051,7 +1115,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>>               if (order-- == min_order) {
>>                   /* Try allocation through force merge method */
>>                   if (mm->clear_avail &&
>> -                    !__force_merge(mm, start, end, min_order)) {
>> +                    !__force_merge(mm, start, end, min_order, 0)) {
>>                       block = __drm_buddy_alloc_blocks(mm, start,
>>                                        end,
>>                                        min_order,
>> diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h
>> index 9689a7c5dd36..8b5273d4b2d1 100644
>> --- a/include/drm/drm_buddy.h
>> +++ b/include/drm/drm_buddy.h
>> @@ -160,6 +160,8 @@ int drm_buddy_block_trim(struct drm_buddy *mm,
>>                u64 new_size,
>>                struct list_head *blocks);
>>   +void drm_buddy_reset_clear_state(struct drm_buddy *mm, bool 
>> is_clear);
>> +
>>   void drm_buddy_free_block(struct drm_buddy *mm, struct 
>> drm_buddy_block *block);
>>     void drm_buddy_free_list(struct drm_buddy *mm,
>

[PATCH v2 3/3] drm/buddy: Add a new unit test case for buffer clearance issue

[Arunpravin Paneer Selvam]

Add a new unit test case for buffer clearance issue during
resume.

Using a non-power-of-two mm size, allocate alternating blocks of
4KiB in an even sequence and free them as cleared. All alternate
blocks should be marked as dirty and the split blocks should be
merged back to their original size when the blocks clear reset
function is called.

Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
---
 drivers/gpu/drm/tests/drm_buddy_test.c | 41 ++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_buddy_test.c b/drivers/gpu/drm/tests/drm_buddy_test.c
index 7a0e523651f0..e23922a21c77 100644
--- a/drivers/gpu/drm/tests/drm_buddy_test.c
+++ b/drivers/gpu/drm/tests/drm_buddy_test.c
@@ -408,6 +408,47 @@ static void drm_test_buddy_alloc_clear(struct kunit *test)
 				"buddy_alloc hit an error size=%lu\n", ps);
 	drm_buddy_free_list(&mm, &allocated, DRM_BUDDY_CLEARED);
 	drm_buddy_fini(&mm);
+
+	/*
+	 * Using a non-power-of-two mm size, allocate alternating blocks of 4KiB in an
+	 * even sequence and free them as cleared. All blocks should be marked as
+	 * dirty and the split blocks should be merged back to their original
+	 * size when the blocks clear reset function is called.
+	 */
+	KUNIT_EXPECT_FALSE(test, drm_buddy_init(&mm, mm_size, ps));
+	KUNIT_EXPECT_EQ(test, mm.max_order, max_order);
+
+	i = 0;
+	n_pages = mm_size / ps;
+	do {
+		if (i % 2 == 0)
+			KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
+									    ps, ps, &allocated, 0),
+						"buddy_alloc hit an error size=%lu\n", ps);
+	} while (++i < n_pages);
+
+	drm_buddy_free_list(&mm, &allocated, DRM_BUDDY_CLEARED);
+	drm_buddy_reset_clear_state(&mm, false);
+	KUNIT_EXPECT_EQ(test, mm.clear_avail, 0);
+
+	/*
+	 * Using a non-power-of-two mm size, allocate alternating blocks of 4KiB in an
+	 * odd sequence and free them as cleared. All blocks should be marked as
+	 * cleared and the split blocks should be merged back to their original
+	 * size when the blocks clear reset function is called.
+	 */
+	i = 0;
+	do {
+		if (i % 2 != 0)
+			KUNIT_ASSERT_FALSE_MSG(test, drm_buddy_alloc_blocks(&mm, 0, mm_size,
+									    ps, ps, &allocated, 0),
+						"buddy_alloc hit an error size=%lu\n", ps);
+	} while (++i < n_pages);
+
+	drm_buddy_free_list(&mm, &allocated, DRM_BUDDY_CLEARED);
+	drm_buddy_reset_clear_state(&mm, true);
+	KUNIT_EXPECT_EQ(test, mm.clear_avail, mm_size);
+	drm_buddy_fini(&mm);
 }
 
 static void drm_test_buddy_alloc_contiguous(struct kunit *test)
-- 
2.43.0

Re: [PATCH v2 1/3] drm/amdgpu: Dirty cleared blocks on free

[Christian König]

On 01.07.25 21:08, Arunpravin Paneer Selvam wrote:
> Set the dirty bit when the memory resource is not cleared
> during BO release.
> 
> v2(Christian):
>   - Drop the cleared flag set to false.
>   - Improve the amdgpu_vram_mgr_set_clear_state() function.
> 
> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Cc: stable@vger.kernel.org
> Fixes: a68c7eaa7a8f ("drm/amdgpu: Enable clear page functionality")
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c      | 1 -
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h | 5 ++++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 9c5df35f05b7..86eb6d47dcc5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -409,7 +409,6 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo,
>  		if (r) {
>  			goto error;
>  		} else if (wipe_fence) {
> -			amdgpu_vram_mgr_set_cleared(bo->resource);

Mhm, that looks incorrect to me.

Why don't we consider the resource cleared after it go wiped during eviction?

Regards,
Christian.

>  			dma_fence_put(fence);
>  			fence = wipe_fence;
>  		}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
> index b256cbc2bc27..2c88d5fd87da 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
> @@ -66,7 +66,10 @@ to_amdgpu_vram_mgr_resource(struct ttm_resource *res)
>  
>  static inline void amdgpu_vram_mgr_set_cleared(struct ttm_resource *res)
>  {
> -	to_amdgpu_vram_mgr_resource(res)->flags |= DRM_BUDDY_CLEARED;
> +	struct amdgpu_vram_mgr_resource *ares = to_amdgpu_vram_mgr_resource(res);
> +
> +	WARN_ON(ares->flags & DRM_BUDDY_CLEARED);
> +	ares->flags |= DRM_BUDDY_CLEARED;
>  }
>  
>  #endif

Re: [PATCH v2 1/3] drm/amdgpu: Dirty cleared blocks on free

[Arunpravin Paneer Selvam]

[-- Attachment #1: Type: text/plain, Size: 2505 bytes --]

Hi Christian,

On 7/2/2025 1:27 PM, Christian König wrote:
>
> On 01.07.25 21:08, Arunpravin Paneer Selvam wrote:
>> Set the dirty bit when the memory resource is not cleared
>> during BO release.
>>
>> v2(Christian):
>>    - Drop the cleared flag set to false.
>>    - Improve the amdgpu_vram_mgr_set_clear_state() function.
>>
>> Signed-off-by: Arunpravin Paneer Selvam<Arunpravin.PaneerSelvam@amd.com>
>> Suggested-by: Christian König<christian.koenig@amd.com>
>> Cc:stable@vger.kernel.org
>> Fixes: a68c7eaa7a8f ("drm/amdgpu: Enable clear page functionality")
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c      | 1 -
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h | 5 ++++-
>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 9c5df35f05b7..86eb6d47dcc5 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -409,7 +409,6 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo,
>>   		if (r) {
>>   			goto error;
>>   		} else if (wipe_fence) {
>> -			amdgpu_vram_mgr_set_cleared(bo->resource);
> Mhm, that looks incorrect to me.
>
> Why don't we consider the resource cleared after it go wiped during eviction?

Modifying the resource flag here doesn't go into effect until we call 
the drm_buddy_free_list() in amdgpu_vram_mgr_del(). This BO will be 
cleared once again after executing amdgpu_bo_release_notify(). With the 
new implementation, there's a chance that changing the resource flag the 
second time would cause the WARN_ON to occur. Hence I removed the 
resource cleared function call in amdgpu_move_blit. Thanks, Arun.

>
> Regards,
> Christian.
>
>>   			dma_fence_put(fence);
>>   			fence = wipe_fence;
>>   		}
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
>> index b256cbc2bc27..2c88d5fd87da 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
>> @@ -66,7 +66,10 @@ to_amdgpu_vram_mgr_resource(struct ttm_resource *res)
>>   
>>   static inline void amdgpu_vram_mgr_set_cleared(struct ttm_resource *res)
>>   {
>> -	to_amdgpu_vram_mgr_resource(res)->flags |= DRM_BUDDY_CLEARED;
>> +	struct amdgpu_vram_mgr_resource *ares = to_amdgpu_vram_mgr_resource(res);
>> +
>> +	WARN_ON(ares->flags & DRM_BUDDY_CLEARED);
>> +	ares->flags |= DRM_BUDDY_CLEARED;
>>   }
>>   
>>   #endif

[-- Attachment #2: Type: text/html, Size: 3544 bytes --]

Re: [PATCH v2 1/3] drm/amdgpu: Dirty cleared blocks on free

[Christian König]

On 02.07.25 13:58, Arunpravin Paneer Selvam wrote:
> Hi Christian,
> 
> On 7/2/2025 1:27 PM, Christian König wrote:
>> On 01.07.25 21:08, Arunpravin Paneer Selvam wrote:
>>> Set the dirty bit when the memory resource is not cleared
>>> during BO release.
>>>
>>> v2(Christian):
>>>   - Drop the cleared flag set to false.
>>>   - Improve the amdgpu_vram_mgr_set_clear_state() function.
>>>
>>> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
>>> Suggested-by: Christian König <christian.koenig@amd.com>
>>> Cc: stable@vger.kernel.org
>>> Fixes: a68c7eaa7a8f ("drm/amdgpu: Enable clear page functionality")
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c      | 1 -
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h | 5 ++++-
>>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> index 9c5df35f05b7..86eb6d47dcc5 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>> @@ -409,7 +409,6 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo,
>>>  		if (r) {
>>>  			goto error;
>>>  		} else if (wipe_fence) {
>>> -			amdgpu_vram_mgr_set_cleared(bo->resource);
>> Mhm, that looks incorrect to me.
>>
>> Why don't we consider the resource cleared after it go wiped during eviction?
> 
> Modifying the resource flag here doesn't go into effect until we call the drm_buddy_free_list() in amdgpu_vram_mgr_del(). This BO will be cleared once again after executing amdgpu_bo_release_notify(). With the new implementation, there's a chance that changing the resource flag the second time would cause the WARN_ON to occur. Hence I removed the resource cleared function call in amdgpu_move_blit. Thanks, Arun.

Something fishy is going on that we don't fully understand.

What happens here is that we move from VRAM to GTT, clear the VRAM BO after the move and set the flag.

When the BO is destroyed the it is backed by GTT and not VRAM any more, so no clear operation and no flag setting is performed.

It looks more like we forget to clear the flag in some cases.

Regards,
Christian.

> 
>> Regards,
>> Christian.
>>
>>>  			dma_fence_put(fence);
>>>  			fence = wipe_fence;
>>>  		}
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
>>> index b256cbc2bc27..2c88d5fd87da 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
>>> @@ -66,7 +66,10 @@ to_amdgpu_vram_mgr_resource(struct ttm_resource *res)
>>>  
>>>  static inline void amdgpu_vram_mgr_set_cleared(struct ttm_resource *res)
>>>  {
>>> -	to_amdgpu_vram_mgr_resource(res)->flags |= DRM_BUDDY_CLEARED;
>>> +	struct amdgpu_vram_mgr_resource *ares = to_amdgpu_vram_mgr_resource(res);
>>> +
>>> +	WARN_ON(ares->flags & DRM_BUDDY_CLEARED);
>>> +	ares->flags |= DRM_BUDDY_CLEARED;
>>>  }
>>>  
>>>  #endif

Re: [PATCH v2 1/3] drm/amdgpu: Dirty cleared blocks on free

[Arunpravin Paneer Selvam]

On 7/2/2025 7:11 PM, Christian König wrote:
> On 02.07.25 13:58, Arunpravin Paneer Selvam wrote:
>> Hi Christian,
>>
>> On 7/2/2025 1:27 PM, Christian König wrote:
>>> On 01.07.25 21:08, Arunpravin Paneer Selvam wrote:
>>>> Set the dirty bit when the memory resource is not cleared
>>>> during BO release.
>>>>
>>>> v2(Christian):
>>>>    - Drop the cleared flag set to false.
>>>>    - Improve the amdgpu_vram_mgr_set_clear_state() function.
>>>>
>>>> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
>>>> Suggested-by: Christian König <christian.koenig@amd.com>
>>>> Cc: stable@vger.kernel.org
>>>> Fixes: a68c7eaa7a8f ("drm/amdgpu: Enable clear page functionality")
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c      | 1 -
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h | 5 ++++-
>>>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> index 9c5df35f05b7..86eb6d47dcc5 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>> @@ -409,7 +409,6 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo,
>>>>   		if (r) {
>>>>   			goto error;
>>>>   		} else if (wipe_fence) {
>>>> -			amdgpu_vram_mgr_set_cleared(bo->resource);
>>> Mhm, that looks incorrect to me.
>>>
>>> Why don't we consider the resource cleared after it go wiped during eviction?
>> Modifying the resource flag here doesn't go into effect until we call the drm_buddy_free_list() in amdgpu_vram_mgr_del(). This BO will be cleared once again after executing amdgpu_bo_release_notify(). With the new implementation, there's a chance that changing the resource flag the second time would cause the WARN_ON to occur. Hence I removed the resource cleared function call in amdgpu_move_blit. Thanks, Arun.
> Something fishy is going on that we don't fully understand.
>
> What happens here is that we move from VRAM to GTT, clear the VRAM BO after the move and set the flag.
>
> When the BO is destroyed the it is backed by GTT and not VRAM any more, so no clear operation and no flag setting is performed.
>
> It looks more like we forget to clear the flag in some cases.

Got it. I tried to add back the resource cleared after being wiped 
during eviction and verified it, and I didn't observe WARN_ON messages.

I will update the patch and send the next version.

Thanks,

Arun.

>
> Regards,
> Christian.
>
>>> Regards,
>>> Christian.
>>>
>>>>   			dma_fence_put(fence);
>>>>   			fence = wipe_fence;
>>>>   		}
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
>>>> index b256cbc2bc27..2c88d5fd87da 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
>>>> @@ -66,7 +66,10 @@ to_amdgpu_vram_mgr_resource(struct ttm_resource *res)
>>>>   
>>>>   static inline void amdgpu_vram_mgr_set_cleared(struct ttm_resource *res)
>>>>   {
>>>> -	to_amdgpu_vram_mgr_resource(res)->flags |= DRM_BUDDY_CLEARED;
>>>> +	struct amdgpu_vram_mgr_resource *ares = to_amdgpu_vram_mgr_resource(res);
>>>> +
>>>> +	WARN_ON(ares->flags & DRM_BUDDY_CLEARED);
>>>> +	ares->flags |= DRM_BUDDY_CLEARED;
>>>>   }
>>>>   
>>>>   #endif