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 <[email protected]>
> Cc: Christian König <[email protected]>
> Suggested-by: Christian König <[email protected]>
> Signed-off-by: Srinivasan Shanmugam <[email protected]>
> ---
> 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
<[email protected]>
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:
