[PATCH] drm/amdgpu: fix max_entries calculation v2

[Christian König]

Am 02.09.20 um 16:53 schrieb Pan, Xinhui:
>
>> 2020年9月2日 22:31，Christian König <ckoenig.leichtzumerken at gmail.com> 写道：
>>
>> Am 02.09.20 um 16:27 schrieb Pan, Xinhui:
>>>> 2020年9月2日 22:05，Christian König <ckoenig.leichtzumerken at gmail.com> 写道：
>>>>
>>>> Calculate the correct value for max_entries or we might run after the
>>>> page_address array.
>>>>
>>>> v2: Xinhui pointed out we don't need the shift
>>>>
>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>> Fixes: 1e691e244487 drm/amdgpu: stop allocating dummy GTT nodes
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 8bc2253939be..be886bdca5c6 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -1697,7 +1697,7 @@ static int amdgpu_vm_bo_split_mapping(struct amdgpu_device *adev,
>>>> 				AMDGPU_GPU_PAGES_IN_CPU_PAGE;
>>>> 		} else {
>>>> 			addr = 0;
>>>> -			max_entries = S64_MAX;
>>>> +			max_entries = mapping->last - mapping->start + 1;
>>> You need minus pfn here.
>> That doesn't sound correct either. The pfn is the destination of the mapping, e.g. the offset inside the BO and not related to the virtual address range we map.
> I mean we need minus pfn too. pfn is mapping->offset >> PAGE_SHIFT.
>
> In amdgpu_vm_bo_map(), there is a check  below
> if (bo && offset + size > amdgpu_bo_size(bo))
> return -EINVAL;
> so mapping->offset is just an offset_in_bytes inside the BO as you said.

Correct, but this is the destination of the mapping and not the covered 
VA space.

In other words start can be 4, last be 5 and offset 64k and it would 
still be valid as long as the bo is at leat 64k+2 pages in size.

> mapping->start and mapping->last are virtual addresses in pfns, the range we are going to touch then is [start+ offset_in_pfns, last].

No, that is completely unrelated.

Christian.

>
>>> The range we are going to touch is [start + offset, last].
>>> so the max_entries is last - (start + offset) + 1. and offset is pfn in this case.
>>>
>>> I still hit panic with this patch in practice.
>> Thanks for testing, I think I know what the problem is.
>>
>> We need to start instead of mapping->start or otherwise the values is to large after the first iteration.
>>
>> Give me a second for a v3.
>>
>> Christian.
>>
>>>> 		}
>>>>
>>>> 		if (pages_addr) {
>>>> -- 
>>>> 2.17.1
>>>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx