Re: [PATCH] drm/amd: Simplify the size check funciton

["Christian König" <christian.koenig@amd.com>]

Am 25.08.23 um 07:22 schrieb Ma Jun:
> Simplify the code logic of size check function amdgpu_bo_validate_size
>
> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 28 +++++++++-------------
>   1 file changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 807ea74ece25..4c95db954a76 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -480,7 +480,7 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
>   		*cpu_addr = NULL;
>   }
>   
> -/* Validate bo size is bit bigger then the request domain */
> +/* Validate bo size is bit bigger than the request domain */
>   static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
>   					  unsigned long size, u32 domain)
>   {
> @@ -490,29 +490,23 @@ static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
>   	 * If GTT is part of requested domains the check must succeed to
>   	 * allow fall back to GTT.
>   	 */
> -	if (domain & AMDGPU_GEM_DOMAIN_GTT) {
> +	if (domain & AMDGPU_GEM_DOMAIN_GTT)
>   		man = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT);
> -
> -		if (man && size < man->size)
> -			return true;
> -		else if (!man)
> -			WARN_ON_ONCE("GTT domain requested but GTT mem manager uninitialized");
> -		goto fail;
> -	} else if (domain & AMDGPU_GEM_DOMAIN_VRAM) {
> +	else if (domain & AMDGPU_GEM_DOMAIN_VRAM)
>   		man = ttm_manager_type(&adev->mman.bdev, TTM_PL_VRAM);
> +	else
> +		return true;
>   
> -		if (man && size < man->size)
> -			return true;
> -		goto fail;
> +	if (!man) {
> +		WARN_ON_ONCE("Mem mananger of mem domain %d is uninitialized", domain);
> +		return false;
>   	}

That change here is not correct. It's perfectly valid for userspace to 
request VRAM even if VRAM isn't initialized.

Only the GTT manager is mandatory. That's why the code previously looked 
like it does.

regards,
Christian.

>   
>   	/* TODO add more domains checks, such as AMDGPU_GEM_DOMAIN_CPU, _DOMAIN_DOORBELL */
> -	return true;
> +	if (size < man->size)
> +		return true;
>   
> -fail:
> -	if (man)
> -		DRM_DEBUG("BO size %lu > total memory in domain: %llu\n", size,
> -			  man->size);
> +	DRM_DEBUG("BO size %lu > total memory in domain: %llu\n", size, man->size);
>   	return false;
>   }
>