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

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

[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@amd.com>
---
 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;
 	struct amdgpu_bo 	*proc_ctx_bo;
 	uint64_t 		proc_ctx_gpu_addr;
 	void 			*proc_ctx_cpu_ptr;
-- 
2.37.3

[PATCH 2/2] drm/amdgpu: Put page tables to GTT memory for APUs.

[Yifan Zhang]

To decrease VRAM pressure for APUs, put page tables to
GTT domain.

Signed-off-by: Yifan Zhang <yifan1.zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
index 96d601e209b8..26063e611c49 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
@@ -515,7 +515,7 @@ int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	bp.size = amdgpu_vm_pt_size(adev, level);
 	bp.byte_align = AMDGPU_GPU_PAGE_SIZE;
 
-	if (!adev->gmc.is_app_apu)
+	if (!(adev->gmc.is_app_apu || (adev->flags & AMD_IS_APU)))
 		bp.domain = AMDGPU_GEM_DOMAIN_VRAM;
 	else
 		bp.domain = AMDGPU_GEM_DOMAIN_GTT;
-- 
2.37.3

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

[Christian König]

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@amd.com>

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;

Re: [PATCH 2/2] drm/amdgpu: Put page tables to GTT memory for APUs.

[Christian König]

Am 22.08.23 um 08:17 schrieb Yifan Zhang:
> To decrease VRAM pressure for APUs, put page tables to
> GTT domain.
>
> Signed-off-by: Yifan Zhang <yifan1.zhang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> index 96d601e209b8..26063e611c49 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> @@ -515,7 +515,7 @@ int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   	bp.size = amdgpu_vm_pt_size(adev, level);
>   	bp.byte_align = AMDGPU_GPU_PAGE_SIZE;
>   
> -	if (!adev->gmc.is_app_apu)
> +	if (!(adev->gmc.is_app_apu || (adev->flags & AMD_IS_APU)))

I've tried this before and it doesn't work in quite a bunch of 
configurations. Especially IOMMU has big problems with that.

Alex do you know which hw generation finally supported that without 
problems?

Regards,
Christian.

>   		bp.domain = AMDGPU_GEM_DOMAIN_VRAM;
>   	else
>   		bp.domain = AMDGPU_GEM_DOMAIN_GTT;

Re: [PATCH 2/2] drm/amdgpu: Put page tables to GTT memory for APUs.

[Alex Deucher]

On Tue, Aug 22, 2023 at 8:00 AM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 22.08.23 um 08:17 schrieb Yifan Zhang:
> > To decrease VRAM pressure for APUs, put page tables to
> > GTT domain.
> >
> > Signed-off-by: Yifan Zhang <yifan1.zhang@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> > index 96d601e209b8..26063e611c49 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> > @@ -515,7 +515,7 @@ int amdgpu_vm_pt_create(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> >       bp.size = amdgpu_vm_pt_size(adev, level);
> >       bp.byte_align = AMDGPU_GPU_PAGE_SIZE;
> >
> > -     if (!adev->gmc.is_app_apu)
> > +     if (!(adev->gmc.is_app_apu || (adev->flags & AMD_IS_APU)))
>
> I've tried this before and it doesn't work in quite a bunch of
> configurations. Especially IOMMU has big problems with that.
>
> Alex do you know which hw generation finally supported that without
> problems?

We ran into issues on raven boards IIRC.  It's probably safe to enable
on Renoir and newer APUs since those should have all been validated
with windows + IOMMU.

Alex

>
> Regards,
> Christian.
>
> >               bp.domain = AMDGPU_GEM_DOMAIN_VRAM;
> >       else
> >               bp.domain = AMDGPU_GEM_DOMAIN_GTT;
>