[AMD Official Use Only - General] On 2024-01-11 11:19, Felix Kuehling wrote: > On 2024-01-11 02:22, Lang Yu wrote: >> Fixes: 410f08516e0f ("drm/amdkfd: Move dma unmapping after TLB flush") >> >> [ 41.708711] WARNING: CPU: 0 PID: 1463 at >> drivers/gpu/drm/ttm/ttm_bo.c:846 ttm_bo_validate+0x146/0x1b0 [ttm] >> [ 41.708989] Call Trace: >> [ 41.708992] <TASK> >> [ 41.708996] ? show_regs+0x6c/0x80 >> [ 41.709000] ? ttm_bo_validate+0x146/0x1b0 [ttm] >> [ 41.709008] ? __warn+0x93/0x190 >> [ 41.709014] ? ttm_bo_validate+0x146/0x1b0 [ttm] >> [ 41.709024] ? report_bug+0x1f9/0x210 >> [ 41.709035] ? handle_bug+0x46/0x80 >> [ 41.709041] ? exc_invalid_op+0x1d/0x80 >> [ 41.709048] ? asm_exc_invalid_op+0x1f/0x30 >> [ 41.709057] ? amdgpu_amdkfd_gpuvm_dmaunmap_mem+0x2c/0x80 [amdgpu] >> [ 41.709185] ? ttm_bo_validate+0x146/0x1b0 [ttm] >> [ 41.709197] ? amdgpu_amdkfd_gpuvm_dmaunmap_mem+0x2c/0x80 [amdgpu] >> [ 41.709337] ? srso_alias_return_thunk+0x5/0x7f >> [ 41.709346] kfd_mem_dmaunmap_attachment+0x9e/0x1e0 [amdgpu] >> [ 41.709467] amdgpu_amdkfd_gpuvm_dmaunmap_mem+0x56/0x80 [amdgpu] >> [ 41.709586] kfd_ioctl_unmap_memory_from_gpu+0x1b7/0x300 [amdgpu] >> [ 41.709710] kfd_ioctl+0x1ec/0x650 [amdgpu] >> [ 41.709822] ? __pfx_kfd_ioctl_unmap_memory_from_gpu+0x10/0x10 >> [amdgpu] >> [ 41.709945] ? srso_alias_return_thunk+0x5/0x7f >> [ 41.709949] ? tomoyo_file_ioctl+0x20/0x30 >> [ 41.709959] __x64_sys_ioctl+0x9c/0xd0 >> [ 41.709967] do_syscall_64+0x3f/0x90 >> [ 41.709973] entry_SYSCALL_64_after_hwframe+0x6e/0xd8 >> >> Signed-off-by: Lang Yu <lang...@amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c >> index 48697b789342..f5542a4ab8ed 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c >> @@ -2095,8 +2095,13 @@ void amdgpu_amdkfd_gpuvm_dmaunmap_mem(struct >> kgd_mem *mem, void *drm_priv) >> mutex_lock(&mem->lock); >> list_for_each_entry(entry, &mem->attachments, list) { >> - if (entry->bo_va->base.vm == vm) >> + if (entry->bo_va->base.vm != vm) >> + continue; >> + >> + if (!WARN_ON(amdgpu_bo_reserve(entry->bo_va->base.bo, true))) { >> kfd_mem_dmaunmap_attachment(mem, entry); >> + amdgpu_bo_unreserve(entry->bo_va->base.bo); >> + } > > I'm pretty sure someone else worked on a fix for this before. This is > not a good solution. We need to handle failed reservations (due to > ERESTARTSYS) and make sure that the unmap ioctl can be restarted > correctly in that case. > > See > https://lore.kernel.org/amd-gfx/530aac57-5561-4d1d-879a-93b108e5c...@gmail.com/ > > David, do you have any update on this work? > I tried to solve this same problem.
After feedback from November's post, I updated my patch. In testing, I found that my patch (attached to avoid cluttering this thread and to make clear the patch is not ready for primetime) causes lockdep to emit possible circular dependency warnings, so I haven't pursued it any further. > Regards, > Felix > > >> } >> mutex_unlock(&mem->lock);
From 5f00ef4518ae1971f2cd31a89a647380b49fbe43 Mon Sep 17 00:00:00 2001 From: David Francis <david.fran...@amd.com> Date: Thu, 5 Oct 2023 11:36:06 -0400 Subject: [PATCH] drm/amdgpu: Acquire ttm locks for dmaunmap dmaunmap can call ttm_bo_validate, which expects the ttm dma_resv to be held. Acquire the locks in amdgpu_amdkfd_gpuvm_dmaunmap_mem. Because the dmaunmap step can now fail, it is now necessary to track which attachments have and have not been dmaunmapped. This failure can also cause the sync_memory step of the ioctl to be repeated; it is idempotent, so this should not cause any issues. Signed-off-by: David Francis <david.fran...@amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 3 ++- .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 18 +++++++++++++++--- drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 7 ++++++- 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h index 3ad8dc523b42..aeed6a893902 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h @@ -58,6 +58,7 @@ struct kfd_mem_attachment { struct list_head list; enum kfd_mem_attachment_type type; bool is_mapped; + bool is_dmamapped; struct amdgpu_bo_va *bo_va; struct amdgpu_device *adev; uint64_t va; @@ -302,7 +303,7 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(struct amdgpu_device *adev, struct kgd_mem *mem, void *drm_priv); int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu( struct amdgpu_device *adev, struct kgd_mem *mem, void *drm_priv); -void amdgpu_amdkfd_gpuvm_dmaunmap_mem(struct kgd_mem *mem, void *drm_priv); +int amdgpu_amdkfd_gpuvm_dmaunmap_mem(struct kgd_mem *mem, void *drm_priv); int amdgpu_amdkfd_gpuvm_sync_memory( struct amdgpu_device *adev, struct kgd_mem *mem, bool intr); int amdgpu_amdkfd_gpuvm_map_gtt_bo_to_kernel(struct kgd_mem *mem, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index 54f31a420229..c05acd99c25a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -2081,6 +2081,7 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu( } entry->is_mapped = true; + entry->is_dmamapped = true; mem->mapped_to_gpu_memory++; pr_debug("\t INC mapping count %d\n", mem->mapped_to_gpu_memory); @@ -2102,21 +2103,32 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu( return ret; } -void amdgpu_amdkfd_gpuvm_dmaunmap_mem(struct kgd_mem *mem, void *drm_priv) +int amdgpu_amdkfd_gpuvm_dmaunmap_mem(struct kgd_mem *mem, void *drm_priv) { struct kfd_mem_attachment *entry; struct amdgpu_vm *vm; + int ret; vm = drm_priv_to_vm(drm_priv); mutex_lock(&mem->lock); list_for_each_entry(entry, &mem->attachments, list) { - if (entry->bo_va->base.vm == vm) + if (entry->bo_va->base.vm == vm && entry->is_dmamapped) { + ret = amdgpu_bo_reserve(entry->bo_va->base.bo, false); + if (ret) { + goto out; + } + kfd_mem_dmaunmap_attachment(mem, entry); - } + entry->is_dmamapped = false; + amdgpu_bo_unreserve(entry->bo_va->base.bo); + } + } +out: mutex_unlock(&mem->lock); + return ret; } int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu( diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c index 06988cf1db51..d75c181cb744 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c @@ -1442,7 +1442,11 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file *filep, kfd_flush_tlb(peer_pdd, TLB_FLUSH_HEAVYWEIGHT); /* Remove dma mapping after tlb flush to avoid IO_PAGE_FAULT */ - amdgpu_amdkfd_gpuvm_dmaunmap_mem(mem, peer_pdd->drm_priv); + err = amdgpu_amdkfd_gpuvm_dmaunmap_mem(mem, peer_pdd->drm_priv); + if (err) { + pr_debug("DMA unmapping failed, acquire interrupted by user signal\n"); + goto dmaunmap_failed; + } } mutex_unlock(&p->mutex); @@ -1455,6 +1459,7 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file *filep, get_mem_obj_from_handle_failed: unmap_memory_from_gpu_failed: sync_memory_failed: +dmaunmap_failed: mutex_unlock(&p->mutex); copy_from_user_failed: kfree(devices_arr); -- 2.34.1