> static u64 gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, u64 addr)
> {
> - addr = adev->vm_manager.vram_base_offset + addr - adev->mc.vram_start;
> + addr = adev->vm_manager.vram_base_offset + addr;
Please be noted that mv.vram_start is not 0, it is equal to the value from mmMC_VM_FB_LOCATION_BASE, which is initialized by vbios. Typically is 0xf400000000, mc address. Thus any addr here should be based on 0xf400000000.
+ addr = adev->vm_manager.vram_base_offset + addr;
I don't think it is correct to add an absolute addr here. vram_base_offset is already the base physical address of vram (typically 0 for dGPU, while some non-zero value for APU), we need an relative address/offset to reflect the location of vram bo.
addr - adev->mc.vram_start just calculate the relative address of the vram bo, or the offset from vram start point.
Anyway, the base physical address (vram_base_offset) should plus an offset to reflect the real physical address of vram bo.
Regards,
Hawking
-----Original Message-----
From: Zhou, David(ChunMing)
Sent: Tuesday, May 16, 2017 10:45
To: Christian König <deathsimple at vodafone.de>; amd-gfx at lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang at amd.com>
Subject: Re: [PATCH 3/3] drm/amdgpu: fix another fundamental VM bug
On 2017å¹´05æ??15æ?¥ 19:57, Christian König wrote:
> From: Christian König <christian.koenig at amd.com>
>
> I always wondered why this code uses the MC address. Now it came to me
> that this is actually a bug and only works by coincident because we
> used to have VRAM mapped at zero.
adding Hawking to confirm this.
Regards,
David Zhou
>
> Signed-off-by: Christian König <christian.koenig at amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index e4c5d31..ba6eb73 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -884,7 +884,7 @@ static int amdgpu_cs_ib_vm_chunk(struct amdgpu_device *adev,
> }
>
> if (p->job->vm) {
> - p->job->vm_pd_addr = amdgpu_bo_gpu_offset(vm->root.bo);
> + p->job->vm_pd_addr = vm->root.bo->tbo.mem.start << PAGE_SHIFT;
>
> r = amdgpu_bo_vm_update_pte(p);
> if (r)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index b877f9f3..7256fcc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1012,7 +1012,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
> return r;
> }
>
> - pt = amdgpu_bo_gpu_offset(bo);
> + pt = bo->tbo.mem.start << PAGE_SHIFT;
> if (parent->entries[pt_idx].addr == pt)
> continue;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 047b1a7..63cb573 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -360,7 +360,7 @@ static uint64_t gmc_v9_0_get_vm_pte_flags(struct
> amdgpu_device *adev,
>
> static u64 gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, u64 addr)
> {
> - addr = adev->vm_manager.vram_base_offset + addr - adev->mc.vram_start;
> + addr = adev->vm_manager.vram_base_offset + addr;
> BUG_ON(addr & 0xFFFF00000000003FULL);
> return addr;
> }
