Re: [PATCH v9 2/3] drm/amdgpu: Enable clear page functionality

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

Hi Alex,

On 3/26/2024 8:23 PM, Alex Deucher wrote:
> On Tue, Mar 26, 2024 at 10:01 AM Alex Deucher <alexdeucher@gmail.com> wrote:
>> On Tue, Mar 26, 2024 at 9:59 AM Paneer Selvam, Arunpravin
>> <arunpravin.paneerselvam@amd.com> wrote:
>>> Hi Alex,
>>>
>>> On 3/26/2024 7:08 PM, Alex Deucher wrote:
>>>> On Mon, Mar 18, 2024 at 5:47 PM Arunpravin Paneer Selvam
>>>> <Arunpravin.PaneerSelvam@amd.com> wrote:
>>>>> Add clear page support in vram memory region.
>>>>>
>>>>> v1(Christian):
>>>>>     - Dont handle clear page as TTM flag since when moving the BO back
>>>>>       in from GTT again we don't need that.
>>>>>     - Make a specialized version of amdgpu_fill_buffer() which only
>>>>>       clears the VRAM areas which are not already cleared
>>>>>     - Drop the TTM_PL_FLAG_WIPE_ON_RELEASE check in
>>>>>       amdgpu_object.c
>>>>>
>>>>> v2:
>>>>>     - Modify the function name amdgpu_ttm_* (Alex)
>>>>>     - Drop the delayed parameter (Christian)
>>>>>     - handle amdgpu_res_cleared(&cursor) just above the size
>>>>>       calculation (Christian)
>>>>>     - Use AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE for clearing the buffers
>>>>>       in the free path to properly wait for fences etc.. (Christian)
>>>>>
>>>>> v3(Christian):
>>>>>     - Remove buffer clear code in VRAM manager instead change the
>>>>>       AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE handling to set
>>>>>       the DRM_BUDDY_CLEARED flag.
>>>>>     - Remove ! from amdgpu_res_cleared(&cursor) check.
>>>>>
>>>>> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com>
>>>>> Suggested-by: Christian König <christian.koenig@amd.com>
>>>>> Acked-by: Felix Kuehling <felix.kuehling@amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_object.c    | 22 ++++---
>>>>>    .../gpu/drm/amd/amdgpu/amdgpu_res_cursor.h    | 25 ++++++++
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c       | 61 ++++++++++++++++++-
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h       |  5 +-
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c  |  6 +-
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h  |  5 ++
>>>>>    6 files changed, 111 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> index 8bc79924d171..c92d92b28a57 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> @@ -39,6 +39,7 @@
>>>>>    #include "amdgpu.h"
>>>>>    #include "amdgpu_trace.h"
>>>>>    #include "amdgpu_amdkfd.h"
>>>>> +#include "amdgpu_vram_mgr.h"
>>>>>
>>>>>    /**
>>>>>     * DOC: amdgpu_object
>>>>> @@ -601,8 +602,7 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
>>>>>           if (!amdgpu_bo_support_uswc(bo->flags))
>>>>>                   bo->flags &= ~AMDGPU_GEM_CREATE_CPU_GTT_USWC;
>>>>>
>>>>> -       if (adev->ras_enabled)
>>>>> -               bo->flags |= AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE;
>>>>> +       bo->flags |= AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE;
>>>>>
>>>>>           bo->tbo.bdev = &adev->mman.bdev;
>>>>>           if (bp->domain & (AMDGPU_GEM_DOMAIN_GWS | AMDGPU_GEM_DOMAIN_OA |
>>>>> @@ -632,15 +632,17 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
>>>>>
>>>>>           if (bp->flags & AMDGPU_GEM_CREATE_VRAM_CLEARED &&
>>>>>               bo->tbo.resource->mem_type == TTM_PL_VRAM) {
>>>>> -               struct dma_fence *fence;
>>>>> +               struct dma_fence *fence = NULL;
>>>>>
>>>>> -               r = amdgpu_fill_buffer(bo, 0, bo->tbo.base.resv, &fence, true);
>>>>> +               r = amdgpu_ttm_clear_buffer(bo, bo->tbo.base.resv, &fence);
>>>>>                   if (unlikely(r))
>>>>>                           goto fail_unreserve;
>>>>>
>>>>> -               dma_resv_add_fence(bo->tbo.base.resv, fence,
>>>>> -                                  DMA_RESV_USAGE_KERNEL);
>>>>> -               dma_fence_put(fence);
>>>>> +               if (fence) {
>>>>> +                       dma_resv_add_fence(bo->tbo.base.resv, fence,
>>>>> +                                          DMA_RESV_USAGE_KERNEL);
>>>>> +                       dma_fence_put(fence);
>>>>> +               }
>>>>>           }
>>>>>           if (!bp->resv)
>>>>>                   amdgpu_bo_unreserve(bo);
>>>>> @@ -1365,8 +1367,12 @@ void amdgpu_bo_release_notify(struct ttm_buffer_object *bo)
>>>>>           if (WARN_ON_ONCE(!dma_resv_trylock(bo->base.resv)))
>>>>>                   return;
>>>>>
>>>>> -       r = amdgpu_fill_buffer(abo, AMDGPU_POISON, bo->base.resv, &fence, true);
>>>>> +       r = amdgpu_fill_buffer(abo, 0, bo->base.resv, &fence, true);
>>>>>           if (!WARN_ON(r)) {
>>>>> +               struct amdgpu_vram_mgr_resource *vres;
>>>>> +
>>>>> +               vres = to_amdgpu_vram_mgr_resource(bo->resource);
>>>>> +               vres->flags |= DRM_BUDDY_CLEARED;
>>>>>                   amdgpu_bo_fence(abo, fence, false);
>>>>>                   dma_fence_put(fence);
>>>>>           }
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_res_cursor.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_res_cursor.h
>>>>> index 381101d2bf05..50fcd86e1033 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_res_cursor.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_res_cursor.h
>>>>> @@ -164,4 +164,29 @@ static inline void amdgpu_res_next(struct amdgpu_res_cursor *cur, uint64_t size)
>>>>>           }
>>>>>    }
>>>>>
>>>>> +/**
>>>>> + * amdgpu_res_cleared - check if blocks are cleared
>>>>> + *
>>>>> + * @cur: the cursor to extract the block
>>>>> + *
>>>>> + * Check if the @cur block is cleared
>>>>> + */
>>>>> +static inline bool amdgpu_res_cleared(struct amdgpu_res_cursor *cur)
>>>>> +{
>>>>> +       struct drm_buddy_block *block;
>>>>> +
>>>>> +       switch (cur->mem_type) {
>>>>> +       case TTM_PL_VRAM:
>>>>> +               block = cur->node;
>>>>> +
>>>>> +               if (!amdgpu_vram_mgr_is_cleared(block))
>>>>> +                       return false;
>>>>> +               break;
>>>>> +       default:
>>>>> +               return false;
>>>>> +       }
>>>>> +
>>>>> +       return true;
>>>>> +}
>>>>> +
>>>>>    #endif
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>> index 8722beba494e..bcbffe909b47 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>>>>> @@ -378,11 +378,15 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo,
>>>>>               (abo->flags & AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE)) {
>>>>>                   struct dma_fence *wipe_fence = NULL;
>>>>>
>>>>> -               r = amdgpu_fill_buffer(abo, AMDGPU_POISON, NULL, &wipe_fence,
>>>>> -                                       false);
>>>>> +               r = amdgpu_fill_buffer(abo, 0, NULL, &wipe_fence,
>>>>> +                                      false);
>>>>>                   if (r) {
>>>>>                           goto error;
>>>>>                   } else if (wipe_fence) {
>>>>> +                       struct amdgpu_vram_mgr_resource *vres;
>>>>> +
>>>>> +                       vres = to_amdgpu_vram_mgr_resource(bo->resource);
>>>>> +                       vres->flags |= DRM_BUDDY_CLEARED;
>>>>>                           dma_fence_put(fence);
>>>>>                           fence = wipe_fence;
>>>>>                   }
>>>>> @@ -2214,6 +2218,59 @@ static int amdgpu_ttm_fill_mem(struct amdgpu_ring *ring, uint32_t src_data,
>>>>>           return 0;
>>>>>    }
>>>>>
>>>>> +int amdgpu_ttm_clear_buffer(struct amdgpu_bo *bo,
>>>>> +                           struct dma_resv *resv,
>>>>> +                           struct dma_fence **fence)
>>>>> +{
>>>>> +       struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>>>>> +       struct amdgpu_ring *ring = adev->mman.buffer_funcs_ring;
>>>>> +       struct amdgpu_res_cursor cursor;
>>>>> +       struct dma_fence *f = NULL;
>>>>> +       u64 addr;
>>>>> +       int r;
>>>>> +
>>>>> +       if (!adev->mman.buffer_funcs_enabled)
>>>>> +               return -EINVAL;
>>>>> +
>>>>> +       amdgpu_res_first(bo->tbo.resource, 0, amdgpu_bo_size(bo), &cursor);
>>>>> +
>>>>> +       mutex_lock(&adev->mman.gtt_window_lock);
>>>>> +       while (cursor.remaining) {
>>>>> +               struct dma_fence *next = NULL;
>>>>> +               u64 size;
>>>>> +
>>>>> +               if (amdgpu_res_cleared(&cursor)) {
>>>>> +                       amdgpu_res_next(&cursor, cursor.size);
>>>>> +                       continue;
>>>>> +               }
>>>>> +
>>>>> +               /* Never clear more than 256MiB at once to avoid timeouts */
>>>>> +               size = min(cursor.size, 256ULL << 20);
>>>>> +
>>>>> +               r = amdgpu_ttm_map_buffer(&bo->tbo, bo->tbo.resource, &cursor,
>>>>> +                                         1, ring, false, &size, &addr);
>>>>> +               if (r)
>>>>> +                       goto err;
>>>>> +
>>>>> +               r = amdgpu_ttm_fill_mem(ring, 0, addr, size, resv,
>>>>> +                                       &next, true, true);
>>>>> +               if (r)
>>>>> +                       goto err;
>>>>> +
>>>>> +               dma_fence_put(f);
>>>>> +               f = next;
>>>>> +
>>>>> +               amdgpu_res_next(&cursor, size);
>>>>> +       }
>>>>> +err:
>>>>> +       mutex_unlock(&adev->mman.gtt_window_lock);
>>>>> +       if (fence)
>>>>> +               *fence = dma_fence_get(f);
>>>>> +       dma_fence_put(f);
>>>>> +
>>>>> +       return r;
>>>>> +}
>>>>> +
>>>>>    int amdgpu_fill_buffer(struct amdgpu_bo *bo,
>>>>>                           uint32_t src_data,
>>>>>                           struct dma_resv *resv,
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>>>> index 65ec82141a8e..b404d89d52e5 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>>>>> @@ -38,8 +38,6 @@
>>>>>    #define AMDGPU_GTT_MAX_TRANSFER_SIZE   512
>>>>>    #define AMDGPU_GTT_NUM_TRANSFER_WINDOWS        2
>>>>>
>>>>> -#define AMDGPU_POISON  0xd0bed0be
>>>>> -
>>>>>    extern const struct attribute_group amdgpu_vram_mgr_attr_group;
>>>>>    extern const struct attribute_group amdgpu_gtt_mgr_attr_group;
>>>>>
>>>>> @@ -155,6 +153,9 @@ int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
>>>>>                                  uint64_t size, bool tmz,
>>>>>                                  struct dma_resv *resv,
>>>>>                                  struct dma_fence **f);
>>>>> +int amdgpu_ttm_clear_buffer(struct amdgpu_bo *bo,
>>>>> +                           struct dma_resv *resv,
>>>>> +                           struct dma_fence **fence);
>>>>>    int amdgpu_fill_buffer(struct amdgpu_bo *bo,
>>>>>                           uint32_t src_data,
>>>>>                           struct dma_resv *resv,
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>>>> index c0c851409241..e494f5bf136a 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
>>>>> @@ -450,6 +450,7 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
>>>>>    {
>>>>>           struct amdgpu_vram_mgr *mgr = to_vram_mgr(man);
>>>>>           struct amdgpu_device *adev = to_amdgpu_device(mgr);
>>>>> +       struct amdgpu_bo *bo = ttm_to_amdgpu_bo(tbo);
>>>>>           u64 vis_usage = 0, max_bytes, min_block_size;
>>>>>           struct amdgpu_vram_mgr_resource *vres;
>>>>>           u64 size, remaining_size, lpfn, fpfn;
>>>>> @@ -501,6 +502,9 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man,
>>>>>           if (place->flags & TTM_PL_FLAG_CONTIGUOUS)
>>>>>                   vres->flags |= DRM_BUDDY_CONTIGUOUS_ALLOCATION;
>>>>>
>>>>> +       if (bo->flags & AMDGPU_GEM_CREATE_VRAM_CLEARED)
>>>>> +               vres->flags |= DRM_BUDDY_CLEAR_ALLOCATION;
>>>> Is there any reason to not always do this?
>>> Here we are trying to keep a pool of cleared memory which are cleared on
>>> free path and that can given
>>> to the application which requires a zeroed memory. I think here if we
>>> set always clear the memory,
>>> we would go over the limit of cleared memory in that particular instance
>>> and the application should wait until
>>> the hardware clears the memory and this might impact the overall
>>> performance.
>> I'd like to have the driver always deliver cleared memory.
> Actually, I think we can just always set the flag in the allocation IOCTLs.
Sure, we can set the flag in the allocation IOCTLs.
Thanks,
Arun.
>
> Alex
>
>> Alex
>>
>>> Thanks,
>>> Arun.
>>>> Alex
>>>>
>>>>
>>>>> +
>>>>>           if (fpfn || lpfn != mgr->mm.size)
>>>>>                   /* Allocate blocks in desired range */
>>>>>                   vres->flags |= DRM_BUDDY_RANGE_ALLOCATION;
>>>>> @@ -604,7 +608,7 @@ static void amdgpu_vram_mgr_del(struct ttm_resource_manager *man,
>>>>>
>>>>>           amdgpu_vram_mgr_do_reserve(man);
>>>>>
>>>>> -       drm_buddy_free_list(mm, &vres->blocks, 0);
>>>>> +       drm_buddy_free_list(mm, &vres->blocks, vres->flags);
>>>>>           mutex_unlock(&mgr->lock);
>>>>>
>>>>>           atomic64_sub(vis_usage, &mgr->vis_usage);
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
>>>>> index 0e04e42cf809..8478522d7366 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h
>>>>> @@ -53,6 +53,11 @@ static inline u64 amdgpu_vram_mgr_block_size(struct drm_buddy_block *block)
>>>>>           return (u64)PAGE_SIZE << drm_buddy_block_order(block);
>>>>>    }
>>>>>
>>>>> +static inline bool amdgpu_vram_mgr_is_cleared(struct drm_buddy_block *block)
>>>>> +{
>>>>> +       return drm_buddy_block_is_clear(block);
>>>>> +}
>>>>> +
>>>>>    static inline struct amdgpu_vram_mgr_resource *
>>>>>    to_amdgpu_vram_mgr_resource(struct ttm_resource *res)
>>>>>    {
>>>>> --
>>>>> 2.25.1
>>>>>