[PATCH] drm/amdgpu: Refactor amdgpu_gem_va_ioctl for Handling Last Fence Update and Timeline Management

[Christian König]

On 4/24/25 05:21, Srinivasan Shanmugam wrote:
> This commit simplifies the amdgpu_gem_va_ioctl function, key updates
> include:
>  - Moved the logic for managing the last update fence directly into
>    amdgpu_gem_va_update_vm.
>  - Introduced checks for the timeline point to enable conditional
>    replacement or addition of fences.
> 
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Christian König <christian.koenig at amd.com>
> Suggested-by: Christian König <christian.koenig at amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam at amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 96 +++++++++++--------------
>  1 file changed, 41 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index f03fc3cf4d50..c83947c0269b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -112,47 +112,6 @@ amdgpu_gem_update_timeline_node(struct drm_file *filp,
>  	return 0;
>  }
>  
> -static void
> -amdgpu_gem_update_bo_mapping(struct drm_file *filp,
> -			     struct amdgpu_bo_va *bo_va,
> -			     uint32_t operation,
> -			     uint64_t point,
> -			     struct dma_fence *fence,
> -			     struct drm_syncobj *syncobj,
> -			     struct dma_fence_chain *chain)
> -{
> -	struct amdgpu_bo *bo = bo_va ? bo_va->base.bo : NULL;
> -	struct amdgpu_fpriv *fpriv = filp->driver_priv;
> -	struct amdgpu_vm *vm = &fpriv->vm;
> -	struct dma_fence *last_update;
> -
> -	if (!syncobj)
> -		return;
> -
> -	/* Find the last update fence */
> -	switch (operation) {
> -	case AMDGPU_VA_OP_MAP:
> -	case AMDGPU_VA_OP_REPLACE:
> -		if (bo && (bo->tbo.base.resv == vm->root.bo->tbo.base.resv))
> -			last_update = vm->last_update;
> -		else
> -			last_update = bo_va->last_pt_update;
> -		break;
> -	case AMDGPU_VA_OP_UNMAP:
> -	case AMDGPU_VA_OP_CLEAR:
> -		last_update = fence;
> -		break;
> -	default:
> -		return;
> -	}
> -
> -	/* Add fence to timeline */
> -	if (!point)
> -		drm_syncobj_replace_fence(syncobj, last_update);
> -	else
> -		drm_syncobj_add_point(syncobj, chain, last_update, point);
> -}
> -
>  static vm_fault_t amdgpu_gem_fault(struct vm_fault *vmf)
>  {
>  	struct ttm_buffer_object *bo = vmf->vma->vm_private_data;
> @@ -751,6 +710,8 @@ 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
> + * @last_update: a pointer to a pointer where the last update fence will
> + * be stored, reflecting the most recent mapping or update operation.
>   *
>   * Update the bo_va directly after setting its address. Errors are not
>   * vital here, so they are not reported back to userspace.
> @@ -762,9 +723,11 @@ 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)
> +			u32 operation,
> +			struct dma_fence **last_update)

That is duplicated to the return value.

>  {
>  	struct dma_fence *fence = dma_fence_get_stub();
> +	struct amdgpu_bo *bo = bo_va ? bo_va->base.bo : NULL;

I think you don't need that line any more. But see below.

>  	int r;
>  
>  	if (!amdgpu_vm_ready(vm))
> @@ -774,11 +737,22 @@ amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
>  	if (r)
>  		goto error;
>  
> -	if (operation == AMDGPU_VA_OP_MAP ||
> -	    operation == AMDGPU_VA_OP_REPLACE) {
> +	if (operation == AMDGPU_VA_OP_MAP || operation == AMDGPU_VA_OP_REPLACE) {
>  		r = amdgpu_vm_bo_update(adev, bo_va, false);
>  		if (r)
>  			goto error;
> +
> +		/* Set the last_update fence based on the operation */
> +		if (bo && bo->tbo.base.resv == vm->root.bo->tbo.base.resv)

Please use "if (amdgpu_vm_is_bo_always_valid(vm, bo_va->bo))" here.


> +			/* Use VM's last update fence */
> +			*last_update = vm->last_update;
> +		else
> +			/* Use buffer object's last update fence */
> +			*last_update = bo_va->last_pt_update;

Just update fence here instead. That is the return value of the function anyway.

> +
> +	} else if (operation == AMDGPU_VA_OP_UNMAP || operation == AMDGPU_VA_OP_CLEAR) {
> +		/* Assigning the temporary fence for unmap/clear */
> +		*last_update = fence;
>  	}

Which allows that to be dropped.

>  
>  	r = amdgpu_vm_update_pdes(adev, vm, false);
> @@ -839,6 +813,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>  	struct drm_syncobj *timeline_syncobj = NULL;
>  	struct dma_fence_chain *timeline_chain = NULL;
>  	struct dma_fence *fence;
> +	struct dma_fence *last_update_fence = NULL;
>  	struct drm_exec exec;
>  	uint64_t va_flags;
>  	uint64_t vm_size;
> @@ -934,6 +909,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>  		bo_va = NULL;
>  	}
>  
> +	/* Update timeline node for synchronization */
>  	r = amdgpu_gem_update_timeline_node(filp,
>  					    args->vm_timeline_syncobj_out,
>  					    args->vm_timeline_point,
> @@ -942,6 +918,10 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>  	if (r)
>  		goto error;
>  
> +	/* Call to update VM and retrieve the resulting fence */
> +	fence = amdgpu_gem_va_update_vm(adev, &fpriv->vm, bo_va,
> +					args->operation, &last_update_fence);
> +
>  	switch (args->operation) {
>  	case AMDGPU_VA_OP_MAP:
>  		va_flags = amdgpu_gem_va_map_flags(adev, args->flags);
> @@ -967,19 +947,25 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>  	default:
>  		break;
>  	}
> +
>  	if (!r && !(args->flags & AMDGPU_VM_DELAY_UPDATE) && !adev->debug_vm) {
> -		fence = amdgpu_gem_va_update_vm(adev, &fpriv->vm, bo_va,
> -						args->operation);
> -
> -		if (timeline_syncobj)
> -			amdgpu_gem_update_bo_mapping(filp, bo_va,
> -					     args->operation,
> -					     args->vm_timeline_point,
> -					     fence, timeline_syncobj,
> -					     timeline_chain);
> -		else
> +		if (timeline_syncobj) {
> +			if (last_update_fence) {

The fence will never be NULL here.

Apart from that looks good to me now,
Christian.

> +				if (!args->vm_timeline_point) {
> +					/* Replace the existing fence if point is not set */
> +					drm_syncobj_replace_fence(timeline_syncobj,
> +								  last_update_fence);
> +				} else {
> +					/* Add last_update_fence at a specific point */
> +					drm_syncobj_add_point(timeline_syncobj,
> +							      timeline_chain,
> +							      last_update_fence,
> +							      args->vm_timeline_point);
> +				}
> +			}
> +		} else {
>  			dma_fence_put(fence);
> -
> +		}
>  	}
>  
>  error: