Re: [PATCH v1 1/4] drm/amdgpu: make amdgpu_vm_bo_lookup_mapping() take a raw address

["Christian König" <christian.koenig@amd.com>]

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 <sunil.khatri@amd.com>
> ---
>  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);