<html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">On 2024-04-17 10:32, Paneer Selvam, Arunpravin wrote: </div> <blockquote type="cite" cite="mid:ea5d73f1-0bf9-41a1-95b7-6479ea4c6ad8@amd.com">Hi Christian, On 4/17/2024 6:57 PM, Paneer Selvam, Arunpravin wrote: <blockquote type="cite">Hi Christian, On 4/17/2024 12:19 PM, Christian König wrote: <blockquote type="cite">Am 17.04.24 um 08:21 schrieb Arunpravin Paneer Selvam: <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. When we set AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS - This means contiguous is not mandatory. - 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. v2: - keep the mem_flags and bo->flags check as is(Christian) - place the TTM_PL_FLAG_CONTIGUOUS flag setting into the amdgpu_bo_pin_restricted function placement range iteration loop(Christian) - rename find_pages with amdgpu_vram_mgr_calculate_pages_per_block (Christian) - Keep the kernel BO allocation as is(Christain) - If BO pin vram allocation failed, we need to return -ENOSPC as RDMA cannot work with scattered VRAM pages(Philip) 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 | 8 ++- drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 57 +++++++++++++++----- 2 files changed, 50 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index 8bc79924d171..caaef7b1df49 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -153,8 +153,10 @@ 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) + if (abo->tbo.type == ttm_bo_type_kernel && + flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS) places[c].flags |= TTM_PL_FLAG_CONTIGUOUS; + c++; } @@ -966,6 +968,10 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain, if (!bo->placements[i].lpfn || (lpfn && lpfn < bo->placements[i].lpfn)) bo->placements[i].lpfn = lpfn; + + if (bo->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS && + bo->placements[i].mem_type == TTM_PL_VRAM) + bo->placements[i].flags |= TTM_PL_FLAG_CONTIGUOUS; } r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); </blockquote> Nice work, up till here that looks exactly right as far as I can see. <blockquote type="cite">diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c index 8db880244324..4be8b091099a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c @@ -88,6 +88,29 @@ static inline u64 amdgpu_vram_mgr_blocks_size(struct list_head *head) return size; } +static inline unsigned long +amdgpu_vram_mgr_calculate_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) { + pages_per_block = ~0ul; </blockquote> If I understand it correctly this here enforces the allocation of a contiguous buffer in the way that it says we should have only one giant page for the whole BO. </blockquote> yes, for contiguous we don't have the 2MB limit, hence we are setting to maximum to avoid restricting the min_block_size to 2MB limitation. <blockquote type="cite"> <blockquote type="cite">+ } 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 +474,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 +493,9 @@ 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_mgr_calculate_pages_per_block(tbo, place, + bo_flags); vres = kzalloc(sizeof(*vres), GFP_KERNEL); if (!vres) @@ -498,7 +514,7 @@ 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) vres->flags |= DRM_BUDDY_CONTIGUOUS_ALLOCATION; </blockquote> And this here just optimizes it in the way that it says try harder to make it look contiguous. </blockquote> Here I removed the TTM_PL_FLAG_CONTIGUOUS. AFAIU, in both cases (BO pinning and normal contiguous request) this flag AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS is always set. <blockquote type="cite"> Question is if that also works with pages_per_block of 2MiB or does that only kick in when pages_per_block is maximal? </blockquote> AFAIU, if this flag AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS is set, we are setting the pages_per_block as maximal, thus we dont limit the BO. when we set the pages_per_block as maximal, the min_block_size would be the tbo->page_alignment, and this tbo->page_alignment would be the same value as the required size. Required size can be less than 2MB or more than 2MB. Below we have check size >= pages_per_block, when the pages_per_block is maximal we don't limit the min_block_size. </blockquote> a small correction, when we set the pages_per_block as maximal, we don't set the min_block_size, the buddy allocator will set the min_block_size as roundup_pow_of_two(size). </blockquote> If setting 2MB pages_per_block, without DRM_BUDDY_CONTIGUOUS_ALLOCATION flag, does buddy alloc from free 2MB blocks first, or buddy split larger blocks into smaller blocks, as we want to keep larger block for contiguous allocation. Regards, Philip <blockquote type="cite" cite="mid:ea5d73f1-0bf9-41a1-95b7-6479ea4c6ad8@amd.com"> Thanks, Arun. <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite"> if (fpfn || lpfn != mgr->mm.size) @@ -529,8 +545,21 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager *man, min_block_size, &vres->blocks, vres->flags); - if (unlikely(r)) + if (unlikely(r)) { + if (bo_flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS && + !(place->flags & TTM_PL_FLAG_CONTIGUOUS)) { + /* Fallback to non contiguous allocation */ + vres->flags &= ~DRM_BUDDY_CONTIGUOUS_ALLOCATION; + bo_flags &= ~AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS; </blockquote> Well I would say that this is a rather hacky implementation and should be avoided if possible. </blockquote> sure, I will try to rewrite the code. <blockquote type="cite"> <blockquote type="cite">+ + pages_per_block = + amdgpu_vram_mgr_calculate_pages_per_block(tbo, + place, + bo_flags); </blockquote> Rather move the AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS handling out of amdgpu_vram_mgr_calculate_pages_per_block(). </blockquote> sure. Thanks, Arun. <blockquote type="cite"> Regards, Christian. <blockquote type="cite">+ continue; + } goto error_free_blocks; + } if (size > remaining_size) remaining_size = 0; </blockquote> </blockquote> </blockquote> </blockquote> </body> </html>