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

[Matthew Auld <matthew.auld@intel.com>]

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,