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

[Arunpravin Paneer Selvam <arunpravin.paneerselvam@amd.com>]

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,
>