<html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">On 2024-04-16 02:50, Paneer Selvam, Arunpravin wrote: </div> <blockquote type="cite" cite="mid:a8fd7808-a4ae-4e1e-8aae-be22bd4fd3b1@amd.com"> On 4/16/2024 3:32 AM, Philip Yang wrote: <blockquote type="cite"> On 2024-04-14 10:57, Arunpravin Paneer Selvam wrote: <blockquote type="cite">Now we have two flags for contiguous VRAM buffer allocation. If the application request for AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS, it would set the ttm place TTM_PL_FLAG_CONTIGUOUS flag in the buffer's placement function. This patch will change the default behaviour of the two flags. </blockquote> This change will simplify the KFD best effort contiguous VRAM allocation, because KFD doesn't need set new GEM_ flag. <blockquote type="cite">When we set AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS - This means contiguous is not mandatory. </blockquote> AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS used in couple of places. For page table BO, it is fine as BO size is page size 4K. For 64KB reserved BOs and F/W size related BOs, do all allocation happen at driver initialization before the VRAM is fragmented? <blockquote type="cite">- we will try to allocate the contiguous buffer. Say if the allocation fails, we fallback to allocate the individual pages. When we setTTM_PL_FLAG_CONTIGUOUS - This means contiguous allocation is mandatory. - we are setting this in amdgpu_bo_pin_restricted() before bo validation and check this flag in the vram manager file. - if this is set, we should allocate the buffer pages contiguously. the allocation fails, we return -ENOSPC. Signed-off-by: Arunpravin Paneer Selvam<a class="moz-txt-link-rfc2396E" href="mailto:Arunpravin.PaneerSelvam@amd.com"><Arunpravin.PaneerSelvam@amd.com></a> Suggested-by: Christian König<a class="moz-txt-link-rfc2396E" href="mailto:christian.koenig@amd.com"><christian.koenig@amd.com></a> --- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 14 +++-- drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 57 +++++++++++++++----- 2 files changed, 49 insertions(+), 22 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index 8bc79924d171..41926d631563 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -153,8 +153,6 @@ void amdgpu_bo_placement_from_domain(struct amdgpu_bo *abo, u32 domain) else places[c].flags |= TTM_PL_FLAG_TOPDOWN; - if (flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS) - places[c].flags |= TTM_PL_FLAG_CONTIGUOUS; c++; } @@ -899,6 +897,8 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain, { struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); struct ttm_operation_ctx ctx = { false, false }; + struct ttm_place *places = bo->placements; + u32 c = 0; int r, i; if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm)) @@ -921,16 +921,10 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain, if (bo->tbo.pin_count) { uint32_t mem_type = bo->tbo.resource->mem_type; - uint32_t mem_flags = bo->tbo.resource->placement; if (!(domain & amdgpu_mem_type_to_domain(mem_type))) return -EINVAL; - if ((mem_type == TTM_PL_VRAM) && - (bo->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS) && - !(mem_flags & TTM_PL_FLAG_CONTIGUOUS)) - return -EINVAL; - </blockquote> This looks like a bug before, but with this patch, the check makes sense and is needed. <blockquote type="cite"> ttm_bo_pin(&bo->tbo); if (max_offset != 0) { @@ -968,6 +962,10 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain, bo->placements[i].lpfn = lpfn; } + if (domain & AMDGPU_GEM_DOMAIN_VRAM && + !WARN_ON(places[c].mem_type != TTM_PL_VRAM)) + places[c].flags |= TTM_PL_FLAG_CONTIGUOUS; + </blockquote> If BO pinned is not allocated with AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS, should pin and return scattered pages because the RDMA support scattered dmabuf. Christian also pointed this out. If (bo->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS && bo->placements[i].mem_type == TTM_PL_VRAM) o->placements[i].flags |= TTM_PL_FLAG_CONTIGUOUS; <blockquote type="cite"> r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); if (unlikely(r)) { dev_err(adev->dev, "%p pin failed\n", bo); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c index 8db880244324..ddbf302878f6 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c @@ -88,6 +88,30 @@ static inline u64 amdgpu_vram_mgr_blocks_size(struct list_head *head) return size; } +static inline unsigned long +amdgpu_vram_find_pages_per_block(struct ttm_buffer_object *tbo, + const struct ttm_place *place, + unsigned long bo_flags) +{ + unsigned long pages_per_block; + + if (bo_flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS || + place->flags & TTM_PL_FLAG_CONTIGUOUS) { + pages_per_block = ~0ul; + } else { +#ifdef CONFIG_TRANSPARENT_HUGEPAGE + pages_per_block = HPAGE_PMD_NR; +#else + /* default to 2MB */ + pages_per_block = 2UL << (20UL - PAGE_SHIFT); +#endif + pages_per_block = max_t(uint32_t, pages_per_block, + tbo->page_alignment); + } + + return pages_per_block; +} + /** * DOC: mem_info_vram_total * @@ -451,8 +475,10 @@ 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); u64 vis_usage = 0, max_bytes, min_block_size; + struct amdgpu_bo *bo = ttm_to_amdgpu_bo(tbo); struct amdgpu_vram_mgr_resource *vres; u64 size, remaining_size, lpfn, fpfn; + unsigned long bo_flags = bo->flags; struct drm_buddy *mm = &mgr->mm; struct drm_buddy_block *block; unsigned long pages_per_block; @@ -468,18 +494,8 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man, if (tbo->type != ttm_bo_type_kernel) max_bytes -= AMDGPU_VM_RESERVED_VRAM; - if (place->flags & TTM_PL_FLAG_CONTIGUOUS) { - pages_per_block = ~0ul; - } else { -#ifdef CONFIG_TRANSPARENT_HUGEPAGE - pages_per_block = HPAGE_PMD_NR; -#else - /* default to 2MB */ - pages_per_block = 2UL << (20UL - PAGE_SHIFT); -#endif - pages_per_block = max_t(uint32_t, pages_per_block, - tbo->page_alignment); - } + pages_per_block = + amdgpu_vram_find_pages_per_block(tbo, place, bo_flags); vres = kzalloc(sizeof(*vres), GFP_KERNEL); if (!vres) @@ -498,7 +514,8 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man, if (place->flags & TTM_PL_FLAG_TOPDOWN) vres->flags |= DRM_BUDDY_TOPDOWN_ALLOCATION; - if (place->flags & TTM_PL_FLAG_CONTIGUOUS) + if (bo_flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS || + place->flags & TTM_PL_FLAG_CONTIGUOUS) vres->flags |= DRM_BUDDY_CONTIGUOUS_ALLOCATION; if (fpfn || lpfn != mgr->mm.size) @@ -529,8 +546,20 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man, min_block_size, &vres->blocks, vres->flags); - if (unlikely(r)) + if (unlikely(r)) { </blockquote> If pin BO failed to allocate contiguous VRAM, this should return failure to application, as RDMA cannot work with scattered VRAM pages. </blockquote> here we can return -ENOSPC when the BO is pinned (i.e TTM_PL_FLAG_CONTIGUOUS is set). Please find the below modified condition, if (bo_flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS && !(place->flags & TTM_PL_FLAG_CONTIGUOUS) </blockquote> Yes, this change looks good for me. Thanks Philip <blockquote type="cite" cite="mid:a8fd7808-a4ae-4e1e-8aae-be22bd4fd3b1@amd.com">Hence if the TTM_PL_FLAG_CONTIGUOUS flag is set, we don't proceed for allocating individual pages. <blockquote type="cite"> Regards, Philip <blockquote type="cite">+ if (bo_flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS) { + /* Fallback to non contiguous allocation */ + vres->flags &= ~DRM_BUDDY_CONTIGUOUS_ALLOCATION; + bo_flags &= ~AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS; + + pages_per_block = + amdgpu_vram_find_pages_per_block(tbo, + place, + bo_flags); + continue; + } goto error_free_blocks; + } if (size > remaining_size) remaining_size = 0; </blockquote> </blockquote> </blockquote> </body> </html>