2023-05-20 18:25:40

by ghostfly233

Subject: [PATCH] drm/amdgpu: Rearrange WREG32 operations in gfxhub_v2_1.c

In gfxhub_v2_1_setup_vmid_config(), the GCVM_CONTEXT1_CNTL reg is
written before related GCVM_CONTEXT1_PAGE_TABLE_START_ADDR and
GCVM_CONTEXT1_PAGE_TABLE_END_ADDR regs are written, which may
cause undefined behavior.

This patch rearranges WREG32 operations in gfxhub_v2_1_setup_vmid_config(),
so that it can ensure the addresses are initialized before CNTL is enabled
and reduce the risk of encountering undefined behavior.

Signed-off-by: Zibin Liu <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c
index 4aacbbec31e2..6d094e7315eb 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c
@@ -321,8 +321,6 @@ static void gfxhub_v2_1_setup_vmid_config(struct amdgpu_device *adev)
tmp = REG_SET_FIELD(tmp, GCVM_CONTEXT1_CNTL,
RETRY_PERMISSION_OR_INVALID_PAGE_FAULT,
!adev->gmc.noretry);
- WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_CNTL,
- i * hub->ctx_distance, tmp);
WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32,
i * hub->ctx_addr_distance, 0);
WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32,
@@ -333,6 +331,8 @@ static void gfxhub_v2_1_setup_vmid_config(struct amdgpu_device *adev)
WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_PAGE_TABLE_END_ADDR_HI32,
i * hub->ctx_addr_distance,
upper_32_bits(adev->vm_manager.max_pfn - 1));
+ WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_CNTL,
+ i * hub->ctx_distance, tmp);
}

hub->vm_cntx_cntl = tmp;
--
2.34.1

2023-05-31 13:41:40

by Christian König

Subject: Re: [PATCH] drm/amdgpu: Rearrange WREG32 operations in gfxhub_v2_1.c

Am 20.05.23 um 13:59 schrieb ghostfly233:
> In gfxhub_v2_1_setup_vmid_config(), the GCVM_CONTEXT1_CNTL reg is
> written before related GCVM_CONTEXT1_PAGE_TABLE_START_ADDR and
> GCVM_CONTEXT1_PAGE_TABLE_END_ADDR regs are written, which may
> cause undefined behavior.

In which hw documentation did you found that?

>
> This patch rearranges WREG32 operations in gfxhub_v2_1_setup_vmid_config(),
> so that it can ensure the addresses are initialized before CNTL is enabled
> and reduce the risk of encountering undefined behavior.

I have absolutely no idea how you came to this conclusion, but as far as
I know this is just bluntly incorrect.

The control register must be written before the other parameters are set
or otherwise the hw wouldn't know what to do with those values and might
even block writes to the registers or hang.

Regards,
Christian.

>
> Signed-off-by: Zibin Liu <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c b/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c
> index 4aacbbec31e2..6d094e7315eb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfxhub_v2_1.c
> @@ -321,8 +321,6 @@ static void gfxhub_v2_1_setup_vmid_config(struct amdgpu_device *adev)
> tmp = REG_SET_FIELD(tmp, GCVM_CONTEXT1_CNTL,
> RETRY_PERMISSION_OR_INVALID_PAGE_FAULT,
> !adev->gmc.noretry);
> - WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_CNTL,
> - i * hub->ctx_distance, tmp);
> WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_PAGE_TABLE_START_ADDR_LO32,
> i * hub->ctx_addr_distance, 0);
> WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_PAGE_TABLE_START_ADDR_HI32,
> @@ -333,6 +331,8 @@ static void gfxhub_v2_1_setup_vmid_config(struct amdgpu_device *adev)
> WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_PAGE_TABLE_END_ADDR_HI32,
> i * hub->ctx_addr_distance,
> upper_32_bits(adev->vm_manager.max_pfn - 1));
> + WREG32_SOC15_OFFSET(GC, 0, mmGCVM_CONTEXT1_CNTL,
> + i * hub->ctx_distance, tmp);
> }
>
> hub->vm_cntx_cntl = tmp;