Ok understood. Uninterruptible with no return value check. Which fence should it wait on, though?
The jobs that race here are the PD/PT clears from the render node's open, and as far as I can see their fences only end up as BOOKKEEP fences on the root PD: amdgpu_vm_pt_clear() commits with a NULL fence, so they never reach vm->last_update. Nothing else sets vm->last_update between amdgpu_vm_init() and ACQUIRE_VM either: amdgpu_seq64_map() goes through amdgpu_vm_bo_update(), which puts its fence in bo_va->last_pt_update because the seq64 BO is not always valid; amdgpu_map_static_csa() only maps; and the PDEs of the new tables stay on vm->kernel.needs_update. So at this point vm->last_update is still the stub, and a dma_fence_wait(vm->last_update, false) here would not close the race. I think maybe two other ways to wait for the VM's own updates: 1. Have amdgpu_vm_pt_clear() commit with &vm->last_update instead of NULL, and dma_fence_wait(vm->last_update, false) here before it is dropped. The clears all go through vm->delayed, so the last one covers the earlier ones. 2. Keep the clears as they are and wait here only for the fences on the root PD whose owner is AMDGPU_FENCE_OWNER_VM (exporting amdgpu_sync_get_owner() or adding a small helper to amdgpu_sync.c). Which would you prefer, or did you have something else in mind? I'll send a v2 once I've rerun the tests on it. Thanks, John On Thu, 8 Oct 2026 at 21:13, Christian König <[email protected]> wrote: > > On 10/7/26 22:03, 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 VM's own page > > table jobs (the root clear from amdgpu_vm_init(), and the PD/PT clears > > and the seq64 mapping from the render node's open) are added by > > amdgpu_vm_sdma_commit() with DMA_RESV_USAGE_BOOKKEEP. 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. > > > > Wait for every fence on the root PD's reservation before switching the > > update functions. With this the same client reads the handler 10 of 10 > > times, 8 busy queues on the 4 compute slots complete in 5 of 5 runs > > (1 of 6 before), and two processes with 8 queues each complete together, > > 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]> > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 +++++++++++++---- > > 1 file changed, 13 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > > index 29187fabc..8d0c03a40 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > > @@ -2736,11 +2736,20 @@ int amdgpu_vm_make_compute(struct amdgpu_device > > *adev, struct amdgpu_vm *vm) > > "CPU update of VM recommended only for large BAR > > system\n"); > > > > 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) > > + /* > > + * Sync with the last SDMA update/clear before switching to > > CPU > > + * updates. The VM's own page table updates are tracked with > > + * DMA_RESV_USAGE_BOOKKEEP on the root PD, which > > + * amdgpu_bo_sync_wait() does not wait for: a clear of a PD > > still > > + * pending here lands after the CPU has written its entries. > > That comment is a bit superfluous, it describes the what and not the why. > > Just keep the "Sync with last SDMA update/clear before switching to CPU" or > maybe change to "Sync with the last async SDMA update". > > > + */ > > + long t = dma_resv_wait_timeout(vm->root.bo->tbo.base.resv, > > + DMA_RESV_USAGE_BOOKKEEP, > > true, > > + MAX_SCHEDULE_TIMEOUT); > > That is quite suboptimal, we don't want to wait for the BOOKKEEP fences here. > And drop the return value checking and wait uninterruptible. > > Regards, > Christian. > > > + if (t < 0) { > > + r = t; > > goto unreserve_bo; > > + } > > > > vm->update_funcs = &amdgpu_vm_cpu_funcs; > > r = amdgpu_vm_pt_map_tables(adev, vm); > > -- > > 2.55.0 > > >
