[AMD Official Use Only]
Hi Ramesh,
kfd_mem_attach_dmabuf would deadlock if the BO is already reserved, which is obviously problematic.
Also, if it's AQL, we make 2 BOs, and would need to reserve each one of those during addition, which we can't easily do if we lock outside of attach.
Kent
> -----Original Message-----
> From: Errabolu, Ramesh <Ramesh.Errabolu@xxxxxxx>
> Sent: Monday, November 1, 2021 4:43 PM
> To: Kuehling, Felix <Felix.Kuehling@xxxxxxx>; Russell, Kent <Kent.Russell@xxxxxxx>;
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH] drm/amdgpu: Make sure to reserve BOs before adding or removing
>
> Kent, the call to map has the same structure. Is it not possible to call kfd_mem_attach after
> BO's are reserved?
>
> Regards,
> Ramesh
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Felix Kuehling
> Sent: Monday, November 1, 2021 3:18 PM
> To: Russell, Kent <Kent.Russell@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH] drm/amdgpu: Make sure to reserve BOs before adding or removing
>
> [CAUTION: External Email]
>
> Am 2021-11-01 um 4:14 p.m. schrieb Kent Russell:
> > BOs need to be reserved before they are added or removed, so ensure
> > that they are reserved during kfd_mem_attach and kfd_mem_detach
> >
> > Signed-off-by: Kent Russell <kent.russell@xxxxxxx>
>
> Reviewed-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>
>
>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 13 ++++++++++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > index 61784bbfd7fd..8d30ccd70800 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > @@ -798,14 +798,19 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct
> kgd_mem *mem,
> > }
> >
> > /* Add BO to VM internal data structures */
> > + ret = amdgpu_bo_reserve(bo[i], false);
> > + if (ret) {
> > + pr_debug("Unable to reserve BO during memory attach");
> > + goto unwind;
> > + }
> > attachment[i]->bo_va = amdgpu_vm_bo_add(adev, vm,
> > bo[i]);
> > + amdgpu_bo_unreserve(bo[i]);
> > if (unlikely(!attachment[i]->bo_va)) {
> > ret = -ENOMEM;
> > pr_err("Failed to add BO object to VM. ret == %d\n",
> > ret);
> > goto unwind;
> > }
> > -
> > attachment[i]->va = va;
> > attachment[i]->pte_flags = get_pte_flags(adev, mem);
> > attachment[i]->adev = adev; @@ -821,7 +826,9 @@ static
> > int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
> > if (!attachment[i])
> > continue;
> > if (attachment[i]->bo_va) {
> > + amdgpu_bo_reserve(bo[i], true);
> > amdgpu_vm_bo_rmv(adev, attachment[i]->bo_va);
> > + amdgpu_bo_unreserve(bo[i]);
> > list_del(&attachment[i]->list);
> > }
> > if (bo[i])
> > @@ -1700,12 +1707,12 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
> > pr_debug("Release VA 0x%llx - 0x%llx\n", mem->va,
> > mem->va + bo_size * (1 + mem->aql_queue));
> >
> > - ret = unreserve_bo_and_vms(&ctx, false, false);
> > -
> > /* Remove from VM internal data structures */
> > list_for_each_entry_safe(entry, tmp, &mem->attachments, list)
> > kfd_mem_detach(entry);
> >
> > + ret = unreserve_bo_and_vms(&ctx, false, false);
> > +
> > /* Free the sync object */
> > amdgpu_sync_free(&mem->sync);
> >
