[PATCH] drm/amdgpu: wait for pending page table updates before switching a VM to CPU updates

[PATCH] drm/amdgpu: wait for pending page table updates before switching a VM to CPU updates

[John Fisher]

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: stable@vger.kernel.org # v6.16+
Cc: Prike Liang <Prike.Liang@amd.com>
Signed-off-by: John Fisher <jfisher84@gmail.com>
---
 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.
+		 */
+		long t = dma_resv_wait_timeout(vm->root.bo->tbo.base.resv,
+					       DMA_RESV_USAGE_BOOKKEEP, true,
+					       MAX_SCHEDULE_TIMEOUT);
+		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

Re: [PATCH] drm/amdgpu: wait for pending page table updates before switching a VM to CPU updates

[Christian König]

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: stable@vger.kernel.org # v6.16+
> Cc: Prike Liang <Prike.Liang@amd.com>
> Signed-off-by: John Fisher <jfisher84@gmail.com>
> ---
>  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
> 

Re: [PATCH] drm/amdgpu: wait for pending page table updates before switching a VM to CPU updates

[John Fisher]

[-- Attachment #1: Type: text/plain, Size: 6820 bytes --]

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 <christian.koenig@amd.com>
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: stable@vger.kernel.org # v6.16+
> > Cc: Prike Liang <Prike.Liang@amd.com>
> > Signed-off-by: John Fisher <jfisher84@gmail.com>
> > ---
> >  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
> >
>
>

[-- Attachment #2: Type: text/html, Size: 8249 bytes --]

Re: [PATCH] drm/amdgpu: wait for pending page table updates before switching a VM to CPU updates

[John Fisher]

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 <christian.koenig@amd.com> 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: stable@vger.kernel.org # v6.16+
> > Cc: Prike Liang <Prike.Liang@amd.com>
> > Signed-off-by: John Fisher <jfisher84@gmail.com>
> > ---
> >  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
> >
>

Re: [PATCH] drm/amdgpu: wait for pending page table updates before switching a VM to CPU updates

[Christian König]

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 <christian.koenig@amd.com <mailto:christian.koenig@amd.com>> 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: stable@vger.kernel.org <mailto:stable@vger.kernel.org> # v6.16+
>     > Cc: Prike Liang <Prike.Liang@amd.com <mailto:Prike.Liang@amd.com>>
>     > Signed-off-by: John Fisher <jfisher84@gmail.com <mailto:jfisher84@gmail.com>>
>     > ---
>     >  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
>     >
>