On 10/8/26 22:45, John Fisher wrote:
> amdgpu_vm_make_compute() switches a VM from SDMA to CPU page table
> updates when a KFD process acquires it. It syncs "with the last SDMA
> update/clear" through amdgpu_bo_sync_wait() on the root PD, but that
> walks the reservation with DMA_RESV_USAGE_READ, and the clears of the
> VM's page tables (the root PD from amdgpu_vm_init(), and the PDs/PTs
> the seq64 mapping allocates at the render node's open) are added there
> by amdgpu_vm_sdma_commit() with DMA_RESV_USAGE_BOOKKEEP only:
> amdgpu_vm_pt_clear() commits them with a NULL fence, so they do not
> reach vm->last_update. The CPU path's own waits (amdgpu_vm_cpu_update(),
> amdgpu_bo_kmap()) use DMA_RESV_USAGE_KERNEL. None of them waits for a
> clear that is still queued.
>
> When one is, the CPU writes the first entries of the new compute VM (the
> KFD trap handler's TBA/TMA page at AMDGPU_VA_RESERVED_TRAP_START, mapped
> during ACQUIRE_VM) into a page directory the SDMA then clears. The
> entries are gone, the handler's page is unmapped, and the first wave
> save of any queue of the process faults in the SQC instruction fetch at
> the handler's address (GCVM_L2_PROTECTION_FAULT_STATUS 0x008012B1,
> PERMISSION_FAULTS 0xb). The MES then fails REMOVE_QUEUE and SUSPEND, and
> the device takes a MODE1 reset with VRAM lost on 7.2.9; on 7.3-rc6 the
> queue is reset instead.
>
> The window is the time between the render node's open and ACQUIRE_VM.
> ROCr spends milliseconds there and is practically never hit; a minimal
> KFD client that opens the render node and acquires the VM at once is hit
> most of the time. On a Radeon AI PRO R9700 (gfx1201, Resizable BAR, so
> vm_update_mode defaults to CPU updates for compute VMs), a wave reading
> the trap handler's page right after queue creation found it unmapped in
> 8 of 12 runs; a 5 ms sleep between the open and ACQUIRE_VM made that 0
> of 9, and the same sleep after queue creation instead 3 of 3.
>
> The wait covered these jobs until commit 926c79ad6ecd ("drm/amdgpu:
> promote the implicit sync to the dependent read fences") narrowed
> amdgpu_sync_resv() from DMA_RESV_USAGE_BOOKKEEP to DMA_RESV_USAGE_READ;
> the VM's page table jobs have been added with BOOKKEEP since commit
> 4247084057cf ("drm/amdgpu: use DMA_RESV_USAGE_BOOKKEEP v2"), so from
> v6.16 on the sync in amdgpu_vm_make_compute() waits for nothing of
> them.
>
> Record the clears in vm->last_update, as amdgpu_vm_update_pdes() does
> with its updates, and wait for it, uninterruptibly, before switching the
> update functions. The clears all go through vm->delayed, so the last one
> covers the earlier ones; immediate clears are not recorded, as before.
>
> With this, on 7.3-rc6 and on 7.2.9, the same client reads the handler
> 10 of 10 times, 8 busy queues on the 4 compute slots complete in 5 of 5
> runs on 7.3-rc6 and 3 of 3 on 7.2.9 (1 of 6 before), 16 and 32 queues
> complete on 7.3-rc6, and two processes with 8 queues each complete
> together on both, with no fault.
>
> Fixes: 926c79ad6ecd ("drm/amdgpu: promote the implicit sync to the dependent
> read fences")
> Cc: [email protected] # v6.16+
> Cc: Prike Liang <[email protected]>
> Signed-off-by: John Fisher <[email protected]>
Reviewed-by: Christian König <[email protected]>
> ---
> v2: wait for vm->last_update, in which amdgpu_vm_pt_clear() now records
> the clears, instead of every BOOKKEEP fence on the root PD; wait
> uninterruptibly without checking the result; keep the original
> comment (Christian)
> v1:
> https://lore.kernel.org/amd-gfx/[email protected]/
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 5 +----
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 2 +-
> 2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 29187fabc..757a037ed 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2737,10 +2737,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev,
> struct amdgpu_vm *vm)
>
> if (vm->use_cpu_for_update) {
> /* Sync with last SDMA update/clear before switching to CPU */
> - r = amdgpu_bo_sync_wait(vm->root.bo,
> - AMDGPU_FENCE_OWNER_UNDEFINED, true);
> - if (r)
> - goto unreserve_bo;
> + dma_fence_wait(vm->last_update, false);
>
> vm->update_funcs = &amdgpu_vm_cpu_funcs;
> r = amdgpu_vm_pt_map_tables(adev, vm);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> index c03327f12..cdbc2e931 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> @@ -427,7 +427,7 @@ int amdgpu_vm_pt_clear(struct amdgpu_device *adev, struct
> amdgpu_vm *vm,
> if (r)
> goto exit;
>
> - r = vm->update_funcs->commit(¶ms, NULL);
> + r = vm->update_funcs->commit(¶ms, &vm->last_update);
> exit:
> drm_dev_exit(idx);
> return r;
