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