[PATCH 1/3] drm/amdgpu: implicitly sync the dependent read fences

[Liang, Prike]

[Public]

> From: Koenig, Christian <Christian.Koenig at amd.com>
> Sent: Monday, April 28, 2025 10:29 PM
> To: Liang, Prike <Prike.Liang at amd.com>; Yadav, Arvind
> <Arvind.Yadav at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
> Subject: Re: [PATCH 1/3] drm/amdgpu: implicitly sync the dependent read fences
>
> On 4/27/25 05:20, Liang, Prike wrote:
> > [Public]
> >
> >> From: Liang, Prike
> >> Sent: Friday, April 25, 2025 3:44 PM
> >> To: Yadav, Arvind <Arvind.Yadav at amd.com>;
> >> amd-gfx at lists.freedesktop.org
> >> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Koenig, Christian
> >> <Christian.Koenig at amd.com>
> >> Subject: RE: [PATCH 1/3] drm/amdgpu: implicitly sync the dependent
> >> read fences
> >>
> >>> From: Yadav, Arvind <Arvind.Yadav at amd.com>
> >>> Sent: Friday, April 25, 2025 3:21 PM
> >>> To: Liang, Prike <Prike.Liang at amd.com>;
> >>> amd-gfx at lists.freedesktop.org
> >>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Koenig,
> >>> Christian <Christian.Koenig at amd.com>
> >>> Subject: Re: [PATCH 1/3] drm/amdgpu: implicitly sync the dependent
> >>> read fences
> >>>
> >>> This is problem for TLB flush. We should not do this changes. Here
> >>> we are utilizing DMA_RESV_USAGE_BOOKKEEP due to the TLB flush fence
> >>> associated with the page table (PT). We are ensuring that no page
> >>> directory (PD) or page table (PT) should be free before flush and
> >>> ttm bo release and delete both are also waiting for BOOKKEEP fence.
> >>> Please drop
> >> this changes for eviction fence.
> >> Thanks for sharing the background. So, we may need to test the fence
> >> whether is an eviction fence in amdgpu_sync_test_fence () before added it to
> the VM sync.
> > It looks the TLB flush fence only added to the VM BO reservation and requires a
> sync at compute VM.
>
> The TLB flush fence should always be explicitly synced to by the KFD.
>
> Where do you see that it is implicitly synced using the amdgpu_sync objected?

 >>>>>/* Prepare a TLB flush fence to be attached to PTs */
->>>>>>>if (!params->unlocked && vm->is_compute_context) {
->>>>>>>->>>>>>>amdgpu_vm_tlb_fence_create(params->adev, vm, fence);

->>>>>>>->>>>>>>/* Makes sure no PD/PT is freed before the flush */
->>>>>>>->>>>>>>dma_resv_add_fence(vm->root.bo->tbo.base.resv, *fence,
->>>>>>>->>>>>>>->>>>>>>->>>>>>>   DMA_RESV_USAGE_BOOKKEEP);
->>>>>>>}
Yes, the TLB flush fence is explicitly sync and the flush should be performed after the dependent PD/PT fence signaled.
To the Arvind's concern on the TLB flush fence should be handled properly by explicitly syncing the TLB fence, so it's
ok to promote the implicit sync to the READ fence from the BOOKEEP?

Thanks,
Prike
> Regards,
> Christian.
>
> > As to the VM reservation sync whether can return and sync to the
> > DMA_RESV_USAGE_READ, and I will further check it separately with the
> > eviction fence release. As to the eviction fence sync problem issue, I would like
> to put exclude the eviction fence sync at amdgpu_sync_test_fence().
> >
> > Thanks,
> > Prike
> >> Thanks,
> >> Prike
> >>> Regards,
> >>> ~arvind
> >>>
> >>> On 4/25/2025 12:37 PM, Prike Liang wrote:
> >>>> The driver doesn't want to sync on the DMA_RESV_USAGE_BOOKKEEP
> >> usage
> >>>> fences, so here only return and sync the dependent read fences.
> >>>>
> >>>> Signed-off-by: Prike Liang <Prike.Liang at amd.com>
> >>>> ---
> >>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 5 ++---
> >>>>   1 file changed, 2 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> >>>> index 5576ed0b508f..4e1d30ecb6cc 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> >>>> @@ -249,9 +249,8 @@ int amdgpu_sync_resv(struct amdgpu_device
> >>>> *adev, struct amdgpu_sync *sync,
> >>>>
> >>>>           if (resv == NULL)
> >>>>                   return -EINVAL;
> >>>> -
> >>>> - /* TODO: Use DMA_RESV_USAGE_READ here */
> >>>> - dma_resv_for_each_fence(&cursor, resv,
> >>> DMA_RESV_USAGE_BOOKKEEP, f) {
> >>>> + /*Only return and sync the fences of usage <=
> >>> DMA_RESV_USAGE_READ.*/
> >>>> + dma_resv_for_each_fence(&cursor, resv, DMA_RESV_USAGE_READ, f)
> {
> >>>>                   dma_fence_chain_for_each(f, f) {
> >>>>                           struct dma_fence *tmp =
> >>>> dma_fence_chain_contained(f);
> >>>>