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

[Ma, Jun]

On 8/28/2023 2:00 PM, Christian König wrote:
> Am 28.08.23 um 07:09 schrieb Ma, Jun:
>> 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 at 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;
>>          }
> 
> Of hand that looks like it should work, but I need to see the full patch.
> 
Thanks, I'll send v2 with this change.

Regards,
Ma Jun
> Regards,
> Christian.
> 
>>
>> 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;
>>>>    }
>>>>    
>