[PATCH v2 2/2] drm/amdgpu: Enable clear page functionality
Christian König
ckoenig.leichtzumerken at gmail.com
Mon Dec 11 14:41:11 UTC 2023
Am 11.12.23 um 08:31 schrieb Arunpravin Paneer Selvam:
> 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)
>
> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam at amd.com>
> Suggested-by: Christian König <christian.koenig at amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 16 +++---
> .../gpu/drm/amd/amdgpu/amdgpu_res_cursor.h | 25 +++++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 53 +++++++++++++++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 3 ++
> drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 15 +++++-
> drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.h | 5 ++
> 6 files changed, 109 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index cef920a93924..5a01b6266772 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
> @@ -598,8 +599,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 |
> @@ -629,15 +629,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);
> 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 05991c5c8ddb..252b384194c5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -2222,6 +2222,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)) {
That test here is now inverted. You need to remove the !
> + 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..cf77489a7adb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> @@ -155,6 +155,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 d0e199cc8f17..f3d36c345b63 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> @@ -435,6 +435,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;
> @@ -486,6 +487,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;
> +
> if (fpfn || lpfn != mgr->mm.size)
> /* Allocate blocks in desired range */
> vres->flags |= DRM_BUDDY_RANGE_ALLOCATION;
> @@ -579,7 +583,9 @@ static void amdgpu_vram_mgr_del(struct ttm_resource_manager *man,
> struct amdgpu_vram_mgr_resource *vres = to_amdgpu_vram_mgr_resource(res);
> 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(res->bo);
> struct drm_buddy *mm = &mgr->mm;
> + struct dma_fence *fence = NULL;
> struct drm_buddy_block *block;
> uint64_t vis_usage = 0;
>
> @@ -589,7 +595,14 @@ 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);
> + if (bo->flags & AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE) {
> + if (!amdgpu_fill_buffer(bo, 0, NULL, &fence, true)) {
> + vres->flags |= DRM_BUDDY_CLEARED;
> + dma_fence_put(fence);
> + }
> + }
Please completely drop that chunk.
AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE should already be handled elsewhere.
What you might want to do is to change the
AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE handling to set the
DRM_BUDDY_CLEARED flag.
That might be much cleaner and could potentially also handle the case
where we can't execute the AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE flag
because the engine isn't ready.
Regards,
Christian.
> +
> + 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)
> {
More information about the dri-devel
mailing list