Re: [PATCH v2] drm/amdgpu: Fix size validation for non-exclusive domains

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 13.12.22 um 12:40 schrieb Lazar, Lijo:
> On 12/13/2022 12:30 PM, Christian König wrote:
>> Am 13.12.22 um 00:44 schrieb Luben Tuikov:
>>> On 2022-12-12 14:19, Christian König wrote:
>>>> Am 12.12.22 um 18:48 schrieb Luben Tuikov:
>>>>> Fix amdgpu_bo_validate_size() to check whether the TTM domain 
>>>>> manager for the
>>>>> requested memory exists, and to allow for non-exclusive domain 
>>>>> allocations, as
>>>>> there would be if the domain is a mask, e.g. AMDGPU_GEM_DOMAIN_VRAM |
>>>>> AMDGPU_GEM_DOMAIN_GTT.
>>>>>
>>>>> Cc: Alex Deucher <Alexander.Deucher@amd.com>
>>>>> Cc: Christian König <christian.koenig@amd.com>
>>>>> Signed-off-by: Luben Tuikov <luben.tuikov@amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 19 
>>>>> +++++++------------
>>>>>    1 file changed, 7 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> index fd3ab4b5e5bb1f..e0f103f0ec2178 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> @@ -448,31 +448,26 @@ 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
>>>>> +     * allow fall back to GTT.
>>>>> +     *
>>>>> +     * Note that allocations can request from either domain. For
>>>>> +     * this reason, check either in non-exclusive way, and if
>>>>> +     * neither satisfies, fail the validation.
>>>> That's not correct, the original logic was completely intentional.
>>>>
>>>> If both VRAM and GTT are specified it's valid if the size fits only 
>>>> into
>>>> GTT.
>>> Given that this patch fixes a kernel oops, should this patch then 
>>> fail the validation,
>>> i.e. return false?
>>
>> It should be sufficient if a BO fits into the GTT domain for size 
>> validation. If we haven't initialized the GTT domain and end up here 
>> we should probably just ignore it.
>>
>>>
>>> This would then fail, in amdgpu_ttm_reserve_tmr():
>>>
>>>     ret = amdgpu_bo_create_kernel_at(adev,
>>>                 adev->gmc.real_vram_size - 
>>> adev->mman.discovery_tmr_size,
>>>                 adev->mman.discovery_tmr_size,
>>>                 AMDGPU_GEM_DOMAIN_VRAM |
>>>                 AMDGPU_GEM_DOMAIN_GTT,
>>
>> As I said before using amdgpu_bo_create_kernel_at() with VRAM|GTT 
>> doesn't make any sense at all. We should probably drop the domain 
>> parameter altogether.
>>
>
> What is the alternative planned to prevent usage of VRAM at fixed 
> offsets?
>
> BTW, AMDGPU_GEM_DOMAIN_GTT for above doesn't make any sense. Discovery 
> region is always in VRAM domain.


Well that was my point, reserved regions are always in VRAM.

We probably don't need to ability to reserve in any other domain so we 
can drop the domain parameter here and just always assume that we need VRAM.

Regards,
Christian.

>
> Thanks,
> Lijo
>
>> Regards,
>> Christian.
>>
>>> &adev->mman.discovery_memory,
>>>                 NULL);
>>>
>>> Regards,
>>> Luben
>>>
>>