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

[Christian König]

On 5/18/25 08:03, 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.
> 
> v2: Addressed review comments from Christian.
> 
> 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 | 82 +++++++++----------------
>  1 file changed, 30 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 2c68118fe9fd..aea0c63e441a 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;
> @@ -780,6 +739,12 @@ amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
>  		r = amdgpu_vm_bo_update(adev, bo_va, false);
>  		if (r)
>  			goto error;
> +
> +		/* Set the last_update fence based on the operation */
> +		if (amdgpu_vm_is_bo_always_valid(vm, bo_va->base.bo))
> +			fence = vm->last_update; /* Use VM's last update fence */
> +		else
> +			fence = bo_va->last_pt_update; /* Use buffer object's last update fence */

The those are not very good comments. A comment should always explain why something is done and not what is done. Additional to that it makes the line to long.

I would just drop them.

>  	}
>  
>  	r = amdgpu_vm_update_pdes(adev, vm, false);
> @@ -845,6 +810,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>  	uint64_t vm_size;
>  	int r = 0;
>  
> +	/* Validate virtual address */
>  	if (args->va_address < AMDGPU_VA_RESERVED_BOTTOM) {
>  		dev_dbg(dev->dev,
>  			"va_address 0x%llx is in reserved area 0x%llx\n",
> @@ -878,6 +844,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>  		return -EINVAL;
>  	}
>  
> +	/* Operational check */
>  	switch (args->operation) {
>  	case AMDGPU_VA_OP_MAP:
>  	case AMDGPU_VA_OP_UNMAP:
> @@ -901,6 +868,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>  		abo = NULL;
>  	}
>  
> +	/* Add input fence for synchronization */
>  	r = amdgpu_gem_add_input_fence(filp,
>  				       args->input_fence_syncobj_handles,
>  				       args->num_syncobj_handles);
> @@ -923,6 +891,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>  			goto error;
>  	}
>  
> +	/* Find buffer object virtual address */
>  	if (abo) {
>  		bo_va = amdgpu_vm_bo_find(&fpriv->vm, abo);
>  		if (!bo_va) {
> @@ -935,6 +904,7 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
>  		bo_va = NULL;
>  	}
>  
> +	/* Update timeline node for synchronization */

Same with those comments, write a bit why we do things and not what is done.

>  	r = amdgpu_gem_update_timeline_node(filp,
>  					    args->vm_timeline_syncobj_out,
>  					    args->vm_timeline_point,
> @@ -943,6 +913,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 */

That's a good comment.

Apart from the comments, the patch is Reviewed-by: Christian König <christian.koenig at amd.com>

Regards,
Christian.

> +	fence = amdgpu_gem_va_update_vm(adev, &fpriv->vm, bo_va,
> +					args->operation);
> +
>  	switch (args->operation) {
>  	case AMDGPU_VA_OP_MAP:
>  		va_flags = amdgpu_gem_va_map_flags(adev, args->flags);
> @@ -968,19 +942,23 @@ 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 (!args->vm_timeline_point) {
> +				/* Replace the existing fence if point is not set */
> +				drm_syncobj_replace_fence(timeline_syncobj,
> +							  fence);
> +			} else {
> +				/* Add last update fence at a specific timeline point */
> +				drm_syncobj_add_point(timeline_syncobj,
> +						      timeline_chain,
> +						      fence,
> +						      args->vm_timeline_point);
> +			}
> +		} else {
>  			dma_fence_put(fence);
> -
> +		}
>  	}
>  
>  error: