On 10/8/26 11:46, John Fisher wrote: > > > 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.
Yeah that sounds like the right thing to do. We are in the process of dropping those clears, but it is still something we need to backport. In general CPU based page table updates are not well tested nor supported, so I wouldn't put to much effort into them. If they keep making trouble I think we will just drop the feature. > 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). No, that is code we are slowly removing over time. So I clearly don't want to add anything new here. Regards, Christian. > > 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] > <mailto:[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] <mailto:[email protected]> # v6.16+ > > Cc: Prike Liang <[email protected] <mailto:[email protected]>> > > Signed-off-by: John Fisher <[email protected] > <mailto:[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 <http://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 > > >
