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
>
