[PATCH] drm/amdgpu: Don't clear PT after process killed

[PATCH] drm/amdgpu: Don't clear PT after process killed

[Philip Yang]

Move amdgpu_vm_ready check to inside amdgpu_vm_clear_freed, this removes
the duplicate code, also removes the error message "*ERROR* Trying to
push to a killed entity" when KFD release wq unmap_bo_from_gpuvm to
unmap outstanding BOs if using SDMA update page table.

Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 3 ---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 9 ++-------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 2 ++
 3 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index ecdfe6cb36cc..6e1a5b922eb1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1116,9 +1116,6 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 		}
 	}
 
-	if (!amdgpu_vm_ready(vm))
-		return -EINVAL;
-
 	r = amdgpu_vm_clear_freed(adev, vm, NULL);
 	if (r)
 		return r;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index ce073e894584..f6c297d62cfe 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -373,11 +373,9 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
 
 	amdgpu_vm_bo_del(adev, bo_va);
 	amdgpu_vm_bo_update_shared(bo);
-	if (!amdgpu_vm_ready(vm))
-		goto out_unlock;
 
 	r = amdgpu_vm_clear_freed(adev, vm, &fence);
-	if (unlikely(r < 0))
+	if (unlikely(r < 0 && r != -EINVAL))
 		dev_err(adev->dev, "failed to clear page "
 			"tables on GEM object close (%ld)\n", r);
 	if (r || !fence)
@@ -387,7 +385,7 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
 	dma_fence_put(fence);
 
 out_unlock:
-	if (r)
+	if (r && r != -EINVAL)
 		dev_err(adev->dev, "leaking bo va (%ld)\n", r);
 	drm_exec_fini(&exec);
 }
@@ -766,9 +764,6 @@ amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
 	struct dma_fence *fence = dma_fence_get_stub();
 	int r;
 
-	if (!amdgpu_vm_ready(vm))
-		return fence;
-
 	r = amdgpu_vm_clear_freed(adev, vm, &fence);
 	if (r)
 		goto error;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index db66b4232de0..febdd1b4286c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1543,6 +1543,8 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
 	struct amdgpu_sync sync;
 	int r;
 
+	if (!amdgpu_vm_ready(vm))
+		return -EINVAL;
 
 	/*
 	 * Implicitly sync to command submissions in the same VM before
-- 
2.49.0

Re: [PATCH] drm/amdgpu: Don't clear PT after process killed

[Christian König]

On 10/30/25 20:01, Philip Yang wrote:
> Move amdgpu_vm_ready check to inside amdgpu_vm_clear_freed, this removes
> the duplicate code, also removes the error message "*ERROR* Trying to
> push to a killed entity" when KFD release wq unmap_bo_from_gpuvm to
> unmap outstanding BOs if using SDMA update page table.

In general good idea to have that fixed, but the ready check should stay outside of amdgpu_vm_clear_freed().

Background is that there are more operations than only clearling the freed covered by this check.

Regards,
Christian.

> 
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 3 ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 9 ++-------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 2 ++
>  3 files changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index ecdfe6cb36cc..6e1a5b922eb1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1116,9 +1116,6 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>  		}
>  	}
>  
> -	if (!amdgpu_vm_ready(vm))
> -		return -EINVAL;
> -
>  	r = amdgpu_vm_clear_freed(adev, vm, NULL);
>  	if (r)
>  		return r;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index ce073e894584..f6c297d62cfe 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -373,11 +373,9 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
>  
>  	amdgpu_vm_bo_del(adev, bo_va);
>  	amdgpu_vm_bo_update_shared(bo);
> -	if (!amdgpu_vm_ready(vm))
> -		goto out_unlock;
>  
>  	r = amdgpu_vm_clear_freed(adev, vm, &fence);
> -	if (unlikely(r < 0))
> +	if (unlikely(r < 0 && r != -EINVAL))
>  		dev_err(adev->dev, "failed to clear page "
>  			"tables on GEM object close (%ld)\n", r);
>  	if (r || !fence)
> @@ -387,7 +385,7 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
>  	dma_fence_put(fence);
>  
>  out_unlock:
> -	if (r)
> +	if (r && r != -EINVAL)
>  		dev_err(adev->dev, "leaking bo va (%ld)\n", r);
>  	drm_exec_fini(&exec);
>  }
> @@ -766,9 +764,6 @@ amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
>  	struct dma_fence *fence = dma_fence_get_stub();
>  	int r;
>  
> -	if (!amdgpu_vm_ready(vm))
> -		return fence;
> -
>  	r = amdgpu_vm_clear_freed(adev, vm, &fence);
>  	if (r)
>  		goto error;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index db66b4232de0..febdd1b4286c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1543,6 +1543,8 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>  	struct amdgpu_sync sync;
>  	int r;
>  
> +	if (!amdgpu_vm_ready(vm))
> +		return -EINVAL;
>  
>  	/*
>  	 * Implicitly sync to command submissions in the same VM before

Re: [PATCH] drm/amdgpu: Don't clear PT after process killed

[Philip Yang]

On 2025-10-31 03:42, Christian König wrote:
> On 10/30/25 20:01, Philip Yang wrote:
>> Move amdgpu_vm_ready check to inside amdgpu_vm_clear_freed, this removes
>> the duplicate code, also removes the error message "*ERROR* Trying to
>> push to a killed entity" when KFD release wq unmap_bo_from_gpuvm to
>> unmap outstanding BOs if using SDMA update page table.
> In general good idea to have that fixed, but the ready check should stay outside of amdgpu_vm_clear_freed().
>
> Background is that there are more operations than only clearling the freed covered by this check.

ok, then I will add amdgpu_vm_ready check inside unmap_bo_from_gpuvm to 
remove the ERROR message, leave the possible cleanup in another patch as 
it is tricky to handle the error path, amdgpu_dma_buf_move_notify also 
calls amdgpu_vm_clear_freed without checking vm ready.

Thanks,

Philip

>
> Regards,
> Christian.
>
>> Suggested-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 3 ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 9 ++-------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 2 ++
>>   3 files changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index ecdfe6cb36cc..6e1a5b922eb1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -1116,9 +1116,6 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>>   		}
>>   	}
>>   
>> -	if (!amdgpu_vm_ready(vm))
>> -		return -EINVAL;
>> -
>>   	r = amdgpu_vm_clear_freed(adev, vm, NULL);
>>   	if (r)
>>   		return r;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index ce073e894584..f6c297d62cfe 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -373,11 +373,9 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
>>   
>>   	amdgpu_vm_bo_del(adev, bo_va);
>>   	amdgpu_vm_bo_update_shared(bo);
>> -	if (!amdgpu_vm_ready(vm))
>> -		goto out_unlock;
>>   
>>   	r = amdgpu_vm_clear_freed(adev, vm, &fence);
>> -	if (unlikely(r < 0))
>> +	if (unlikely(r < 0 && r != -EINVAL))
>>   		dev_err(adev->dev, "failed to clear page "
>>   			"tables on GEM object close (%ld)\n", r);
>>   	if (r || !fence)
>> @@ -387,7 +385,7 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
>>   	dma_fence_put(fence);
>>   
>>   out_unlock:
>> -	if (r)
>> +	if (r && r != -EINVAL)
>>   		dev_err(adev->dev, "leaking bo va (%ld)\n", r);
>>   	drm_exec_fini(&exec);
>>   }
>> @@ -766,9 +764,6 @@ amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
>>   	struct dma_fence *fence = dma_fence_get_stub();
>>   	int r;
>>   
>> -	if (!amdgpu_vm_ready(vm))
>> -		return fence;
>> -
>>   	r = amdgpu_vm_clear_freed(adev, vm, &fence);
>>   	if (r)
>>   		goto error;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index db66b4232de0..febdd1b4286c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1543,6 +1543,8 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>>   	struct amdgpu_sync sync;
>>   	int r;
>>   
>> +	if (!amdgpu_vm_ready(vm))
>> +		return -EINVAL;
>>   
>>   	/*
>>   	 * Implicitly sync to command submissions in the same VM before