On 8/27/26 17:48, Sunil Khatri wrote:
> amdgpu_vm_bo_lookup_mapping() expected callers to pre-shift the
> address to a page frame number before calling in, unlike its sibling
> functions amdgpu_vm_bo_map() and amdgpu_vm_bo_replace_map(), which
> accept a raw address and shift internally. This inconsistency meant
> every caller had to duplicate the same shift to make it pfn and many
> place the shift is not AMDGPU_GPU_PAGE_SHIFT but normal PAGE_SHIFT too.
>
> Move the shift inside amdgpu_vm_bo_lookup_mapping() and update all
> callers to stop pre-shifting, so the function's calling convention
> matches its siblings.
>
> Signed-off-by: Sunil Khatri <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 --
> drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c | 8 +++-----
> drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 9 +++------
> drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 1 +
> drivers/gpu/drm/amd/amdgpu/mes_userqueue.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 2 +-
> drivers/gpu/drm/amd/amdkfd/kfd_queue.c | 10 +++++-----
> 8 files changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index e129ec46441e..87ccef153073 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1809,8 +1809,6 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser
> *parser,
> struct amdgpu_bo_va_mapping *mapping;
> int i, r;
>
> - addr /= AMDGPU_GPU_PAGE_SIZE;
> -
> mapping = amdgpu_vm_bo_lookup_mapping(vm, addr);
> if (!mapping || !mapping->bo_va || !mapping->bo_va->base.bo)
> return -EINVAL;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c
> index 87e15e39eb30..76771ad30c41 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c
> @@ -256,10 +256,9 @@ amdgpu_devcoredump_print_ibs(struct drm_printer *p,
> goto unlock;
>
> for (int i = 0; i < coredump->num_ibs; i++) {
> - u64 pfn = (coredump->ibs[i].gpu_addr &
> - AMDGPU_GMC_HOLE_MASK) / AMDGPU_GPU_PAGE_SIZE;
> + u64 addr = coredump->ibs[i].gpu_addr &
> AMDGPU_GMC_HOLE_MASK;
>
> - mapping = amdgpu_vm_bo_lookup_mapping(vm, pfn);
> + mapping = amdgpu_vm_bo_lookup_mapping(vm, addr);
> if (!mapping)
> continue;
>
> @@ -280,8 +279,7 @@ amdgpu_devcoredump_print_ibs(struct drm_printer *p,
> continue;
>
> va_start = coredump->ibs[i].gpu_addr & AMDGPU_GMC_HOLE_MASK;
> - mapping = amdgpu_vm_bo_lookup_mapping(vm,
> - va_start /
> AMDGPU_GPU_PAGE_SIZE);
> + mapping = amdgpu_vm_bo_lookup_mapping(vm, va_start);
> if (!mapping)
> goto output_ib_content;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 0a816b3c5ff9..62bf6b78a534 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -241,7 +241,6 @@ int amdgpu_userq_input_va_validate(struct amdgpu_device
> *adev,
> struct amdgpu_vm *vm = queue->vm;
> u64 start_addr;
> u64 end_addr;
> - u64 start_page;
>
> /* Caller must hold vm->root.bo reservation */
> dma_resv_assert_held(queue->vm->root.bo->tbo.base.resv);
> @@ -253,16 +252,14 @@ int amdgpu_userq_input_va_validate(struct amdgpu_device
> *adev,
> if (check_add_overflow(start_addr, expected_size - 1, &end_addr))
> return -EINVAL;
>
> - start_page = start_addr >> AMDGPU_GPU_PAGE_SHIFT;
> -
> - va_map = amdgpu_vm_bo_lookup_mapping(vm, start_page);
> + va_map = amdgpu_vm_bo_lookup_mapping(vm, start_addr);
> if (!va_map)
> return -EINVAL;
>
> - /* Lookup guarantees start_page is mapped; ensure full span is covered.
> */
> + /* Lookup guarantees start_addr is mapped; ensure full span is covered.
> */
> if ((end_addr >> AMDGPU_GPU_PAGE_SHIFT) <= va_map->last) {
> va_map->bo_va->userq_va_mapped = true;
> - *va_out = start_page;
> + *va_out = start_addr;
What is va_out used for? Cause that is now an address instead of a pfn.
Apart from that it looks good to me.
Regards,
Christian.
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> index cee5b0241196..5b936d12c55b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> @@ -385,7 +385,7 @@ static int amdgpu_userq_fence_read_wptr(struct
> amdgpu_device *adev,
> if (unlikely(ret))
> goto lock_error;
>
> - mapping = amdgpu_vm_bo_lookup_mapping(queue->vm, addr >>
> PAGE_SHIFT);
> + mapping = amdgpu_vm_bo_lookup_mapping(queue->vm, addr);
> if (!mapping) {
> ret = -EINVAL;
> goto lock_error;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 88249aa89ee3..c8d1f2624b1b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2165,6 +2165,7 @@ int amdgpu_vm_bo_clear_mappings(struct amdgpu_device
> *adev,
> struct amdgpu_bo_va_mapping *amdgpu_vm_bo_lookup_mapping(struct amdgpu_vm
> *vm,
> uint64_t addr)
> {
> + addr /= AMDGPU_GPU_PAGE_SIZE;
> return amdgpu_vm_it_iter_first(&vm->va, addr, addr);
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> index 7f334f718cd8..46ebc002548d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> @@ -53,7 +53,7 @@ mes_userq_create_wptr_mapping(struct amdgpu_device *adev,
> if (unlikely(ret))
> goto fail_lock;
>
> - wptr_mapping = amdgpu_vm_bo_lookup_mapping(vm, wptr >>
> PAGE_SHIFT);
> + wptr_mapping = amdgpu_vm_bo_lookup_mapping(vm, wptr);
> if (!wptr_mapping) {
> ret = -EINVAL;
> goto fail_lock;
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> index 69976c8be034..72fd3022b606 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> @@ -2064,7 +2064,7 @@ static int vcn_v1_0_validate_bo(struct amdgpu_cs_parser
> *parser,
> return -EINVAL;
> }
>
> - mapping = amdgpu_vm_bo_lookup_mapping(vm, addr/AMDGPU_GPU_PAGE_SIZE);
> + mapping = amdgpu_vm_bo_lookup_mapping(vm, addr);
> if (!mapping || !mapping->bo_va || !mapping->bo_va->base.bo)
> return -EINVAL;
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_queue.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_queue.c
> index 15eeaaebbbcd..ae3e7c54014a 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_queue.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_queue.c
> @@ -197,18 +197,18 @@ int kfd_queue_buffer_get(struct amdgpu_vm *vm, void
> __user *addr, struct amdgpu_
> u64 expected_size)
> {
> struct amdgpu_bo_va_mapping *mapping;
> - u64 user_addr;
> + u64 user_pfn;
> u64 size;
>
> - user_addr = (u64)addr >> AMDGPU_GPU_PAGE_SHIFT;
> size = expected_size >> AMDGPU_GPU_PAGE_SHIFT;
>
> - mapping = amdgpu_vm_bo_lookup_mapping(vm, user_addr);
> + mapping = amdgpu_vm_bo_lookup_mapping(vm, (u64)(uintptr_t)addr);
> if (!mapping)
> goto out_err;
>
> - if (user_addr != mapping->start ||
> - (size != 0 && user_addr + size - 1 != mapping->last)) {
> + user_pfn = (u64)(uintptr_t)addr >> AMDGPU_GPU_PAGE_SHIFT;
> + if (user_pfn != mapping->start ||
> + (size != 0 && user_pfn + size - 1 != mapping->last)) {
> pr_debug("expected size 0x%llx not equal to mapping addr 0x%llx
> size 0x%llx\n",
> expected_size, mapping->start << AMDGPU_GPU_PAGE_SHIFT,
> (mapping->last - mapping->start + 1) <<
> AMDGPU_GPU_PAGE_SHIFT);
