[PATCH] drm/amdgpu: fix max_entries calculation v2

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

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

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