Re: [PATCH 2/2] drm/amdgpu: adjust vmhub flush tlb iteration to fit the new GFXHUB/MMHUB layout

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

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);