* Re: [PATCH 2/2] drm/amdgpu: adjust vmhub flush tlb iteration to fit the new GFXHUB/MMHUB layout
2023-05-04 15:50 ` [PATCH 2/2] drm/amdgpu: adjust vmhub flush tlb iteration to fit the " Yifan Zhang
@ 2023-05-04 19:15 ` Alex Deucher
2023-05-05 8:10 ` Christian König
1 sibling, 0 replies; 4+ messages in thread
From: Alex Deucher @ 2023-05-04 19:15 UTC (permalink / raw)
To: Yifan Zhang
Cc: Alexander.Deucher, le.ma, christian.koenig, amd-gfx,
Hawking.Zhang
Maybe add:
Fixes: dc267018dac1 ("drm/amdgpu: introduce vmhub definition for
multi-partition cases (v3)")
to the series. With that, the series is:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Alex
On Thu, May 4, 2023 at 11:51 AM Yifan Zhang <yifan1.zhang@amd.com> wrote:
>
> tlb flush has to be changed for the new mmhub layout
>
> Signed-off-by: Yifan Zhang <yifan1.zhang@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 16 ++++++++++++----
> drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c | 10 +++++++---
> 2 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> index 01cb89ffbd56..2383db399c95 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> @@ -160,6 +160,7 @@ void amdgpu_gart_unbind(struct amdgpu_device *adev, uint64_t offset,
> /* Starting from VEGA10, system bit must be 0 to mean invalid. */
> uint64_t flags = 0;
> int idx;
> + struct amdgpu_vmhub *hub;
>
> if (!adev->gart.ptr)
> return;
> @@ -182,8 +183,11 @@ void amdgpu_gart_unbind(struct amdgpu_device *adev, uint64_t offset,
> }
> mb();
> amdgpu_device_flush_hdp(adev, NULL);
> - for (i = 0; i < adev->num_vmhubs; i++)
> - amdgpu_gmc_flush_gpu_tlb(adev, 0, i, 0);
> + for (i = 0; i < AMDGPU_MAX_VMHUBS; i++) {
> + hub = &adev->vmhub[i];
> + if (hub->vmhub_funcs)
> + amdgpu_gmc_flush_gpu_tlb(adev, 0, i, 0);
> + }
>
> drm_dev_exit(idx);
> }
> @@ -258,14 +262,18 @@ void amdgpu_gart_bind(struct amdgpu_device *adev, uint64_t offset,
> void amdgpu_gart_invalidate_tlb(struct amdgpu_device *adev)
> {
> int i;
> + struct amdgpu_vmhub *hub;
>
> if (!adev->gart.ptr)
> return;
>
> mb();
> amdgpu_device_flush_hdp(adev, NULL);
> - for (i = 0; i < adev->num_vmhubs; i++)
> - amdgpu_gmc_flush_gpu_tlb(adev, 0, i, 0);
> + for (i = 0; i < AMDGPU_MAX_VMHUBS; i++) {
> + hub = &adev->vmhub[i];
> + if (hub->vmhub_funcs)
> + amdgpu_gmc_flush_gpu_tlb(adev, 0, i, 0);
> + }
> }
>
> /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> index 90cf79f8ddde..3ee7f5e067fb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> @@ -331,6 +331,7 @@ static int gmc_v11_0_flush_gpu_tlb_pasid(struct amdgpu_device *adev,
> bool ret;
> struct amdgpu_ring *ring = &adev->gfx.kiq[0].ring;
> struct amdgpu_kiq *kiq = &adev->gfx.kiq[0];
> + struct amdgpu_vmhub *hub;
>
> if (amdgpu_emu_mode == 0 && ring->sched.ready) {
> spin_lock(&adev->gfx.kiq[0].ring_lock);
> @@ -362,9 +363,12 @@ static int gmc_v11_0_flush_gpu_tlb_pasid(struct amdgpu_device *adev,
> &queried_pasid);
> if (ret && queried_pasid == pasid) {
> if (all_hub) {
> - for (i = 0; i < adev->num_vmhubs; i++)
> - gmc_v11_0_flush_gpu_tlb(adev, vmid,
> - i, flush_type);
> + for (i = 0; i < AMDGPU_MAX_VMHUBS; i++) {
> + hub = &adev->vmhub[i];
> + if (hub->vmhub_funcs)
> + gmc_v11_0_flush_gpu_tlb(adev, vmid,
> + i, flush_type);
> + }
> } else {
> gmc_v11_0_flush_gpu_tlb(adev, vmid,
> AMDGPU_GFXHUB(0), flush_type);
> --
> 2.37.3
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH 2/2] drm/amdgpu: adjust vmhub flush tlb iteration to fit the new GFXHUB/MMHUB layout
2023-05-04 15:50 ` [PATCH 2/2] drm/amdgpu: adjust vmhub flush tlb iteration to fit the " Yifan Zhang
2023-05-04 19:15 ` Alex Deucher
@ 2023-05-05 8:10 ` Christian König
1 sibling, 0 replies; 4+ messages in thread
From: Christian König @ 2023-05-05 8:10 UTC (permalink / raw)
To: Yifan Zhang, amd-gfx
Cc: Alexander.Deucher, le.ma, christian.koenig, Hawking.Zhang
Am 04.05.23 um 17:50 schrieb Yifan Zhang:
> tlb flush has to be changed for the new mmhub layout
>
> Signed-off-by: Yifan Zhang <yifan1.zhang@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 16 ++++++++++++----
> drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c | 10 +++++++---
> 2 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> index 01cb89ffbd56..2383db399c95 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> @@ -160,6 +160,7 @@ void amdgpu_gart_unbind(struct amdgpu_device *adev, uint64_t offset,
> /* Starting from VEGA10, system bit must be 0 to mean invalid. */
> uint64_t flags = 0;
> int idx;
> + struct amdgpu_vmhub *hub;
Please always define variables as local as possible. E.g. in this case
not in the function but rather in the loop.
Apart from that it's good practice to keep defines like i, r, idx etc..
last. Some upstream maintainer even enforce reverse xmas tree ordering.
Regards,
Christian.
>
> if (!adev->gart.ptr)
> return;
> @@ -182,8 +183,11 @@ void amdgpu_gart_unbind(struct amdgpu_device *adev, uint64_t offset,
> }
> mb();
> amdgpu_device_flush_hdp(adev, NULL);
> - for (i = 0; i < adev->num_vmhubs; i++)
> - amdgpu_gmc_flush_gpu_tlb(adev, 0, i, 0);
> + for (i = 0; i < AMDGPU_MAX_VMHUBS; i++) {
> + hub = &adev->vmhub[i];
> + if (hub->vmhub_funcs)
> + amdgpu_gmc_flush_gpu_tlb(adev, 0, i, 0);
> + }
>
> drm_dev_exit(idx);
> }
> @@ -258,14 +262,18 @@ void amdgpu_gart_bind(struct amdgpu_device *adev, uint64_t offset,
> void amdgpu_gart_invalidate_tlb(struct amdgpu_device *adev)
> {
> int i;
> + struct amdgpu_vmhub *hub;
>
> if (!adev->gart.ptr)
> return;
>
> mb();
> amdgpu_device_flush_hdp(adev, NULL);
> - for (i = 0; i < adev->num_vmhubs; i++)
> - amdgpu_gmc_flush_gpu_tlb(adev, 0, i, 0);
> + for (i = 0; i < AMDGPU_MAX_VMHUBS; i++) {
> + hub = &adev->vmhub[i];
> + if (hub->vmhub_funcs)
> + amdgpu_gmc_flush_gpu_tlb(adev, 0, i, 0);
> + }
> }
>
> /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> index 90cf79f8ddde..3ee7f5e067fb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> @@ -331,6 +331,7 @@ static int gmc_v11_0_flush_gpu_tlb_pasid(struct amdgpu_device *adev,
> bool ret;
> struct amdgpu_ring *ring = &adev->gfx.kiq[0].ring;
> struct amdgpu_kiq *kiq = &adev->gfx.kiq[0];
> + struct amdgpu_vmhub *hub;
>
> if (amdgpu_emu_mode == 0 && ring->sched.ready) {
> spin_lock(&adev->gfx.kiq[0].ring_lock);
> @@ -362,9 +363,12 @@ static int gmc_v11_0_flush_gpu_tlb_pasid(struct amdgpu_device *adev,
> &queried_pasid);
> if (ret && queried_pasid == pasid) {
> if (all_hub) {
> - for (i = 0; i < adev->num_vmhubs; i++)
> - gmc_v11_0_flush_gpu_tlb(adev, vmid,
> - i, flush_type);
> + for (i = 0; i < AMDGPU_MAX_VMHUBS; i++) {
> + hub = &adev->vmhub[i];
> + if (hub->vmhub_funcs)
> + gmc_v11_0_flush_gpu_tlb(adev, vmid,
> + i, flush_type);
> + }
> } else {
> gmc_v11_0_flush_gpu_tlb(adev, vmid,
> AMDGPU_GFXHUB(0), flush_type);
^ permalink raw reply [flat|nested] 4+ messages in thread