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

["Lazar, Lijo" <lijo.lazar@amd.com>]

On 12/13/2022 5:22 PM, Christian König wrote:
> 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.
> 

Got it. Thanks!

Thanks,
Lijo

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