Re: [PATCH 3/3] drm/amdgpu: Fix multi-level page table bugs for large BOs

["Zhang, Jerry (Junwei)" <Jerry.Zhang-5C7GfCeVMHo@public.gmane.org>]

On 03/29/2017 09:00 AM, Felix Kuehling wrote:
> Fix the start/end address calculation for address ranges that span
> multiple page directories in amdgpu_vm_alloc_levels.
>
> Add WARN_ONs if page tables aren't found. Otherwise the page table
> update would just fail silently.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 818747f..44aa039 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -278,6 +278,8 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>   	from = (saddr >> shift) % amdgpu_vm_num_entries(adev, level);
>   	to = (eaddr >> shift) % amdgpu_vm_num_entries(adev, level);
>
> +	WARN_ON(from > to);
> +
>   	if (to > parent->last_entry_used)
>   		parent->last_entry_used = to;
>
> @@ -312,8 +314,11 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>   		}
>
>   		if (level < adev->vm_manager.num_level) {
> -			r = amdgpu_vm_alloc_levels(adev, vm, entry, saddr,
> -						   eaddr, level);
> +			uint64_t sub_saddr = (pt_idx == from) ? saddr : 0;
> +			uint64_t sub_eaddr = (pt_idx == to) ? eaddr :
> +				((1 << shift) - 1);
> +			r = amdgpu_vm_alloc_levels(adev, vm, entry, sub_saddr,
> +						   sub_eaddr, level);

Do you mean to create a full sub-pt if pt_idx != from and != to?
I didn't catch it up clear.

In my perspective, we may need to clear the saddr and eaddr upper level bit 
before going on to allocate the next level PT.
Otherwise, the number of next PT entry will be incorrect, more than num_entries 
as the upper level PT number calculated in.

e.g.
there is a address contains
PD + PT1 + PT2 + PT3

for the first round, we could get correct "from" and "to" address with right 
"shift"(3 blocks), then walk over the address space in PD range.

Then for the 2nd round, we cannot get the expected "from" and "to" address with 
"shift"(2 blocks), as it walks over the address space in PD + PT1 range.
While we'd like it's in range PT1 indeed.

The fault way goes on with later round.

Perhaps, that's the root cause about the issue you face.

Jerry.

>   			if (r)
>   				return r;
>   		}
> @@ -957,9 +962,11 @@ static struct amdgpu_bo *amdgpu_vm_get_pt(struct amdgpu_pte_update_params *p,
>   		entry = &entry->entries[idx];
>   	}
>
> -	if (level)
> +	if (WARN_ON(level))
>   		return NULL;
>
> +	WARN_ON(!entry->bo);
> +
>   	return entry->bo;
>   }
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx