Re: [PATCH 1/2] drm/amdgpu: change page_table_base_addr caculation in mes queue property

[Christian König <christian.koenig@xxxxxxx>]

Am 22.08.23 um 08:17 schrieb Yifan Zhang:
> current method doesn't work for GTT domain page table, change
> it to support both VRAM and GTT domain.
>
> Signed-off-by: Yifan Zhang <yifan1.zhang@xxxxxxx>

Of hand that looks like the right thing to do, one comment below.

With that fixed feel free to add my Acked-by, but Shashank should 
probably take a look as well.

Thanks,
Christian.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c | 6 ++----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h | 2 +-
>   2 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
> index 10ce5557bb11..ee957f059786 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
> @@ -259,7 +259,7 @@ int amdgpu_mes_create_process(struct amdgpu_device *adev, int pasid,
>   	process->vm = vm;
>   	process->pasid = pasid;
>   	process->process_quantum = adev->mes.default_process_quantum;
> -	process->pd_gpu_addr = amdgpu_bo_gpu_offset(vm->root.bo);
> +	process->pd_phys_addr = amdgpu_gmc_pd_addr(vm->root.bo);
>   
>   	amdgpu_mes_unlock(&adev->mes);
>   	return 0;
> @@ -621,9 +621,7 @@ int amdgpu_mes_add_hw_queue(struct amdgpu_device *adev, int gang_id,
>   	/* add hw queue to mes */
>   	queue_input.process_id = gang->process->pasid;
>   
> -	queue_input.page_table_base_addr =
> -		adev->vm_manager.vram_base_offset + gang->process->pd_gpu_addr -
> -		adev->gmc.vram_start;
> +	queue_input.page_table_base_addr =gang->process->pd_phys_addr;
>   
>   	queue_input.process_va_start = 0;
>   	queue_input.process_va_end =
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h
> index a27b424ffe00..e1c20e2453c7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h
> @@ -140,7 +140,7 @@ struct amdgpu_mes {
>   struct amdgpu_mes_process {
>   	int			pasid;
>   	struct			amdgpu_vm *vm;
> -	uint64_t		pd_gpu_addr;
> +	uint64_t		pd_phys_addr;

phys_addr for physical address is quite a bit misleading since this 
isn't a physical address at all. It's actually using the PDE format.

The entry in the job structure is equally bad named because of 
historical reasons.

Maybe a name like root_pde or something similar would be better.

Regards,
Christian.


>   	struct amdgpu_bo 	*proc_ctx_bo;
>   	uint64_t 		proc_ctx_gpu_addr;
>   	void 			*proc_ctx_cpu_ptr;