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

[Luben Tuikov]

On 2022-12-13 02:00, 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 at amd.com>
>>>> Cc: Christian König <christian.koenig at amd.com>
>>>> Signed-off-by: Luben Tuikov <luben.tuikov at 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.

By "should probably just ignore it" do you mean "return true;" or "return false;"?

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

I agree. We can do this in another patch.

Regards,
Luben

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