[PATCH 2/2] drm/amdgpu: Optimize VM invalidation engine allocation and synchronize GPU TLB flush

[Christian König]

Am 19.02.25 um 07:20 schrieb jesse.zhang at amd.com:
> From: "Jesse.zhang at amd.com" <jesse.zhang at amd.com>
>
> - Modify the VM invalidation engine allocation logic to handle SDMA page rings.
>   SDMA page rings now share the VM invalidation engine with SDMA gfx rings instead of
>   allocating a separate engine. This change ensures efficient resource management and
>   avoids the issue of insufficient VM invalidation engines.
>
> - Add synchronization for GPU TLB flush operations in gmc_v9_0.c.
>   Use spin_lock and spin_unlock to ensure thread safety and prevent race conditions
>   during TLB flush operations. This improves the stability and reliability of the driver,
>   especially in multi-threaded environments.
>
> Signed-off-by: Jesse Zhang <jesse.zhang at amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c | 9 +++++++++
>  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c   | 2 ++
>  2 files changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index cb914ce82eb5..013d31f2794b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -601,8 +601,17 @@ int amdgpu_gmc_allocate_vm_inv_eng(struct amdgpu_device *adev)
>  			return -EINVAL;
>  		}
>  
> +	if (ring->funcs->type == AMDGPU_RING_TYPE_SDMA &&
> +	    adev->sdma.has_page_queue &&
> +	    (strncmp(ring->name, "sdma", 4) == 0)) {

Clear NAK to that. *Never ever* use strncmp for a technical decision inside the kernel.

Lijo's suggestion sound much cleaner to me.

> +		/* Do not allocate a separate VM invalidation engine for SDMA page rings.
> +		 * Shared VM invalid engine with sdma gfx ring.
> +		 */
> +		ring->vm_inv_eng = inv_eng - 1;
> +	} else {
>  		ring->vm_inv_eng = inv_eng - 1;
>  		vm_inv_engs[vmhub] &= ~(1 << ring->vm_inv_eng);
> +	}
>  
>  		dev_info(adev->dev, "ring %s uses VM inv eng %u on hub %u\n",
>  			 ring->name, ring->vm_inv_eng, ring->vm_hub);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 2aa87fdf715f..2599da8677da 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -1000,6 +1000,7 @@ static uint64_t gmc_v9_0_emit_flush_gpu_tlb(struct amdgpu_ring *ring,
>  	 * to WA the Issue
>  	 */
>  
> +	spin_lock(&adev->gmc.invalidate_lock);

That makes no sense at all. The commands are written into the ring and executed asynchronously after the lock is already dropped again.

Please completely drop that.

Regards,
Christian.

>  	/* TODO: It needs to continue working on debugging with semaphore for GFXHUB as well. */
>  	if (use_semaphore)
>  		/* a read return value of 1 means semaphore acuqire */
> @@ -1030,6 +1031,7 @@ static uint64_t gmc_v9_0_emit_flush_gpu_tlb(struct amdgpu_ring *ring,
>  		amdgpu_ring_emit_wreg(ring, hub->vm_inv_eng0_sem +
>  				      hub->eng_distance * eng, 0);
>  
> +	spin_unlock(&adev->gmc.invalidate_lock);
>  	return pd_addr;
>  }
>