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

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

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@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.

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