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 <[email protected]>
>> ---
>> 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;
>> }
>>
>
