[PATCH 1/2] drm/amdgpu: fix missing fence reserve in amdgpu_vm_sdma_commit

[Li, Yunxiang (Teddy)]

[Public]

Hmm, seems all the stack trace I get now are from amdgpu_gem_va_ioctl, so I suppose the issue is only on that path now, the gang submit patch fixed it for amdgpu_cs_ioctl.

Teddy

-----Original Message-----
From: Li, Yunxiang (Teddy)
Sent: Thursday, June 22, 2023 10:12
To: Koenig, Christian <Christian.Koenig at amd.com>; dri-devel at lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher at amd.com>
Subject: RE: [PATCH 1/2] drm/amdgpu: fix missing fence reserve in amdgpu_vm_sdma_commit

If I take out this reserve and the change that makes dma_resv_reserve_fences nestable, I get a BUG in the add_fence, so we might be missing a reserve somewhere else then.

Teddy

-----Original Message-----
From: Koenig, Christian <Christian.Koenig at amd.com>
Sent: Thursday, June 22, 2023 3:39
To: Li, Yunxiang (Teddy) <Yunxiang.Li at amd.com>; dri-devel at lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher at amd.com>
Subject: Re: [PATCH 1/2] drm/amdgpu: fix missing fence reserve in amdgpu_vm_sdma_commit

Am 21.06.23 um 18:23 schrieb Yunxiang Li:
> When amdgpu_bo_fence is converted to dma_resv_add_fence, the reserve
> was removed in that process, so putting it back.

The slots for this are reserved in amdgpu_vm_get_pd_bo():

         /* Two for VM updates, one for TTM and one for the CS job */
         entry->tv.num_shared = 4;

The problem here is rather that we didn't took the gang submit into account. See my patch earlier this week I've CCed you on.
.
>
> Fixes: 4247084057cf ("drm/amdgpu: use DMA_RESV_USAGE_BOOKKEEP v2")
> Signed-off-by: Yunxiang Li <Yunxiang.Li at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> index 349416e176a1..f590b97853d9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -120,6 +120,7 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
>       struct amdgpu_ib *ib = p->job->ibs;
>       struct amdgpu_ring *ring;
>       struct dma_fence *f;
> +     int r;
>
>       ring = container_of(p->vm->delayed.rq->sched, struct amdgpu_ring,
>                           sched);
> @@ -135,6 +136,9 @@ static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
>               swap(p->vm->last_unlocked, tmp);
>               dma_fence_put(tmp);
>       } else {
> +             r = dma_resv_reserve_fences(p->vm->root.bo->tbo.base.resv, 1);
> +             if (r)
> +                     return r;

That is simply illegal and would potentially lead to memory corruption.

Regards,
Christian.

>               dma_resv_add_fence(p->vm->root.bo->tbo.base.resv, f,
>                                  DMA_RESV_USAGE_BOOKKEEP);
>       }