Re: [PATCH 1/3] drm/amdgpu: don't wait for the SPM VMID's jobs under the VMID manager lock

["Christian König" <christian.koenig@amd.com>]

On 10/20/26 10:37, Jesse Zhang wrote:
> amdgpu_vmid_free_spm() waits for the jobs of the reserved VMID while
> holding the VMID manager lock of the hub, which every VMID grab on the
> hub takes as well. Freeing the reservation, by UNRESERVE_VMID or when
> the VM is destroyed, so stalls the kernel queue submissions of every
> process on the GFXHUB until those jobs completed, or until the job
> timeout if one of them hangs. A full GPU reset even deadlocks, since
> amdgpu_vmid_reset_all() needs the lock before the scheduler is started
> again to signal the jobs.
> 
> On navi31, with a GFX job of the reserving process that waits for 1s,
> an unrelated process' SDMA NOP takes 0.8s instead of no time when the
> VMID gets unreserved meanwhile.
> 
> Nothing adds to the active fences of the VMID once it's no longer the
> VM's reserved one, so drop the lock while waiting for them. Keep the
> VMID reserved until then, so that nobody else can bind it before.
> 
> Fixes: 9b47649cabf4 ("drm/amdgpu: bind the SPM VMID's page table to the reserving VM")
> Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> index 1482fc031d44..d19bbcf25765 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> @@ -645,17 +645,18 @@ void amdgpu_vmid_free_spm(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  
>  	mutex_lock(&id_mgr->lock);
>  	id = vm->reserved_vmid[vmhub];
> -	if (id) {
> -		vm->reserved_vmid[vmhub] = NULL;
> -		id_mgr->reserved_vmid = false;
> +	vm->reserved_vmid[vmhub] = NULL;
> +	mutex_unlock(&id_mgr->lock);
> +	if (!id)
> +		return;
>  
> -		/* Only for kernel queues and kfd/userqueues by pass that */
> -		amdgpu_sync_wait(&id->active, false);
> +	amdgpu_sync_wait(&id->active, false);

Accessing id->active without holding the lock is a pretty big no-go as well.

We should probably rather move the amdgpu_sync_wait into the caller of the function and wait for vm->root.bo->tbo.base.resv instead.

That should have the same effect, but we don't need to hold the VMID lock for that.

Regards,
Christian.

>  
> -		if (adev->vm_manager.spm_vmid && adev->gfxhub.funcs)
> -			amdgpu_gmc_bind_vmid(adev, AMDGPU_GFXHUB(0),
> -					     adev->vm_manager.spm_vmid, 0);
> -	}
> +	mutex_lock(&id_mgr->lock);
> +	if (adev->vm_manager.spm_vmid && adev->gfxhub.funcs)
> +		amdgpu_gmc_bind_vmid(adev, AMDGPU_GFXHUB(0),
> +				     adev->vm_manager.spm_vmid, 0);
> +	id_mgr->reserved_vmid = false;
>  	mutex_unlock(&id_mgr->lock);
>  }
>