[PATCH] drm/amdgpu: avoid building unused VM update fences

[PATCH] drm/amdgpu: avoid building unused VM update fences

[Bob Zhou]

amdgpu_gem_va_update_vm() returns a fence for every immediate VA
update, but the ioctl caller only consumes that fence when userspace
requests a VM timeline syncobj. Without a timeline syncobj, the helper
may take fence references or allocate a merged fence that is
immediately released.

Pass the timeline syncobj into the helper and skip fence construction
when no output fence is requested. Preserve the existing stub,
merged-fence and error behavior for callers that do request a timeline
update.

Signed-off-by: Bob Zhou <bobzhou2@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index f754a4a3a1c22..2837553e4f58a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -754,6 +754,7 @@ int amdgpu_gem_metadata_ioctl(struct drm_device *dev, void *data,
  * @vm: vm to update
  * @bo_va: bo_va to update
  * @operation: map, unmap or clear
+ * @timeline_syncobj: VM timeline syncobj the fence gets attached to, or NULL
  *
  * Update the bo_va directly after setting its address. Errors are not
  * vital here, so they are not reported back to userspace.
@@ -765,15 +766,15 @@ static struct dma_fence *
 amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
 			struct amdgpu_vm *vm,
 			struct amdgpu_bo_va *bo_va,
-			uint32_t operation)
+			uint32_t operation,
+			struct drm_syncobj *timeline_syncobj)
 {
 	struct dma_fence *fence;
 	int r = 0;
 
 	/* If the VM is not ready return only a stub. */
 	if (!amdgpu_vm_ready(vm))
-		return dma_fence_get_stub();
-
+		return timeline_syncobj ? dma_fence_get_stub() : NULL;
 
 	/*
 	 * First clean up any freed mappings in the VM.
@@ -799,6 +800,14 @@ amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
 	if (r)
 		goto error;
 
+	/*
+	 * The VM update work above is already committed. If the caller does
+	 * not need a fence (no VM timeline syncobj was requested) skip building
+	 * the otherwise-unused merged/last-update fence.
+	 */
+	if (!timeline_syncobj)
+		return NULL;
+
 	if ((operation == AMDGPU_VA_OP_MAP ||
 	     operation == AMDGPU_VA_OP_REPLACE) &&
 	    !amdgpu_vm_is_bo_always_valid(vm, bo_va->base.bo)) {
@@ -826,7 +835,7 @@ amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
 	if (r && r != -ERESTARTSYS)
 		DRM_ERROR("Couldn't update BO_VA (%d)\n", r);
 
-	return dma_fence_get(vm->last_update);
+	return timeline_syncobj ? dma_fence_get(vm->last_update) : NULL;
 }
 
 int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
@@ -996,7 +1005,8 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 		struct dma_fence *fence;
 
 		fence = amdgpu_gem_va_update_vm(adev, &fpriv->vm, bo_va,
-						args->operation);
+						args->operation,
+						timeline_syncobj);
 		if (timeline_syncobj) {
 			if (!args->vm_timeline_point) {
 				/* Replace the existing fence when no point is given. */
-- 
2.34.1

Re: [PATCH] drm/amdgpu: avoid building unused VM update fences

[Christian König]

On 8/10/26 11:18, Bob Zhou wrote:
> amdgpu_gem_va_update_vm() returns a fence for every immediate VA
> update, but the ioctl caller only consumes that fence when userspace
> requests a VM timeline syncobj. Without a timeline syncobj, the helper
> may take fence references or allocate a merged fence that is
> immediately released.
> 
> Pass the timeline syncobj into the helper and skip fence construction
> when no output fence is requested. Preserve the existing stub,
> merged-fence and error behavior for callers that do request a timeline
> update.

That just massively complicates the handling without any gain at all.

Why the heck should we do that?

Regards,
Christian.

> 
> Signed-off-by: Bob Zhou <bobzhou2@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index f754a4a3a1c22..2837553e4f58a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -754,6 +754,7 @@ int amdgpu_gem_metadata_ioctl(struct drm_device *dev, void *data,
>   * @vm: vm to update
>   * @bo_va: bo_va to update
>   * @operation: map, unmap or clear
> + * @timeline_syncobj: VM timeline syncobj the fence gets attached to, or NULL
>   *
>   * Update the bo_va directly after setting its address. Errors are not
>   * vital here, so they are not reported back to userspace.
> @@ -765,15 +766,15 @@ static struct dma_fence *
>  amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
>  			struct amdgpu_vm *vm,
>  			struct amdgpu_bo_va *bo_va,
> -			uint32_t operation)
> +			uint32_t operation,
> +			struct drm_syncobj *timeline_syncobj)
>  {
>  	struct dma_fence *fence;
>  	int r = 0;
>  
>  	/* If the VM is not ready return only a stub. */
>  	if (!amdgpu_vm_ready(vm))
> -		return dma_fence_get_stub();
> -
> +		return timeline_syncobj ? dma_fence_get_stub() : NULL;
>  
>  	/*
>  	 * First clean up any freed mappings in the VM.
> @@ -799,6 +800,14 @@ amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
>  	if (r)
>  		goto error;
>  
> +	/*
> +	 * The VM update work above is already committed. If the caller does
> +	 * not need a fence (no VM timeline syncobj was requested) skip building
> +	 * the otherwise-unused merged/last-update fence.
> +	 */
> +	if (!timeline_syncobj)
> +		return NULL;
> +
>  	if ((operation == AMDGPU_VA_OP_MAP ||
>  	     operation == AMDGPU_VA_OP_REPLACE) &&
>  	    !amdgpu_vm_is_bo_always_valid(vm, bo_va->base.bo)) {
> @@ -826,7 +835,7 @@ amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
>  	if (r && r != -ERESTARTSYS)
>  		DRM_ERROR("Couldn't update BO_VA (%d)\n", r);
>  
> -	return dma_fence_get(vm->last_update);
> +	return timeline_syncobj ? dma_fence_get(vm->last_update) : NULL;
>  }
>  
>  int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
> @@ -996,7 +1005,8 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>  		struct dma_fence *fence;
>  
>  		fence = amdgpu_gem_va_update_vm(adev, &fpriv->vm, bo_va,
> -						args->operation);
> +						args->operation,
> +						timeline_syncobj);
>  		if (timeline_syncobj) {
>  			if (!args->vm_timeline_point) {
>  				/* Replace the existing fence when no point is given. */