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

["Ma, Jun" <majun@amd.com>]

Hi Christian,

On 8/25/2023 4:08 PM, Christian König wrote:
> 
> 
> 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.
> 
Thanks for your explanation.
How about changing the code to the following?

+       if (!man) {
+               if (domain & AMDGPU_GEM_DOMAIN_GTT)
+                       WARN_ON_ONCE("GTT domain requested but GTT mem manager uninitialized");
+               return false;
        }

Regards,
Ma Jun

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