[PATCH v2] drm/amdgpu: Modify the contiguous flags behaviour

Christian König christian.koenig at amd.com
Wed Apr 17 06:49:09 UTC 2024


Am 17.04.24 um 08:21 schrieb Arunpravin Paneer Selvam:
> 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 <Arunpravin.PaneerSelvam at amd.com>
> Suggested-by: Christian König <christian.koenig at amd.com>
> ---
>   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);

Nice work, up till here that looks exactly right as far as I can see.

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

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.

> +	} 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;

And this here just optimizes it in the way that it says try harder to 
make it look contiguous.

Question is if that also works with pages_per_block of 2MiB or does that 
only kick in when pages_per_block is maximal?

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

Well I would say that this is a rather hacky implementation and should 
be avoided if possible.

> +
> +				pages_per_block =
> +					amdgpu_vram_mgr_calculate_pages_per_block(tbo,
> +										  place,
> +										  bo_flags);

Rather move the AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS handling out of 
amdgpu_vram_mgr_calculate_pages_per_block().

Regards,
Christian.

> +				continue;
> +			}
>   			goto error_free_blocks;
> +		}
>   
>   		if (size > remaining_size)
>   			remaining_size = 0;



More information about the amd-gfx mailing list