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.deuc...@amd.com>
> Cc: Christian König <christian.koe...@amd.com>
> Suggested-by: Christian König <christian.koe...@amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmu...@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:
