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
- 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,
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)@@ -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)@@ -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);@@ -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);@@ -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,
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,
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
linux-stable-mirror@lists.linaro.org
-
Arunpravin Paneer Selvam -
Arunpravin Paneer Selvam
-
Christian König -
Matthew Auld