[AMD Official Use Only - Internal Distribution Only]

Acked-by: Ramesh Errabolu ramesh.errabolu@amd.com

-----Original Message----- From: amd-gfx amd-gfx-bounces@lists.freedesktop.org On Behalf Of Kuehling, Felix Sent: Wednesday, April 21, 2021 8:31 PM To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: [PATCH v2 02/10] drm/amdgpu: Rename kfd_bo_va_list to kfd_mem_attachment

This name is more fitting, especially for the changes coming next to support multi-GPU systems with proper DMA mappings. Cleaned up the code and renamed some related functions and variables to improve readability.

Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 8 +- .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 209 +++++++++--------- 2 files changed, 104 insertions(+), 113 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h index 313ee49b9f17..c24b2478f445 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h @@ -38,10 +38,10 @@ extern uint64_t amdgpu_amdkfd_total_mem_size;

struct amdgpu_device;

-struct kfd_bo_va_list { - struct list_head bo_list; +struct kfd_mem_attachment { + struct list_head list; struct amdgpu_bo_va *bo_va; - void *kgd_dev; + struct amdgpu_device *adev; bool is_mapped; uint64_t va; uint64_t pte_flags; @@ -50,7 +50,7 @@ struct kfd_bo_va_list { struct kgd_mem { struct mutex lock; struct amdgpu_bo *bo; - struct list_head bo_va_list; + struct list_head attachments; /* protected by amdkfd_process_info.lock */ struct ttm_validate_buffer validate_list; struct ttm_validate_buffer resv_list; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index dfa025d694f8..fee4c64dd051 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -72,16 +72,16 @@ static inline struct amdgpu_device *get_amdgpu_device(struct kgd_dev *kgd) return (struct amdgpu_device *)kgd; }

-static bool check_if_add_bo_to_vm(struct amdgpu_vm *avm, +static bool kfd_mem_is_attached(struct amdgpu_vm *avm, struct kgd_mem *mem) { - struct kfd_bo_va_list *entry; + struct kfd_mem_attachment *entry;

- list_for_each_entry(entry, &mem->bo_va_list, bo_list) + list_for_each_entry(entry, &mem->attachments, list) if (entry->bo_va->base.vm == avm) - return false; + return true;

- return true; + return false; }

/* Set memory usage limits. Current, limits are @@ -473,7 +473,7 @@ static uint64_t get_pte_flags(struct amdgpu_device *adev, struct kgd_mem *mem) return pte_flags; }

-/* add_bo_to_vm - Add a BO to a VM +/* kfd_mem_attach - Add a BO to a VM * * Everything that needs to bo done only once when a BO is first added * to a VM. It can later be mapped and unmapped many times without @@ -485,15 +485,14 @@ static uint64_t get_pte_flags(struct amdgpu_device *adev, struct kgd_mem *mem) * 4. Alloc page tables and directories if needed * 4a. Validate new page tables and directories */ -static int add_bo_to_vm(struct amdgpu_device *adev, struct kgd_mem *mem, +static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem +*mem, struct amdgpu_vm *vm, bool is_aql, - struct kfd_bo_va_list **p_bo_va_entry) + struct kfd_mem_attachment **p_attachment) { int ret; - struct kfd_bo_va_list *bo_va_entry; + struct kfd_mem_attachment *attachment; struct amdgpu_bo *bo = mem->bo; uint64_t va = mem->va; - struct list_head *list_bo_va = &mem->bo_va_list; unsigned long bo_size = bo->tbo.base.size;

if (!va) { @@ -504,29 +503,29 @@ static int add_bo_to_vm(struct amdgpu_device *adev, struct kgd_mem *mem, if (is_aql) va += bo_size;

- bo_va_entry = kzalloc(sizeof(*bo_va_entry), GFP_KERNEL); - if (!bo_va_entry) + attachment = kzalloc(sizeof(*attachment), GFP_KERNEL); + if (!attachment) return -ENOMEM;

pr_debug("\t add VA 0x%llx - 0x%llx to vm %p\n", va, va + bo_size, vm);

/* Add BO to VM internal data structures*/ - bo_va_entry->bo_va = amdgpu_vm_bo_add(adev, vm, bo); - if (!bo_va_entry->bo_va) { + attachment->bo_va = amdgpu_vm_bo_add(adev, vm, bo); + if (!attachment->bo_va) { ret = -EINVAL; pr_err("Failed to add BO object to VM. ret == %d\n", ret); goto err_vmadd; }

- bo_va_entry->va = va; - bo_va_entry->pte_flags = get_pte_flags(adev, mem); - bo_va_entry->kgd_dev = (void *)adev; - list_add(&bo_va_entry->bo_list, list_bo_va); + attachment->va = va; + attachment->pte_flags = get_pte_flags(adev, mem); + attachment->adev = adev; + list_add(&attachment->list, &mem->attachments);

- if (p_bo_va_entry) - *p_bo_va_entry = bo_va_entry; + if (p_attachment) + *p_attachment = attachment;

/* Allocate validate page tables if needed */ ret = vm_validate_pt_pd_bos(vm); @@ -538,22 +537,20 @@ static int add_bo_to_vm(struct amdgpu_device *adev, struct kgd_mem *mem, return 0;

err_alloc_pts: - amdgpu_vm_bo_rmv(adev, bo_va_entry->bo_va); - list_del(&bo_va_entry->bo_list); + amdgpu_vm_bo_rmv(adev, attachment->bo_va); + list_del(&attachment->list); err_vmadd: - kfree(bo_va_entry); + kfree(attachment); return ret; }

-static void remove_bo_from_vm(struct amdgpu_device *adev, - struct kfd_bo_va_list *entry, unsigned long size) +static void kfd_mem_detach(struct kfd_mem_attachment *attachment) { - pr_debug("\t remove VA 0x%llx - 0x%llx in entry %p\n", - entry->va, - entry->va + size, entry); - amdgpu_vm_bo_rmv(adev, entry->bo_va); - list_del(&entry->bo_list); - kfree(entry); + pr_debug("\t remove VA 0x%llx in entry %p\n", + attachment->va, attachment); + amdgpu_vm_bo_rmv(attachment->adev, attachment->bo_va); + list_del(&attachment->list); + kfree(attachment); }

static void add_kgd_mem_to_kfd_bo_list(struct kgd_mem *mem, @@ -728,7 +725,7 @@ static int reserve_bo_and_cond_vms(struct kgd_mem *mem, struct bo_vm_reservation_context *ctx) { struct amdgpu_bo *bo = mem->bo; - struct kfd_bo_va_list *entry; + struct kfd_mem_attachment *entry; unsigned int i; int ret;

@@ -740,7 +737,7 @@ static int reserve_bo_and_cond_vms(struct kgd_mem *mem, INIT_LIST_HEAD(&ctx->list); INIT_LIST_HEAD(&ctx->duplicates);

- list_for_each_entry(entry, &mem->bo_va_list, bo_list) { + list_for_each_entry(entry, &mem->attachments, list) { if ((vm && vm != entry->bo_va->base.vm) || (entry->is_mapped != map_type && map_type != BO_VM_ALL)) @@ -762,7 +759,7 @@ static int reserve_bo_and_cond_vms(struct kgd_mem *mem, list_add(&ctx->kfd_bo.tv.head, &ctx->list);

i = 0; - list_for_each_entry(entry, &mem->bo_va_list, bo_list) { + list_for_each_entry(entry, &mem->attachments, list) { if ((vm && vm != entry->bo_va->base.vm) || (entry->is_mapped != map_type && map_type != BO_VM_ALL)) @@ -817,7 +814,7 @@ static int unreserve_bo_and_vms(struct bo_vm_reservation_context *ctx, }

static int unmap_bo_from_gpuvm(struct amdgpu_device *adev, - struct kfd_bo_va_list *entry, + struct kfd_mem_attachment *entry, struct amdgpu_sync *sync) { struct amdgpu_bo_va *bo_va = entry->bo_va; @@ -833,7 +830,7 @@ static int unmap_bo_from_gpuvm(struct amdgpu_device *adev, }

static int update_gpuvm_pte(struct amdgpu_device *adev, - struct kfd_bo_va_list *entry, + struct kfd_mem_attachment *entry, struct amdgpu_sync *sync) { int ret; @@ -850,7 +847,7 @@ static int update_gpuvm_pte(struct amdgpu_device *adev, }

static int map_bo_to_gpuvm(struct amdgpu_device *adev, - struct kfd_bo_va_list *entry, struct amdgpu_sync *sync, + struct kfd_mem_attachment *entry, struct amdgpu_sync *sync, bool no_update_pte) { int ret; @@ -1194,7 +1191,7 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu( ret = -ENOMEM; goto err; } - INIT_LIST_HEAD(&(*mem)->bo_va_list); + INIT_LIST_HEAD(&(*mem)->attachments); mutex_init(&(*mem)->lock); (*mem)->aql_queue = !!(flags & KFD_IOC_ALLOC_MEM_FLAGS_AQL_QUEUE_MEM);

@@ -1283,7 +1280,7 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu( { struct amdkfd_process_info *process_info = mem->process_info; unsigned long bo_size = mem->bo->tbo.base.size; - struct kfd_bo_va_list *entry, *tmp; + struct kfd_mem_attachment *entry, *tmp; struct bo_vm_reservation_context ctx; struct ttm_validate_buffer *bo_list_entry; unsigned int mapped_to_gpu_memory; @@ -1327,9 +1324,8 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu( mem->va + bo_size * (1 + mem->aql_queue));

/* Remove from VM internal data structures */ - list_for_each_entry_safe(entry, tmp, &mem->bo_va_list, bo_list) - remove_bo_from_vm((struct amdgpu_device *)entry->kgd_dev, - entry, bo_size); + list_for_each_entry_safe(entry, tmp, &mem->attachments, list) + kfd_mem_detach(entry);

ret = unreserve_bo_and_vms(&ctx, false, false);

@@ -1372,10 +1368,10 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu( int ret; struct amdgpu_bo *bo; uint32_t domain; - struct kfd_bo_va_list *entry; + struct kfd_mem_attachment *entry; struct bo_vm_reservation_context ctx; - struct kfd_bo_va_list *bo_va_entry = NULL; - struct kfd_bo_va_list *bo_va_entry_aql = NULL; + struct kfd_mem_attachment *attachment = NULL; + struct kfd_mem_attachment *attachment_aql = NULL; unsigned long bo_size; bool is_invalid_userptr = false;

@@ -1424,21 +1420,20 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu( bo->tbo.mem.mem_type == TTM_PL_SYSTEM) is_invalid_userptr = true;

- if (check_if_add_bo_to_vm(avm, mem)) { - ret = add_bo_to_vm(adev, mem, avm, false, - &bo_va_entry); + if (!kfd_mem_is_attached(avm, mem)) { + ret = kfd_mem_attach(adev, mem, avm, false, &attachment); if (ret) - goto add_bo_to_vm_failed; + goto attach_failed; if (mem->aql_queue) { - ret = add_bo_to_vm(adev, mem, avm, - true, &bo_va_entry_aql); + ret = kfd_mem_attach(adev, mem, avm, true, + &attachment_aql); if (ret) - goto add_bo_to_vm_failed_aql; + goto attach_failed_aql; } } else { ret = vm_validate_pt_pd_bos(avm); if (unlikely(ret)) - goto add_bo_to_vm_failed; + goto attach_failed; }

if (mem->mapped_to_gpu_memory == 0 && @@ -1454,30 +1449,30 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu( } }

- list_for_each_entry(entry, &mem->bo_va_list, bo_list) { - if (entry->bo_va->base.vm == avm && !entry->is_mapped) { - pr_debug("\t map VA 0x%llx - 0x%llx in entry %p\n", - entry->va, entry->va + bo_size, - entry); + list_for_each_entry(entry, &mem->attachments, list) { + if (entry->bo_va->base.vm != avm || entry->is_mapped) + continue;

- ret = map_bo_to_gpuvm(adev, entry, ctx.sync, - is_invalid_userptr); - if (ret) { - pr_err("Failed to map bo to gpuvm\n"); - goto map_bo_to_gpuvm_failed; - } + pr_debug("\t map VA 0x%llx - 0x%llx in entry %p\n", + entry->va, entry->va + bo_size, entry);

- ret = vm_update_pds(avm, ctx.sync); - if (ret) { - pr_err("Failed to update page directories\n"); - goto map_bo_to_gpuvm_failed; - } + ret = map_bo_to_gpuvm(adev, entry, ctx.sync, + is_invalid_userptr); + if (ret) { + pr_err("Failed to map bo to gpuvm\n"); + goto map_bo_to_gpuvm_failed; + }

- entry->is_mapped = true; - mem->mapped_to_gpu_memory++; - pr_debug("\t INC mapping count %d\n", - mem->mapped_to_gpu_memory); + ret = vm_update_pds(avm, ctx.sync); + if (ret) { + pr_err("Failed to update page directories\n"); + goto map_bo_to_gpuvm_failed; } + + entry->is_mapped = true; + mem->mapped_to_gpu_memory++; + pr_debug("\t INC mapping count %d\n", + mem->mapped_to_gpu_memory); }

if (!amdgpu_ttm_tt_get_usermm(bo->tbo.ttm) && !bo->tbo.pin_count) @@ -1489,12 +1484,12 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu( goto out;

map_bo_to_gpuvm_failed: - if (bo_va_entry_aql) - remove_bo_from_vm(adev, bo_va_entry_aql, bo_size); -add_bo_to_vm_failed_aql: - if (bo_va_entry) - remove_bo_from_vm(adev, bo_va_entry, bo_size); -add_bo_to_vm_failed: + if (attachment_aql) + kfd_mem_detach(attachment_aql); +attach_failed_aql: + if (attachment) + kfd_mem_detach(attachment); +attach_failed: unreserve_bo_and_vms(&ctx, false, false); out: mutex_unlock(&mem->process_info->lock); @@ -1509,7 +1504,7 @@ int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu( struct amdgpu_vm *avm = drm_priv_to_vm(drm_priv); struct amdkfd_process_info *process_info = avm->process_info; unsigned long bo_size = mem->bo->tbo.base.size; - struct kfd_bo_va_list *entry; + struct kfd_mem_attachment *entry; struct bo_vm_reservation_context ctx; int ret;

@@ -1533,26 +1528,24 @@ int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu( mem->va + bo_size * (1 + mem->aql_queue), avm);

- list_for_each_entry(entry, &mem->bo_va_list, bo_list) { - if (entry->bo_va->base.vm == avm && entry->is_mapped) { - pr_debug("\t unmap VA 0x%llx - 0x%llx from entry %p\n", - entry->va, - entry->va + bo_size, - entry); + list_for_each_entry(entry, &mem->attachments, list) { + if (entry->bo_va->base.vm != avm || !entry->is_mapped) + continue;

- ret = unmap_bo_from_gpuvm(adev, entry, ctx.sync); - if (ret == 0) { - entry->is_mapped = false; - } else { - pr_err("failed to unmap VA 0x%llx\n", - mem->va); - goto unreserve_out; - } + pr_debug("\t unmap VA 0x%llx - 0x%llx from entry %p\n", + entry->va, entry->va + bo_size, entry);

- mem->mapped_to_gpu_memory--; - pr_debug("\t DEC mapping count %d\n", - mem->mapped_to_gpu_memory); + ret = unmap_bo_from_gpuvm(adev, entry, ctx.sync); + if (ret == 0) { + entry->is_mapped = false; + } else { + pr_err("failed to unmap VA 0x%llx\n", mem->va); + goto unreserve_out; } + + mem->mapped_to_gpu_memory--; + pr_debug("\t DEC mapping count %d\n", + mem->mapped_to_gpu_memory); }

/* If BO is unmapped from all VMs, unfence it. It can be evicted if @@ -1701,7 +1694,7 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct kgd_dev *kgd, if (mmap_offset) *mmap_offset = amdgpu_bo_mmap_offset(bo);

- INIT_LIST_HEAD(&(*mem)->bo_va_list); + INIT_LIST_HEAD(&(*mem)->attachments); mutex_init(&(*mem)->lock);

(*mem)->alloc_flags = @@ -1898,7 +1891,7 @@ static int validate_invalid_user_pages(struct amdkfd_process_info *process_info) list_for_each_entry_safe(mem, tmp_mem, &process_info->userptr_inval_list, validate_list.head) { - struct kfd_bo_va_list *bo_va_entry; + struct kfd_mem_attachment *attachment;

bo = mem->bo;

@@ -1921,13 +1914,13 @@ static int validate_invalid_user_pages(struct amdkfd_process_info *process_info) * VM faults if the GPU tries to access the invalid * memory. */ - list_for_each_entry(bo_va_entry, &mem->bo_va_list, bo_list) { - if (!bo_va_entry->is_mapped) + list_for_each_entry(attachment, &mem->attachments, list) { + if (!attachment->is_mapped) continue;

ret = update_gpuvm_pte((struct amdgpu_device *) - bo_va_entry->kgd_dev, - bo_va_entry, &sync); + attachment->adev, + attachment, &sync); if (ret) { pr_err("%s: update PTE failed\n", __func__); /* make sure this gets validated again */ @@ -2108,7 +2101,7 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)

struct amdgpu_bo *bo = mem->bo; uint32_t domain = mem->domain; - struct kfd_bo_va_list *bo_va_entry; + struct kfd_mem_attachment *attachment;

total_size += amdgpu_bo_size(bo);

@@ -2128,11 +2121,9 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef) pr_debug("Memory eviction: Sync BO fence failed. Try again\n"); goto validate_map_fail; } - list_for_each_entry(bo_va_entry, &mem->bo_va_list, - bo_list) { + list_for_each_entry(attachment, &mem->attachments, list) { ret = update_gpuvm_pte((struct amdgpu_device *) - bo_va_entry->kgd_dev, - bo_va_entry, + attachment->adev, attachment, &sync_obj); if (ret) { pr_debug("Memory eviction: update PTE failed. Try again\n"); @@ -2208,7 +2199,7 @@ int amdgpu_amdkfd_add_gws_to_process(void *info, void *gws, struct kgd_mem **mem return -ENOMEM;

mutex_init(&(*mem)->lock); - INIT_LIST_HEAD(&(*mem)->bo_va_list); + INIT_LIST_HEAD(&(*mem)->attachments); (*mem)->bo = amdgpu_bo_ref(gws_bo); (*mem)->domain = AMDGPU_GEM_DOMAIN_GWS; (*mem)->process_info = process_info; -- 2.31.1

_______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...

[AMD Official Use Only - Internal Distribution Only]

Acked-by: Ramesh Errabolu ramesh.errabolu@amd.com

-----Original Message----- From: amd-gfx amd-gfx-bounces@lists.freedesktop.org On Behalf Of Kuehling, Felix Sent: Wednesday, April 21, 2021 8:31 PM To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: [PATCH v2 03/10] drm/amdgpu: Keep a bo-reference per-attachment

For now they all reference the same BO. For correct DMA mappings they will refer to different BOs per-GPU.

Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com --- .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index fee4c64dd051..34c9a2d0028e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -489,11 +489,11 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem, struct amdgpu_vm *vm, bool is_aql, struct kfd_mem_attachment **p_attachment) { - int ret; - struct kfd_mem_attachment *attachment; - struct amdgpu_bo *bo = mem->bo; + unsigned long bo_size = mem->bo->tbo.base.size; uint64_t va = mem->va; - unsigned long bo_size = bo->tbo.base.size; + struct kfd_mem_attachment *attachment; + struct amdgpu_bo *bo; + int ret;

if (!va) { pr_err("Invalid VA when adding BO to VM\n"); @@ -510,6 +510,14 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem, pr_debug("\t add VA 0x%llx - 0x%llx to vm %p\n", va, va + bo_size, vm);

+ /* FIXME: For now all attachments use the same BO. This is incorrect + * because one BO can only have one DMA mapping for one GPU. We need + * one BO per GPU, e.g. a DMABuf import with dynamic attachment. This + * will be addressed one BO-type at a time in subsequent patches. + */ + bo = mem->bo; + drm_gem_object_get(&bo->tbo.base); + /* Add BO to VM internal data structures*/ attachment->bo_va = amdgpu_vm_bo_add(adev, vm, bo); if (!attachment->bo_va) { @@ -529,7 +537,7 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,

/* Allocate validate page tables if needed */ ret = vm_validate_pt_pd_bos(vm); - if (ret) { + if (unlikely(ret)) { pr_err("validate_pt_pd_bos() failed\n"); goto err_alloc_pts; } @@ -540,15 +548,19 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem, amdgpu_vm_bo_rmv(adev, attachment->bo_va); list_del(&attachment->list); err_vmadd: + drm_gem_object_put(&bo->tbo.base); kfree(attachment); return ret; }

static void kfd_mem_detach(struct kfd_mem_attachment *attachment) { + struct amdgpu_bo *bo = attachment->bo_va->base.bo; + pr_debug("\t remove VA 0x%llx in entry %p\n", attachment->va, attachment); amdgpu_vm_bo_rmv(attachment->adev, attachment->bo_va); + drm_gem_object_put(&bo->tbo.base); list_del(&attachment->list); kfree(attachment); } -- 2.31.1

_______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...

Regards, Oak

On 2021-04-21, 9:31 PM, "amd-gfx on behalf of Felix Kuehling" <amd-gfx-bounces@lists.freedesktop.org on behalf of Felix.Kuehling@amd.com> wrote:

Do AQL queue double-mapping with a single attach call. That will make it easier to create per-GPU BOs later, to be shared between the two BO VA mappings on the same GPU.

Freeing the attachments is not necessary if map_to_gpu fails. These will be cleaned up when the kdg_mem object is destroyed in amdgpu_amdkfd_gpuvm_free_memory_of_gpu.

Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com --- .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 103 ++++++++---------- 1 file changed, 48 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index 34c9a2d0028e..fbd7e786b54e 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -486,70 +486,76 @@ static uint64_t get_pte_flags(struct amdgpu_device *adev, struct kgd_mem *mem) * 4a. Validate new page tables and directories */ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem, - struct amdgpu_vm *vm, bool is_aql, - struct kfd_mem_attachment **p_attachment) + struct amdgpu_vm *vm, bool is_aql) { unsigned long bo_size = mem->bo->tbo.base.size; uint64_t va = mem->va; - struct kfd_mem_attachment *attachment; - struct amdgpu_bo *bo; - int ret; + struct kfd_mem_attachment *attachment[2] = {NULL, NULL}; + struct amdgpu_bo *bo[2] = {NULL, NULL}; + int i, ret;

if (!va) { pr_err("Invalid VA when adding BO to VM\n"); return -EINVAL; }

- if (is_aql) - va += bo_size; - - attachment = kzalloc(sizeof(*attachment), GFP_KERNEL); - if (!attachment) - return -ENOMEM; + for (i = 0; i <= is_aql; i++) { + attachment[i] = kzalloc(sizeof(*attachment[i]), GFP_KERNEL); + if (unlikely(!attachment[i])) { + ret = -ENOMEM; + goto unwind; + }

- pr_debug("\t add VA 0x%llx - 0x%llx to vm %p\n", va, - va + bo_size, vm); + pr_debug("\t add VA 0x%llx - 0x%llx to vm %p\n", va, + va + bo_size, vm);

- /* FIXME: For now all attachments use the same BO. This is incorrect - * because one BO can only have one DMA mapping for one GPU. We need - * one BO per GPU, e.g. a DMABuf import with dynamic attachment. This - * will be addressed one BO-type at a time in subsequent patches. - */ - bo = mem->bo; - drm_gem_object_get(&bo->tbo.base); + /* FIXME: For now all attachments use the same BO. This is + * incorrect because one BO can only have one DMA mapping + * for one GPU. We need one BO per GPU, e.g. a DMABuf + * import with dynamic attachment. This will be addressed + * one BO-type at a time in subsequent patches. + */ + bo[i] = mem->bo; + drm_gem_object_get(&bo[i]->tbo.base);

- /* Add BO to VM internal data structures*/ - attachment->bo_va = amdgpu_vm_bo_add(adev, vm, bo); - if (!attachment->bo_va) { - ret = -EINVAL; - pr_err("Failed to add BO object to VM. ret == %d\n", - ret); - goto err_vmadd; - } + /* Add BO to VM internal data structures */ + attachment[i]->bo_va = amdgpu_vm_bo_add(adev, vm, bo[i]); Just for discussion. Are we allowed to add one bo twice to a vm? When I looked at amdgpu_vm_bo_base_init (called by amdgpu_vm_bo_add), line: bo->vm_bo = base; when you add the same bo to vm the second time, bo->vm_bo will be overwritten. I am not sure whether this will cause an issue later. This is not introduced by your code. The original code (calling kfd_mem_attach twice for aql) has the same problem. + if (unlikely(!attachment[i]->bo_va)) { + ret = -ENOMEM; + pr_err("Failed to add BO object to VM. ret == %d\n", + ret); + goto unwind; + }

- attachment->va = va; - attachment->pte_flags = get_pte_flags(adev, mem); - attachment->adev = adev; - list_add(&attachment->list, &mem->attachments); + attachment[i]->va = va; + attachment[i]->pte_flags = get_pte_flags(adev, mem); + attachment[i]->adev = adev; + list_add(&attachment[i]->list, &mem->attachments);

- if (p_attachment) - *p_attachment = attachment; + va += bo_size; + }

/* Allocate validate page tables if needed */ ret = vm_validate_pt_pd_bos(vm); if (unlikely(ret)) { pr_err("validate_pt_pd_bos() failed\n"); - goto err_alloc_pts; + goto unwind; }

return 0;

-err_alloc_pts: - amdgpu_vm_bo_rmv(adev, attachment->bo_va); - list_del(&attachment->list); -err_vmadd: - drm_gem_object_put(&bo->tbo.base); - kfree(attachment); +unwind: + for (; i >= 0; i--) { + if (!attachment[i]) + continue; + if (attachment[i]->bo_va) { + amdgpu_vm_bo_rmv(adev, attachment[i]->bo_va); + list_del(&attachment[i]->list); + } + if (bo[i]) + drm_gem_object_put(&bo[i]->tbo.base); + kfree(attachment[i]); + } return ret; }

@@ -1382,8 +1388,6 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu( uint32_t domain; struct kfd_mem_attachment *entry; struct bo_vm_reservation_context ctx; - struct kfd_mem_attachment *attachment = NULL; - struct kfd_mem_attachment *attachment_aql = NULL; unsigned long bo_size; bool is_invalid_userptr = false;

@@ -1433,15 +1437,9 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu( is_invalid_userptr = true;

if (!kfd_mem_is_attached(avm, mem)) { - ret = kfd_mem_attach(adev, mem, avm, false, &attachment); + ret = kfd_mem_attach(adev, mem, avm, mem->aql_queue); if (ret) goto attach_failed; - if (mem->aql_queue) { - ret = kfd_mem_attach(adev, mem, avm, true, - &attachment_aql); - if (ret) - goto attach_failed_aql; - } } else { ret = vm_validate_pt_pd_bos(avm); if (unlikely(ret)) @@ -1496,11 +1494,6 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu( goto out;

map_bo_to_gpuvm_failed: - if (attachment_aql) - kfd_mem_detach(attachment_aql); -attach_failed_aql: - if (attachment) - kfd_mem_detach(attachment); attach_failed: unreserve_bo_and_vms(&ctx, false, false); out: -- 2.31.1

_______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...

[AMD Official Use Only - Internal Distribution Only]

Acked-by: Ramesh Errabolu ramesh.errabolu@amd.com

-----Original Message----- From: amd-gfx amd-gfx-bounces@lists.freedesktop.org On Behalf Of Kuehling, Felix Sent: Friday, April 23, 2021 2:23 AM To: Zeng, Oak Oak.Zeng@amd.com; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [PATCH v2 04/10] drm/amdgpu: Simplify AQL queue mapping

Am 2021-04-22 um 9:33 p.m. schrieb Zeng, Oak:

Regards, Oak

On 2021-04-21, 9:31 PM, "amd-gfx on behalf of Felix Kuehling" <amd-gfx-bounces@lists.freedesktop.org on behalf of Felix.Kuehling@amd.com> wrote:

Do AQL queue double-mapping with a single attach call. That will make it
easier to create per-GPU BOs later, to be shared between the two BO VA
mappings on the same GPU.

Freeing the attachments is not necessary if map_to_gpu fails. These will be
cleaned up when the kdg_mem object is destroyed in
amdgpu_amdkfd_gpuvm_free_memory_of_gpu.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 103 ++++++++----------
 1 file changed, 48 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 34c9a2d0028e..fbd7e786b54e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -486,70 +486,76 @@ static uint64_t get_pte_flags(struct amdgpu_device *adev, struct kgd_mem *mem)
  * 4a.  Validate new page tables and directories
  */
 static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
-		struct amdgpu_vm *vm, bool is_aql,
-		struct kfd_mem_attachment **p_attachment)
+		struct amdgpu_vm *vm, bool is_aql)
 {
 	unsigned long bo_size = mem->bo->tbo.base.size;
 	uint64_t va = mem->va;
-	struct kfd_mem_attachment *attachment;
-	struct amdgpu_bo *bo;
-	int ret;
+	struct kfd_mem_attachment *attachment[2] = {NULL, NULL};
+	struct amdgpu_bo *bo[2] = {NULL, NULL};
+	int i, ret;

 	if (!va) {
 		pr_err("Invalid VA when adding BO to VM\n");
 		return -EINVAL;
 	}

-	if (is_aql)
-		va += bo_size;
-
-	attachment = kzalloc(sizeof(*attachment), GFP_KERNEL);
-	if (!attachment)
-		return -ENOMEM;
+	for (i = 0; i <= is_aql; i++) {
+		attachment[i] = kzalloc(sizeof(*attachment[i]), GFP_KERNEL);
+		if (unlikely(!attachment[i])) {
+			ret = -ENOMEM;
+			goto unwind;
+		}

-	pr_debug("\t add VA 0x%llx - 0x%llx to vm %p\n", va,
-			va + bo_size, vm);
+		pr_debug("\t add VA 0x%llx - 0x%llx to vm %p\n", va,
+			 va + bo_size, vm);

-	/* FIXME: For now all attachments use the same BO. This is incorrect
-	 * because one BO can only have one DMA mapping for one GPU. We need
-	 * one BO per GPU, e.g. a DMABuf import with dynamic attachment. This
-	 * will be addressed one BO-type at a time in subsequent patches.
-	 */
-	bo = mem->bo;
-	drm_gem_object_get(&bo->tbo.base);
+		/* FIXME: For now all attachments use the same BO. This is
+		 * incorrect because one BO can only have one DMA mapping
+		 * for one GPU. We need one BO per GPU, e.g. a DMABuf
+		 * import with dynamic attachment. This will be addressed
+		 * one BO-type at a time in subsequent patches.
+		 */
+		bo[i] = mem->bo;
+		drm_gem_object_get(&bo[i]->tbo.base);

-	/* Add BO to VM internal data structures*/
-	attachment->bo_va = amdgpu_vm_bo_add(adev, vm, bo);
-	if (!attachment->bo_va) {
-		ret = -EINVAL;
-		pr_err("Failed to add BO object to VM. ret == %d\n",
-				ret);
-		goto err_vmadd;
-	}
+		/* Add BO to VM internal data structures */
+		attachment[i]->bo_va = amdgpu_vm_bo_add(adev, vm, bo[i]);

Just for discussion. Are we allowed to add one bo twice to a vm? When I looked at amdgpu_vm_bo_base_init (called by amdgpu_vm_bo_add), line: bo->vm_bo = base; when you add the same bo to vm the second time, bo->vm_bo will be overwritten. I am not sure whether this will cause an issue later. This is not introduced by your code. The original code (calling kfd_mem_attach twice for aql) has the same problem.

If you just add one more line of context, you'll see that bo->vm_bo is the start of a single linked list of struct amdgpu_vm_bo_base. So adding a BO to a VM multiple times just extends that single-linked list:

base->next = bo->vm_bo;         bo->vm_bo = base;

Regards,   Felix

+		if (unlikely(!attachment[i]->bo_va)) {
+			ret = -ENOMEM;
+			pr_err("Failed to add BO object to VM. ret == %d\n",
+			       ret);
+			goto unwind;
+		}

-	attachment->va = va;
-	attachment->pte_flags = get_pte_flags(adev, mem);
-	attachment->adev = adev;
-	list_add(&attachment->list, &mem->attachments);
+		attachment[i]->va = va;
+		attachment[i]->pte_flags = get_pte_flags(adev, mem);
+		attachment[i]->adev = adev;
+		list_add(&attachment[i]->list, &mem->attachments);

-	if (p_attachment)
-		*p_attachment = attachment;
+		va += bo_size;
+	}

 	/* Allocate validate page tables if needed */
 	ret = vm_validate_pt_pd_bos(vm);
 	if (unlikely(ret)) {
 		pr_err("validate_pt_pd_bos() failed\n");
-		goto err_alloc_pts;
+		goto unwind;
 	}

 	return 0;

-err_alloc_pts:
-	amdgpu_vm_bo_rmv(adev, attachment->bo_va);
-	list_del(&attachment->list);
-err_vmadd:
-	drm_gem_object_put(&bo->tbo.base);
-	kfree(attachment);
+unwind:
+	for (; i >= 0; i--) {
+		if (!attachment[i])
+			continue;
+		if (attachment[i]->bo_va) {
+			amdgpu_vm_bo_rmv(adev, attachment[i]->bo_va);
+			list_del(&attachment[i]->list);
+		}
+		if (bo[i])
+			drm_gem_object_put(&bo[i]->tbo.base);
+		kfree(attachment[i]);
+	}
 	return ret;
 }

@@ -1382,8 +1388,6 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
 	uint32_t domain;
 	struct kfd_mem_attachment *entry;
 	struct bo_vm_reservation_context ctx;
-	struct kfd_mem_attachment *attachment = NULL;
-	struct kfd_mem_attachment *attachment_aql = NULL;
 	unsigned long bo_size;
 	bool is_invalid_userptr = false;

@@ -1433,15 +1437,9 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
 		is_invalid_userptr = true;

 	if (!kfd_mem_is_attached(avm, mem)) {
-		ret = kfd_mem_attach(adev, mem, avm, false, &attachment);
+		ret = kfd_mem_attach(adev, mem, avm, mem->aql_queue);
 		if (ret)
 			goto attach_failed;
-		if (mem->aql_queue) {
-			ret = kfd_mem_attach(adev, mem, avm, true,
-					     &attachment_aql);
-			if (ret)
-				goto attach_failed_aql;
-		}
 	} else {
 		ret = vm_validate_pt_pd_bos(avm);
 		if (unlikely(ret))
@@ -1496,11 +1494,6 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
 	goto out;

 map_bo_to_gpuvm_failed:
-	if (attachment_aql)
-		kfd_mem_detach(attachment_aql);
-attach_failed_aql:
-	if (attachment)
-		kfd_mem_detach(attachment);
 attach_failed:
 	unreserve_bo_and_vms(&ctx, false, false);
 out:
-- 
2.31.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist s.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cph ilip.yang%40amd.com%7Ca464efb3fc9e4139a80508d90628b3a0%7C3dd8961fe4884 e608e11a82d994e183d%7C0%7C0%7C637547594180231281%7CUnknown%7CTWFpbGZsb 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D% 7C1000&amp;sdata=D2seAdDqbmuCiDQzFTcv33Uyc8ELzu6zXxIralfCC8E%3D&amp;re served=0

_______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...

As I understand it, when one GPU map another GPU's vram, this vram should also be mapped in iommu page table. Also normal GTT memory (versus userptr) also need to be mapped in iommu. But don't see this code below. I only see you map userptr in iommu. Maybe you map them in iommu not during memory attachment time?

Also see a nit-pick inline

Regards, Oak

On 2021-04-21, 9:31 PM, "dri-devel on behalf of Felix Kuehling" <dri-devel-bounces@lists.freedesktop.org on behalf of Felix.Kuehling@amd.com> wrote:

Add BO-type specific helpers functions to DMA-map and unmap kfd_mem_attachments. Implement this functionality for userptrs by creating one SG BO per GPU and filling it with a DMA mapping of the pages from the original mem->bo.

Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 8 +- .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 146 +++++++++++++++++- 2 files changed, 145 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h index c24b2478f445..63668433f5a6 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h @@ -38,11 +38,17 @@ extern uint64_t amdgpu_amdkfd_total_mem_size;

struct amdgpu_device;

+enum kfd_mem_attachment_type { + KFD_MEM_ATT_SHARED, /* Share kgd_mem->bo or another attachment's */ + KFD_MEM_ATT_USERPTR, /* SG bo to DMA map pages from a userptr bo */ +}; + struct kfd_mem_attachment { struct list_head list; + enum kfd_mem_attachment_type type; + bool is_mapped; struct amdgpu_bo_va *bo_va; struct amdgpu_device *adev; - bool is_mapped; uint64_t va; uint64_t pte_flags; }; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index fbd7e786b54e..49d1af4aa5f1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -473,12 +473,117 @@ static uint64_t get_pte_flags(struct amdgpu_device *adev, struct kgd_mem *mem) return pte_flags; }

+static int +kfd_mem_dmamap_userptr(struct kgd_mem *mem, + struct kfd_mem_attachment *attachment) +{ + enum dma_data_direction direction = + mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ? + DMA_BIDIRECTIONAL : DMA_TO_DEVICE; + struct ttm_operation_ctx ctx = {.interruptible = true}; + struct amdgpu_bo *bo = attachment->bo_va->base.bo; + struct amdgpu_device *adev = attachment->adev; + struct ttm_tt *src_ttm = mem->bo->tbo.ttm; + struct ttm_tt *ttm = bo->tbo.ttm; + int ret; + + ttm->sg = kmalloc(sizeof(*ttm->sg), GFP_KERNEL); + if (unlikely(!ttm->sg)) + return -ENOMEM; + + if (WARN_ON(ttm->num_pages != src_ttm->num_pages)) + return -EINVAL; + + /* Same sequence as in amdgpu_ttm_tt_pin_userptr */ + ret = sg_alloc_table_from_pages(ttm->sg, src_ttm->pages, + ttm->num_pages, 0, + (u64)ttm->num_pages << PAGE_SHIFT, + GFP_KERNEL); + if (unlikely(ret)) + goto release_sg; Should go to a label starting from kfree below? + + ret = dma_map_sgtable(adev->dev, ttm->sg, direction, 0); + if (unlikely(ret)) + goto release_sg; + + drm_prime_sg_to_dma_addr_array(ttm->sg, ttm->dma_address, + ttm->num_pages); + + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT); + ret = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); + if (ret) + goto release_sg; + + return 0; + +release_sg: + pr_err("DMA map userptr failed: %d\n", ret); + sg_free_table(ttm->sg); + kfree(ttm->sg); + ttm->sg = NULL; + return ret; +} + +static int +kfd_mem_dmamap_attachment(struct kgd_mem *mem, + struct kfd_mem_attachment *attachment) +{ + switch (attachment->type) { + case KFD_MEM_ATT_SHARED: + return 0; + case KFD_MEM_ATT_USERPTR: + return kfd_mem_dmamap_userptr(mem, attachment); + default: + WARN_ON_ONCE(1); + } + return -EINVAL; +} + +static void +kfd_mem_dmaunmap_userptr(struct kgd_mem *mem, + struct kfd_mem_attachment *attachment) +{ + enum dma_data_direction direction = + mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ? + DMA_BIDIRECTIONAL : DMA_TO_DEVICE; + struct ttm_operation_ctx ctx = {.interruptible = false}; + struct amdgpu_bo *bo = attachment->bo_va->base.bo; + struct amdgpu_device *adev = attachment->adev; + struct ttm_tt *ttm = bo->tbo.ttm; + + if (unlikely(!ttm->sg)) + return; + + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_CPU); + ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); + + dma_unmap_sgtable(adev->dev, ttm->sg, direction, 0); + sg_free_table(ttm->sg); + ttm->sg = NULL; +} + +static void +kfd_mem_dmaunmap_attachment(struct kgd_mem *mem, + struct kfd_mem_attachment *attachment) +{ + switch (attachment->type) { + case KFD_MEM_ATT_SHARED: + break; + case KFD_MEM_ATT_USERPTR: + kfd_mem_dmaunmap_userptr(mem, attachment); + break; + default: + WARN_ON_ONCE(1); + } +} + /* kfd_mem_attach - Add a BO to a VM * * Everything that needs to bo done only once when a BO is first added * to a VM. It can later be mapped and unmapped many times without * repeating these steps. * + * 0. Create BO for DMA mapping, if needed * 1. Allocate and initialize BO VA entry data structure * 2. Add BO to the VM * 3. Determine ASIC-specific PTE flags @@ -488,10 +593,12 @@ static uint64_t get_pte_flags(struct amdgpu_device *adev, struct kgd_mem *mem) static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem, struct amdgpu_vm *vm, bool is_aql) { + struct amdgpu_device *bo_adev = amdgpu_ttm_adev(mem->bo->tbo.bdev); unsigned long bo_size = mem->bo->tbo.base.size; uint64_t va = mem->va; struct kfd_mem_attachment *attachment[2] = {NULL, NULL}; struct amdgpu_bo *bo[2] = {NULL, NULL}; + struct drm_gem_object *gobj; int i, ret;

if (!va) { @@ -509,14 +616,37 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem, pr_debug("\t add VA 0x%llx - 0x%llx to vm %p\n", va, va + bo_size, vm);

- /* FIXME: For now all attachments use the same BO. This is - * incorrect because one BO can only have one DMA mapping - * for one GPU. We need one BO per GPU, e.g. a DMABuf - * import with dynamic attachment. This will be addressed - * one BO-type at a time in subsequent patches. - */ - bo[i] = mem->bo; - drm_gem_object_get(&bo[i]->tbo.base); + if (adev == bo_adev || (mem->domain == AMDGPU_GEM_DOMAIN_VRAM && + amdgpu_xgmi_same_hive(adev, bo_adev))) { + /* Mappings on the local GPU and VRAM mappings in the + * local hive share the original BO + */ + attachment[i]->type = KFD_MEM_ATT_SHARED; + bo[i] = mem->bo; + drm_gem_object_get(&bo[i]->tbo.base); + } else if (i > 0) { + /* Multiple mappings on the same GPU share the BO */ + attachment[i]->type = KFD_MEM_ATT_SHARED; + bo[i] = bo[0]; + drm_gem_object_get(&bo[i]->tbo.base); + } else if (amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm)) { + /* Create an SG BO to DMA-map userptrs on other GPUs */ + attachment[i]->type = KFD_MEM_ATT_USERPTR; + ret = amdgpu_gem_object_create(adev, bo_size, 1, + AMDGPU_GEM_DOMAIN_CPU, + 0, ttm_bo_type_sg, + mem->bo->tbo.base.resv, + &gobj); + if (ret) + goto unwind; + bo[i] = gem_to_amdgpu_bo(gobj); + bo[i]->parent = amdgpu_bo_ref(mem->bo); + } else { + /* FIXME: Need to DMA-map other BO types */ + attachment[i]->type = KFD_MEM_ATT_SHARED; + bo[i] = mem->bo; + drm_gem_object_get(&bo[i]->tbo.base); + }

/* Add BO to VM internal data structures */ attachment[i]->bo_va = amdgpu_vm_bo_add(adev, vm, bo[i]); -- 2.31.1

_______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...

[AMD Official Use Only - Internal Distribution Only]

Acked-by: Ramesh Errabolu ramesh.errabolu@amd.com

-----Original Message----- From: amd-gfx amd-gfx-bounces@lists.freedesktop.org On Behalf Of Kuehling, Felix Sent: Monday, April 26, 2021 10:41 PM To: Zeng, Oak Oak.Zeng@amd.com; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [PATCH v2 05/10] drm/amdgpu: Add multi-GPU DMA mapping helpers

Am 2021-04-26 um 8:09 p.m. schrieb Zeng, Oak:

As I understand it, when one GPU map another GPU's vram, this vram should also be mapped in iommu page table. Also normal GTT memory (versus userptr) also need to be mapped in iommu. But don't see this code below.

Right, I'm not solving all problems at once. The next patch is there to handle GTT BOs.

Peer mappings of doorbells, MMIO and VRAM still need to be handled in the future. I'm trying to fix the worst issues first. This series should get 99% of real world tests working.

I only see you map userptr in iommu. Maybe you map them in iommu not during memory attachment time?

Also see a nit-pick inline

Regards, Oak

On 2021-04-21, 9:31 PM, "dri-devel on behalf of Felix Kuehling" <dri-devel-bounces@lists.freedesktop.org on behalf of Felix.Kuehling@amd.com> wrote:

Add BO-type specific helpers functions to DMA-map and unmap
kfd_mem_attachments. Implement this functionality for userptrs by creating
one SG BO per GPU and filling it with a DMA mapping of the pages from the
original mem->bo.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |   8 +-
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 146 +++++++++++++++++-
 2 files changed, 145 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index c24b2478f445..63668433f5a6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -38,11 +38,17 @@ extern uint64_t amdgpu_amdkfd_total_mem_size;

 struct amdgpu_device;

+enum kfd_mem_attachment_type {
+	KFD_MEM_ATT_SHARED,	/* Share kgd_mem->bo or another attachment's */
+	KFD_MEM_ATT_USERPTR,	/* SG bo to DMA map pages from a userptr bo */
+};
+
 struct kfd_mem_attachment {
 	struct list_head list;
+	enum kfd_mem_attachment_type type;
+	bool is_mapped;
 	struct amdgpu_bo_va *bo_va;
 	struct amdgpu_device *adev;
-	bool is_mapped;
 	uint64_t va;
 	uint64_t pte_flags;
 };
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index fbd7e786b54e..49d1af4aa5f1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -473,12 +473,117 @@ static uint64_t get_pte_flags(struct amdgpu_device *adev, struct kgd_mem *mem)
 	return pte_flags;
 }

+static int
+kfd_mem_dmamap_userptr(struct kgd_mem *mem,
+		       struct kfd_mem_attachment *attachment)
+{
+	enum dma_data_direction direction =
+		mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ?
+		DMA_BIDIRECTIONAL : DMA_TO_DEVICE;
+	struct ttm_operation_ctx ctx = {.interruptible = true};
+	struct amdgpu_bo *bo = attachment->bo_va->base.bo;
+	struct amdgpu_device *adev = attachment->adev;
+	struct ttm_tt *src_ttm = mem->bo->tbo.ttm;
+	struct ttm_tt *ttm = bo->tbo.ttm;
+	int ret;
+
+	ttm->sg = kmalloc(sizeof(*ttm->sg), GFP_KERNEL);
+	if (unlikely(!ttm->sg))
+		return -ENOMEM;
+
+	if (WARN_ON(ttm->num_pages != src_ttm->num_pages))
+		return -EINVAL;
+
+	/* Same sequence as in amdgpu_ttm_tt_pin_userptr */
+	ret = sg_alloc_table_from_pages(ttm->sg, src_ttm->pages,
+					ttm->num_pages, 0,
+					(u64)ttm->num_pages << PAGE_SHIFT,
+					GFP_KERNEL);
+	if (unlikely(ret))
+		goto release_sg;

Should go to a label starting from kfree below?

Thanks, I'll fix that.

Regards,   Felix

+
+	ret = dma_map_sgtable(adev->dev, ttm->sg, direction, 0);
+	if (unlikely(ret))
+		goto release_sg;
+
+	drm_prime_sg_to_dma_addr_array(ttm->sg, ttm->dma_address,
+				       ttm->num_pages);
+
+	amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT);
+	ret = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
+	if (ret)
+		goto release_sg;
+
+	return 0;
+
+release_sg:
+	pr_err("DMA map userptr failed: %d\n", ret);
+	sg_free_table(ttm->sg);
+	kfree(ttm->sg);
+	ttm->sg = NULL;
+	return ret;
+}
+
+static int
+kfd_mem_dmamap_attachment(struct kgd_mem *mem,
+			  struct kfd_mem_attachment *attachment)
+{
+	switch (attachment->type) {
+	case KFD_MEM_ATT_SHARED:
+		return 0;
+	case KFD_MEM_ATT_USERPTR:
+		return kfd_mem_dmamap_userptr(mem, attachment);
+	default:
+		WARN_ON_ONCE(1);
+	}
+	return -EINVAL;
+}
+
+static void
+kfd_mem_dmaunmap_userptr(struct kgd_mem *mem,
+			 struct kfd_mem_attachment *attachment)
+{
+	enum dma_data_direction direction =
+		mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ?
+		DMA_BIDIRECTIONAL : DMA_TO_DEVICE;
+	struct ttm_operation_ctx ctx = {.interruptible = false};
+	struct amdgpu_bo *bo = attachment->bo_va->base.bo;
+	struct amdgpu_device *adev = attachment->adev;
+	struct ttm_tt *ttm = bo->tbo.ttm;
+
+	if (unlikely(!ttm->sg))
+		return;
+
+	amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_CPU);
+	ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
+
+	dma_unmap_sgtable(adev->dev, ttm->sg, direction, 0);
+	sg_free_table(ttm->sg);
+	ttm->sg = NULL;
+}
+
+static void
+kfd_mem_dmaunmap_attachment(struct kgd_mem *mem,
+			    struct kfd_mem_attachment *attachment)
+{
+	switch (attachment->type) {
+	case KFD_MEM_ATT_SHARED:
+		break;
+	case KFD_MEM_ATT_USERPTR:
+		kfd_mem_dmaunmap_userptr(mem, attachment);
+		break;
+	default:
+		WARN_ON_ONCE(1);
+	}
+}
+
 /* kfd_mem_attach - Add a BO to a VM
  *
  * Everything that needs to bo done only once when a BO is first added
  * to a VM. It can later be mapped and unmapped many times without
  * repeating these steps.
  *
+ * 0. Create BO for DMA mapping, if needed
  * 1. Allocate and initialize BO VA entry data structure
  * 2. Add BO to the VM
  * 3. Determine ASIC-specific PTE flags
@@ -488,10 +593,12 @@ static uint64_t get_pte_flags(struct amdgpu_device *adev, struct kgd_mem *mem)
 static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
 		struct amdgpu_vm *vm, bool is_aql)
 {
+	struct amdgpu_device *bo_adev = amdgpu_ttm_adev(mem->bo->tbo.bdev);
 	unsigned long bo_size = mem->bo->tbo.base.size;
 	uint64_t va = mem->va;
 	struct kfd_mem_attachment *attachment[2] = {NULL, NULL};
 	struct amdgpu_bo *bo[2] = {NULL, NULL};
+	struct drm_gem_object *gobj;
 	int i, ret;

 	if (!va) {
@@ -509,14 +616,37 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
 		pr_debug("\t add VA 0x%llx - 0x%llx to vm %p\n", va,
 			 va + bo_size, vm);

-		/* FIXME: For now all attachments use the same BO. This is
-		 * incorrect because one BO can only have one DMA mapping
-		 * for one GPU. We need one BO per GPU, e.g. a DMABuf
-		 * import with dynamic attachment. This will be addressed
-		 * one BO-type at a time in subsequent patches.
-		 */
-		bo[i] = mem->bo;
-		drm_gem_object_get(&bo[i]->tbo.base);
+		if (adev == bo_adev || (mem->domain == AMDGPU_GEM_DOMAIN_VRAM &&
+					amdgpu_xgmi_same_hive(adev, bo_adev))) {
+			/* Mappings on the local GPU and VRAM mappings in the
+			 * local hive share the original BO
+			 */
+			attachment[i]->type = KFD_MEM_ATT_SHARED;
+			bo[i] = mem->bo;
+			drm_gem_object_get(&bo[i]->tbo.base);
+		} else if (i > 0) {
+			/* Multiple mappings on the same GPU share the BO */
+			attachment[i]->type = KFD_MEM_ATT_SHARED;
+			bo[i] = bo[0];
+			drm_gem_object_get(&bo[i]->tbo.base);
+		} else if (amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm)) {
+			/* Create an SG BO to DMA-map userptrs on other GPUs */
+			attachment[i]->type = KFD_MEM_ATT_USERPTR;
+			ret = amdgpu_gem_object_create(adev, bo_size, 1,
+						       AMDGPU_GEM_DOMAIN_CPU,
+						       0, ttm_bo_type_sg,
+						       mem->bo->tbo.base.resv,
+						       &gobj);
+			if (ret)
+				goto unwind;
+			bo[i] = gem_to_amdgpu_bo(gobj);
+			bo[i]->parent = amdgpu_bo_ref(mem->bo);
+		} else {
+			/* FIXME: Need to DMA-map other BO types */
+			attachment[i]->type = KFD_MEM_ATT_SHARED;
+			bo[i] = mem->bo;
+			drm_gem_object_get(&bo[i]->tbo.base);
+		}

 		/* Add BO to VM internal data structures */
 		attachment[i]->bo_va = amdgpu_vm_bo_add(adev, vm, bo[i]);
-- 
2.31.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist s.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=04%7C01%7C philip.yang%40amd.com%7C3766043c467041cc358408d9092e5990%7C3dd8961fe48 84e608e11a82d994e183d%7C0%7C0%7C637550916942474408%7CUnknown%7CTWFpbGZ sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3 D%7C1000&amp;sdata=H1H%2BCwSeLkcjiUMaE%2Fczj%2BYpBKuF%2Fhqnen0p9duG4UM %3D&amp;reserved=0

_______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...

Regards, Oak

On 2021-04-21, 9:31 PM, "dri-devel on behalf of Felix Kuehling" <dri-devel-bounces@lists.freedesktop.org on behalf of Felix.Kuehling@amd.com> wrote:

DMA map kfd_mem_attachments in update_gpuvm_pte. This function is called with the BO and page tables reserved, so we can safely update the DMA mapping.

DMA unmap when a BO is unmapped from a GPU and before updating mappings in restore workers.

Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com --- .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 56 ++++++++++--------- 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index 49d1af4aa5f1..7d25d886b98c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -961,11 +961,12 @@ static int unreserve_bo_and_vms(struct bo_vm_reservation_context *ctx, return ret; }

-static int unmap_bo_from_gpuvm(struct amdgpu_device *adev, +static void unmap_bo_from_gpuvm(struct kgd_mem *mem, struct kfd_mem_attachment *entry, struct amdgpu_sync *sync) { struct amdgpu_bo_va *bo_va = entry->bo_va; + struct amdgpu_device *adev = entry->adev; struct amdgpu_vm *vm = bo_va->base.vm;

amdgpu_vm_bo_unmap(adev, bo_va, entry->va); @@ -974,15 +975,20 @@ static int unmap_bo_from_gpuvm(struct amdgpu_device *adev,

amdgpu_sync_fence(sync, bo_va->last_pt_update);

- return 0; + kfd_mem_dmaunmap_attachment(mem, entry); }

-static int update_gpuvm_pte(struct amdgpu_device *adev, - struct kfd_mem_attachment *entry, - struct amdgpu_sync *sync) +static int update_gpuvm_pte(struct kgd_mem *mem, + struct kfd_mem_attachment *entry, + struct amdgpu_sync *sync) { - int ret; struct amdgpu_bo_va *bo_va = entry->bo_va; + struct amdgpu_device *adev = entry->adev; + int ret; + + ret = kfd_mem_dmamap_attachment(mem, entry); Should the dma mapping be done in the kfd_mem_attach function on a memory object is attached to a vm the first time? Since each memory object can be mapped to many GPU or many VMs, by doing dma mapping the first it is attached can simplify the logics. Or even simpler, maybe we can just just dma map when a memory object is created - it wastes some iommu page table entry but really simplify the logic in this patch series. I found this series is not very easy to understand. + if (ret) + return ret;

/* Update the page tables */ ret = amdgpu_vm_bo_update(adev, bo_va, false); @@ -994,14 +1000,15 @@ static int update_gpuvm_pte(struct amdgpu_device *adev, return amdgpu_sync_fence(sync, bo_va->last_pt_update); }

-static int map_bo_to_gpuvm(struct amdgpu_device *adev, - struct kfd_mem_attachment *entry, struct amdgpu_sync *sync, - bool no_update_pte) +static int map_bo_to_gpuvm(struct kgd_mem *mem, + struct kfd_mem_attachment *entry, + struct amdgpu_sync *sync, + bool no_update_pte) { int ret;

/* Set virtual address for the allocation */ - ret = amdgpu_vm_bo_map(adev, entry->bo_va, entry->va, 0, + ret = amdgpu_vm_bo_map(entry->adev, entry->bo_va, entry->va, 0, amdgpu_bo_size(entry->bo_va->base.bo), entry->pte_flags); if (ret) { @@ -1013,7 +1020,7 @@ static int map_bo_to_gpuvm(struct amdgpu_device *adev, if (no_update_pte) return 0;

- ret = update_gpuvm_pte(adev, entry, sync); + ret = update_gpuvm_pte(mem, entry, sync); if (ret) { pr_err("update_gpuvm_pte() failed\n"); goto update_gpuvm_pte_failed; @@ -1022,7 +1029,7 @@ static int map_bo_to_gpuvm(struct amdgpu_device *adev, return 0;

update_gpuvm_pte_failed: - unmap_bo_from_gpuvm(adev, entry, sync); + unmap_bo_from_gpuvm(mem, entry, sync); return ret; }

@@ -1596,7 +1603,7 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu( pr_debug("\t map VA 0x%llx - 0x%llx in entry %p\n", entry->va, entry->va + bo_size, entry);

- ret = map_bo_to_gpuvm(adev, entry, ctx.sync, + ret = map_bo_to_gpuvm(mem, entry, ctx.sync, is_invalid_userptr); if (ret) { pr_err("Failed to map bo to gpuvm\n"); @@ -1635,7 +1642,6 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu( int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu( struct kgd_dev *kgd, struct kgd_mem *mem, void *drm_priv) { - struct amdgpu_device *adev = get_amdgpu_device(kgd); struct amdgpu_vm *avm = drm_priv_to_vm(drm_priv); struct amdkfd_process_info *process_info = avm->process_info; unsigned long bo_size = mem->bo->tbo.base.size; @@ -1670,13 +1676,8 @@ int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu( pr_debug("\t unmap VA 0x%llx - 0x%llx from entry %p\n", entry->va, entry->va + bo_size, entry);

- ret = unmap_bo_from_gpuvm(adev, entry, ctx.sync); - if (ret == 0) { - entry->is_mapped = false; - } else { - pr_err("failed to unmap VA 0x%llx\n", mem->va); - goto unreserve_out; - } + unmap_bo_from_gpuvm(mem, entry, ctx.sync); + entry->is_mapped = false;

mem->mapped_to_gpu_memory--; pr_debug("\t DEC mapping count %d\n", @@ -2053,9 +2054,8 @@ static int validate_invalid_user_pages(struct amdkfd_process_info *process_info) if (!attachment->is_mapped) continue;

- ret = update_gpuvm_pte((struct amdgpu_device *) - attachment->adev, - attachment, &sync); + kfd_mem_dmaunmap_attachment(mem, attachment); + ret = update_gpuvm_pte(mem, attachment, &sync); if (ret) { pr_err("%s: update PTE failed\n", __func__); /* make sure this gets validated again */ @@ -2257,9 +2257,11 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef) goto validate_map_fail; } list_for_each_entry(attachment, &mem->attachments, list) { - ret = update_gpuvm_pte((struct amdgpu_device *) - attachment->adev, attachment, - &sync_obj); + if (!attachment->is_mapped) + continue; + + kfd_mem_dmaunmap_attachment(mem, attachment); + ret = update_gpuvm_pte(mem, attachment, &sync_obj); if (ret) { pr_debug("Memory eviction: update PTE failed. Try again\n"); goto validate_map_fail; -- 2.31.1

_______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...

[AMD Official Use Only - Internal Distribution Only]

Acked-by: Ramesh Errabolu ramesh.errabolu@amd.com

-----Original Message----- From: amd-gfx amd-gfx-bounces@lists.freedesktop.org On Behalf Of Kuehling, Felix Sent: Monday, April 26, 2021 10:48 PM To: Zeng, Oak Oak.Zeng@amd.com; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [PATCH v2 06/10] drm/amdgpu: DMA map/unmap when updating GPU mappings

Am 2021-04-26 um 8:23 p.m. schrieb Zeng, Oak:

Regards, Oak

On 2021-04-21, 9:31 PM, "dri-devel on behalf of Felix Kuehling" <dri-devel-bounces@lists.freedesktop.org on behalf of Felix.Kuehling@amd.com> wrote:

DMA map kfd_mem_attachments in update_gpuvm_pte. This function is called
with the BO and page tables reserved, so we can safely update the DMA
mapping.

DMA unmap when a BO is unmapped from a GPU and before updating mappings
in restore workers.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 56 ++++++++++---------
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 49d1af4aa5f1..7d25d886b98c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -961,11 +961,12 @@ static int unreserve_bo_and_vms(struct bo_vm_reservation_context *ctx,
 	return ret;
 }

-static int unmap_bo_from_gpuvm(struct amdgpu_device *adev,
+static void unmap_bo_from_gpuvm(struct kgd_mem *mem,
 				struct kfd_mem_attachment *entry,
 				struct amdgpu_sync *sync)
 {
 	struct amdgpu_bo_va *bo_va = entry->bo_va;
+	struct amdgpu_device *adev = entry->adev;
 	struct amdgpu_vm *vm = bo_va->base.vm;

 	amdgpu_vm_bo_unmap(adev, bo_va, entry->va);
@@ -974,15 +975,20 @@ static int unmap_bo_from_gpuvm(struct 

amdgpu_device *adev,

 	amdgpu_sync_fence(sync, bo_va->last_pt_update);

-	return 0;
+	kfd_mem_dmaunmap_attachment(mem, entry);
 }

-static int update_gpuvm_pte(struct amdgpu_device *adev,
-		struct kfd_mem_attachment *entry,
-		struct amdgpu_sync *sync)
+static int update_gpuvm_pte(struct kgd_mem *mem,
+			    struct kfd_mem_attachment *entry,
+			    struct amdgpu_sync *sync)
 {
-	int ret;
 	struct amdgpu_bo_va *bo_va = entry->bo_va;
+	struct amdgpu_device *adev = entry->adev;
+	int ret;
+
+	ret = kfd_mem_dmamap_attachment(mem, entry);

Should the dma mapping be done in the kfd_mem_attach function on a memory object is attached to a vm the first time? Since each memory object can be mapped to many GPU or many VMs, by doing dma mapping the first it is attached can simplify the logics. Or even simpler, maybe we can just just dma map when a memory object is created - it wastes some iommu page table entry but really simplify the logic in this patch series. I found this series is not very easy to understand.

The DMA mapping must be updated every time the physical memory allocation changes, e.g. after a BO was evicted and restored. Basically, if the physical pages of the BO change, we need to update the DMA mapping to point to those new pages. Therefore I added this in the update_gpu_vm_pte function, which is called after a BO has been validated the first time, or revalidated after an eviction.

You'll also see that I call dmaunmap in the re-validation cases (in the restore workers below) to ensure that we don't leak DMA mappings.

Regards,   Felix

+	if (ret)
+		return ret;

 	/* Update the page tables  */
 	ret = amdgpu_vm_bo_update(adev, bo_va, false);
@@ -994,14 +1000,15 @@ static int update_gpuvm_pte(struct amdgpu_device *adev,
 	return amdgpu_sync_fence(sync, bo_va->last_pt_update);
 }

-static int map_bo_to_gpuvm(struct amdgpu_device *adev,
-		struct kfd_mem_attachment *entry, struct amdgpu_sync *sync,
-		bool no_update_pte)
+static int map_bo_to_gpuvm(struct kgd_mem *mem,
+			   struct kfd_mem_attachment *entry,
+			   struct amdgpu_sync *sync,
+			   bool no_update_pte)
 {
 	int ret;

 	/* Set virtual address for the allocation */
-	ret = amdgpu_vm_bo_map(adev, entry->bo_va, entry->va, 0,
+	ret = amdgpu_vm_bo_map(entry->adev, entry->bo_va, entry->va, 0,
 			       amdgpu_bo_size(entry->bo_va->base.bo),
 			       entry->pte_flags);
 	if (ret) {
@@ -1013,7 +1020,7 @@ static int map_bo_to_gpuvm(struct amdgpu_device *adev,
 	if (no_update_pte)
 		return 0;

-	ret = update_gpuvm_pte(adev, entry, sync);
+	ret = update_gpuvm_pte(mem, entry, sync);
 	if (ret) {
 		pr_err("update_gpuvm_pte() failed\n");
 		goto update_gpuvm_pte_failed;
@@ -1022,7 +1029,7 @@ static int map_bo_to_gpuvm(struct amdgpu_device *adev,
 	return 0;

 update_gpuvm_pte_failed:
-	unmap_bo_from_gpuvm(adev, entry, sync);
+	unmap_bo_from_gpuvm(mem, entry, sync);
 	return ret;
 }

@@ -1596,7 +1603,7 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
 		pr_debug("\t map VA 0x%llx - 0x%llx in entry %p\n",
 			 entry->va, entry->va + bo_size, entry);

-		ret = map_bo_to_gpuvm(adev, entry, ctx.sync,
+		ret = map_bo_to_gpuvm(mem, entry, ctx.sync,
 				      is_invalid_userptr);
 		if (ret) {
 			pr_err("Failed to map bo to gpuvm\n");
@@ -1635,7 +1642,6 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
 int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu(
 		struct kgd_dev *kgd, struct kgd_mem *mem, void *drm_priv)
 {
-	struct amdgpu_device *adev = get_amdgpu_device(kgd);
 	struct amdgpu_vm *avm = drm_priv_to_vm(drm_priv);
 	struct amdkfd_process_info *process_info = avm->process_info;
 	unsigned long bo_size = mem->bo->tbo.base.size;
@@ -1670,13 +1676,8 @@ int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu(
 		pr_debug("\t unmap VA 0x%llx - 0x%llx from entry %p\n",
 			 entry->va, entry->va + bo_size, entry);

-		ret = unmap_bo_from_gpuvm(adev, entry, ctx.sync);
-		if (ret == 0) {
-			entry->is_mapped = false;
-		} else {
-			pr_err("failed to unmap VA 0x%llx\n", mem->va);
-			goto unreserve_out;
-		}
+		unmap_bo_from_gpuvm(mem, entry, ctx.sync);
+		entry->is_mapped = false;

 		mem->mapped_to_gpu_memory--;
 		pr_debug("\t DEC mapping count %d\n",
@@ -2053,9 +2054,8 @@ static int validate_invalid_user_pages(struct amdkfd_process_info *process_info)
 			if (!attachment->is_mapped)
 				continue;

-			ret = update_gpuvm_pte((struct amdgpu_device *)
-					       attachment->adev,
-					       attachment, &sync);
+			kfd_mem_dmaunmap_attachment(mem, attachment);
+			ret = update_gpuvm_pte(mem, attachment, &sync);
 			if (ret) {
 				pr_err("%s: update PTE failed\n", __func__);
 				/* make sure this gets validated again */
@@ -2257,9 +2257,11 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)
 			goto validate_map_fail;
 		}
 		list_for_each_entry(attachment, &mem->attachments, list) {
-			ret = update_gpuvm_pte((struct amdgpu_device *)
-					      attachment->adev, attachment,
-					      &sync_obj);
+			if (!attachment->is_mapped)
+				continue;
+
+			kfd_mem_dmaunmap_attachment(mem, attachment);
+			ret = update_gpuvm_pte(mem, attachment, &sync_obj);
 			if (ret) {
 				pr_debug("Memory eviction: update PTE failed. Try again\n");
 				goto validate_map_fail;
-- 
2.31.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist s.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&amp;data=04%7C01%7C philip.yang%40amd.com%7C81ace888057c4deed29308d9092f3d71%7C3dd8961fe48 84e608e11a82d994e183d%7C0%7C0%7C637550920765995756%7CUnknown%7CTWFpbGZ sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3 D%7C1000&amp;sdata=lsvr1xaDROEZrpHSNqD%2FCN4yDSccsM3WO2UnGXFXVLc%3D&am p;reserved=0

_______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...

[AMD Official Use Only - Internal Distribution Only]

Acked-by: Ramesh Errabolu ramesh.errabolu@amd.com

-----Original Message----- From: amd-gfx amd-gfx-bounces@lists.freedesktop.org On Behalf Of Kuehling, Felix Sent: Wednesday, April 21, 2021 8:31 PM To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: [PATCH v2 07/10] drm/amdgpu: Move kfd_mem_attach outside reservation

This is needed to avoid deadlocks with DMA buf import in the next patch. Also move PT/PD validation out of kfd_mem_attach, that way the caller can bo this unconditionally.

Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com --- .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 75 +++++++++++-------- 1 file changed, 44 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index 7d25d886b98c..9eeedd0c7920 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -577,6 +577,34 @@ kfd_mem_dmaunmap_attachment(struct kgd_mem *mem, } }

+static int +kfd_mem_attach_userptr(struct amdgpu_device *adev, struct kgd_mem *mem, + struct amdgpu_bo **bo) +{ + unsigned long bo_size = mem->bo->tbo.base.size; + struct drm_gem_object *gobj; + int ret; + + ret = amdgpu_bo_reserve(mem->bo, false); + if (ret) + return ret; + + ret = amdgpu_gem_object_create(adev, bo_size, 1, + AMDGPU_GEM_DOMAIN_CPU, + 0, ttm_bo_type_sg, + mem->bo->tbo.base.resv, + &gobj); + if (ret) + return ret; + + amdgpu_bo_unreserve(mem->bo); + + *bo = gem_to_amdgpu_bo(gobj); + (*bo)->parent = amdgpu_bo_ref(mem->bo); + + return 0; +} + /* kfd_mem_attach - Add a BO to a VM * * Everything that needs to bo done only once when a BO is first added @@ -598,7 +626,6 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem, uint64_t va = mem->va; struct kfd_mem_attachment *attachment[2] = {NULL, NULL}; struct amdgpu_bo *bo[2] = {NULL, NULL}; - struct drm_gem_object *gobj; int i, ret;

if (!va) { @@ -632,15 +659,9 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem, } else if (amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm)) { /* Create an SG BO to DMA-map userptrs on other GPUs */ attachment[i]->type = KFD_MEM_ATT_USERPTR; - ret = amdgpu_gem_object_create(adev, bo_size, 1, - AMDGPU_GEM_DOMAIN_CPU, - 0, ttm_bo_type_sg, - mem->bo->tbo.base.resv, - &gobj); + ret = kfd_mem_attach_userptr(adev, mem, &bo[i]); if (ret) goto unwind; - bo[i] = gem_to_amdgpu_bo(gobj); - bo[i]->parent = amdgpu_bo_ref(mem->bo); } else { /* FIXME: Need to DMA-map other BO types */ attachment[i]->type = KFD_MEM_ATT_SHARED; @@ -665,13 +686,6 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem, va += bo_size; }

- /* Allocate validate page tables if needed */ - ret = vm_validate_pt_pd_bos(vm); - if (unlikely(ret)) { - pr_err("validate_pt_pd_bos() failed\n"); - goto unwind; - } - return 0;

unwind: @@ -1478,12 +1492,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);

@@ -1560,6 +1574,12 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu( mem->va + bo_size * (1 + mem->aql_queue), avm, domain_string(domain));

+ if (!kfd_mem_is_attached(avm, mem)) { + ret = kfd_mem_attach(adev, mem, avm, mem->aql_queue); + if (ret) + goto out; + } + ret = reserve_bo_and_vm(mem, avm, &ctx); if (unlikely(ret)) goto out; @@ -1573,15 +1593,9 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu( bo->tbo.mem.mem_type == TTM_PL_SYSTEM) is_invalid_userptr = true;

- if (!kfd_mem_is_attached(avm, mem)) { - ret = kfd_mem_attach(adev, mem, avm, mem->aql_queue); - if (ret) - goto attach_failed; - } else { - ret = vm_validate_pt_pd_bos(avm); - if (unlikely(ret)) - goto attach_failed; - } + ret = vm_validate_pt_pd_bos(avm); + if (unlikely(ret)) + goto out_unreserve;

if (mem->mapped_to_gpu_memory == 0 && !amdgpu_ttm_tt_get_usermm(bo->tbo.ttm)) { @@ -1592,7 +1606,7 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu( ret = amdgpu_amdkfd_bo_validate(bo, domain, true); if (ret) { pr_debug("Validate failed\n"); - goto map_bo_to_gpuvm_failed; + goto out_unreserve; } }

@@ -1607,13 +1621,13 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu( is_invalid_userptr); if (ret) { pr_err("Failed to map bo to gpuvm\n"); - goto map_bo_to_gpuvm_failed; + goto out_unreserve; }

ret = vm_update_pds(avm, ctx.sync); if (ret) { pr_err("Failed to update page directories\n"); - goto map_bo_to_gpuvm_failed; + goto out_unreserve; }

entry->is_mapped = true; @@ -1630,8 +1644,7 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(

goto out;

-map_bo_to_gpuvm_failed: -attach_failed: +out_unreserve: unreserve_bo_and_vms(&ctx, false, false); out: mutex_unlock(&mem->process_info->lock); -- 2.31.1

_______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...

Regards, Oak

On 2021-04-21, 9:31 PM, "amd-gfx on behalf of Felix Kuehling" <amd-gfx-bounces@lists.freedesktop.org on behalf of Felix.Kuehling@amd.com> wrote:

Use DMABufs with dynamic attachment to DMA-map GTT BOs on other GPUs.

Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 2 + .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 76 ++++++++++++++++++- 2 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h index 63668433f5a6..b706e5a54782 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h @@ -41,6 +41,7 @@ struct amdgpu_device; enum kfd_mem_attachment_type { KFD_MEM_ATT_SHARED, /* Share kgd_mem->bo or another attachment's */ KFD_MEM_ATT_USERPTR, /* SG bo to DMA map pages from a userptr bo */ + KFD_MEM_ATT_DMABUF, /* DMAbuf to DMA map TTM BOs */ };

struct kfd_mem_attachment { @@ -56,6 +57,7 @@ struct kfd_mem_attachment { struct kgd_mem { struct mutex lock; struct amdgpu_bo *bo; + struct dma_buf *dmabuf; struct list_head attachments; /* protected by amdkfd_process_info.lock */ struct ttm_validate_buffer validate_list; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index 9eeedd0c7920..18a1f9222a59 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -524,6 +524,16 @@ kfd_mem_dmamap_userptr(struct kgd_mem *mem, return ret; }

+static int +kfd_mem_dmamap_dmabuf(struct kfd_mem_attachment *attachment) +{ + struct ttm_operation_ctx ctx = {.interruptible = true}; + struct amdgpu_bo *bo = attachment->bo_va->base.bo; + + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT); + return ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); How does this work? The function name says this is dma mapping a buffer but from the implementation, it is just a placement and validation +} + static int kfd_mem_dmamap_attachment(struct kgd_mem *mem, struct kfd_mem_attachment *attachment) @@ -533,6 +543,8 @@ kfd_mem_dmamap_attachment(struct kgd_mem *mem, return 0; case KFD_MEM_ATT_USERPTR: return kfd_mem_dmamap_userptr(mem, attachment); + case KFD_MEM_ATT_DMABUF: + return kfd_mem_dmamap_dmabuf(attachment); default: WARN_ON_ONCE(1); } @@ -562,6 +574,19 @@ kfd_mem_dmaunmap_userptr(struct kgd_mem *mem, ttm->sg = NULL; }

+static void +kfd_mem_dmaunmap_dmabuf(struct kfd_mem_attachment *attachment) +{ + struct ttm_operation_ctx ctx = {.interruptible = true}; + struct amdgpu_bo *bo = attachment->bo_va->base.bo; + + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_CPU); + ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); + /* FIXME: This does not guarantee that amdgpu_ttm_tt_unpopulate is + * called + */ +} + static void kfd_mem_dmaunmap_attachment(struct kgd_mem *mem, struct kfd_mem_attachment *attachment) @@ -572,6 +597,9 @@ kfd_mem_dmaunmap_attachment(struct kgd_mem *mem, case KFD_MEM_ATT_USERPTR: kfd_mem_dmaunmap_userptr(mem, attachment); break; + case KFD_MEM_ATT_DMABUF: + kfd_mem_dmaunmap_dmabuf(attachment); + break; default: WARN_ON_ONCE(1); } @@ -605,6 +633,38 @@ kfd_mem_attach_userptr(struct amdgpu_device *adev, struct kgd_mem *mem, return 0; }

+static int +kfd_mem_attach_dmabuf(struct amdgpu_device *adev, struct kgd_mem *mem, + struct amdgpu_bo **bo) +{ + struct drm_gem_object *gobj; + + if (!mem->dmabuf) { + mem->dmabuf = amdgpu_gem_prime_export(&mem->bo->tbo.base, + mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ? + DRM_RDWR : 0); + if (IS_ERR(mem->dmabuf)) { + mem->dmabuf = NULL; + return PTR_ERR(mem->dmabuf); + } + } + + gobj = amdgpu_gem_prime_import(&adev->ddev, mem->dmabuf); + if (IS_ERR(gobj)) + return PTR_ERR(gobj); + + /* Import takes an extra reference on the dmabuf. Drop it now to + * avoid leaking it. We only need the one reference in + * kgd_mem->dmabuf. + */ + dma_buf_put(mem->dmabuf); + + *bo = gem_to_amdgpu_bo(gobj); + (*bo)->parent = amdgpu_bo_ref(mem->bo); + + return 0; +} + /* kfd_mem_attach - Add a BO to a VM * * Everything that needs to bo done only once when a BO is first added @@ -662,8 +722,20 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem, ret = kfd_mem_attach_userptr(adev, mem, &bo[i]); if (ret) goto unwind; + } else if (mem->domain == AMDGPU_GEM_DOMAIN_GTT && + mem->bo->tbo.type != ttm_bo_type_sg) { + /* GTT BOs use DMA-mapping ability of dynamic-attach + * DMA bufs. TODO: The same should work for VRAM on + * large-BAR GPUs. + */ + attachment[i]->type = KFD_MEM_ATT_DMABUF; + ret = kfd_mem_attach_dmabuf(adev, mem, &bo[i]); + if (ret) + goto unwind; } else { - /* FIXME: Need to DMA-map other BO types */ + /* FIXME: Need to DMA-map other BO types: + * large-BAR VRAM, doorbells, MMIO remap + */ attachment[i]->type = KFD_MEM_ATT_SHARED; bo[i] = mem->bo; drm_gem_object_get(&bo[i]->tbo.base); @@ -1522,6 +1594,8 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(

/* Free the BO*/ drm_vma_node_revoke(&mem->bo->tbo.base.vma_node, drm_priv); + if (mem->dmabuf) + dma_buf_put(mem->dmabuf); drm_gem_object_put(&mem->bo->tbo.base); mutex_destroy(&mem->lock); kfree(mem); -- 2.31.1

_______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...

Regards, Oak

On 2021-04-26, 11:56 PM, "Kuehling, Felix" Felix.Kuehling@amd.com wrote:

Am 2021-04-26 um 8:35 p.m. schrieb Zeng, Oak: > Regards, > Oak > > > > On 2021-04-21, 9:31 PM, "amd-gfx on behalf of Felix Kuehling" <amd-gfx-bounces@lists.freedesktop.org on behalf of Felix.Kuehling@amd.com> wrote: > > Use DMABufs with dynamic attachment to DMA-map GTT BOs on other GPUs. > > Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 2 + > .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 76 ++++++++++++++++++- > 2 files changed, 77 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h > index 63668433f5a6..b706e5a54782 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h > @@ -41,6 +41,7 @@ struct amdgpu_device; > enum kfd_mem_attachment_type { > KFD_MEM_ATT_SHARED, /* Share kgd_mem->bo or another attachment's */ > KFD_MEM_ATT_USERPTR, /* SG bo to DMA map pages from a userptr bo */ > + KFD_MEM_ATT_DMABUF, /* DMAbuf to DMA map TTM BOs */ > }; > > struct kfd_mem_attachment { > @@ -56,6 +57,7 @@ struct kfd_mem_attachment { > struct kgd_mem { > struct mutex lock; > struct amdgpu_bo *bo; > + struct dma_buf *dmabuf; > struct list_head attachments; > /* protected by amdkfd_process_info.lock */ > struct ttm_validate_buffer validate_list; > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > index 9eeedd0c7920..18a1f9222a59 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > @@ -524,6 +524,16 @@ kfd_mem_dmamap_userptr(struct kgd_mem *mem, > return ret; > } > > +static int > +kfd_mem_dmamap_dmabuf(struct kfd_mem_attachment *attachment) > +{ > + struct ttm_operation_ctx ctx = {.interruptible = true}; > + struct amdgpu_bo *bo = attachment->bo_va->base.bo; > + > + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT); > + return ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); > How does this work? The function name says this is dma mapping a buffer but from the implementation, it is just a placement and validation

Conceptually, calling ttm_bo_validate ensures that the BO is in the specified domain, in this case GTT. Before calling validate, it can be in the CPU domain, which means it may be swapped to disk so it's not GPU accessible. For a DMABuf attachment, the CPU domain means, that the DMABuf is not attached because the underlying memory object may be on the move or swapped out.

The actual implementation of the dmabuf attachment is currently in amdgpu_ttm_populate/unpopulate. This is incorrect. Patch 10 in this series fixes that to move the actual dmabuf attachment into amdgpu_ttm_backend_bind/unbind, which is called from amdgpu_bo_move when a BO is moved between the CPU and GTT domains.

Thanks for the explanation. One more thing I don't quite understand: before this series, GTT memory should already has been validated somewhere before GTT memory is mapped to GPU. You added GTT memory validation here - will this validation be duplicated?

The function naming kfd_mem_dmamap_dmabuf is still confusing since it seems to me it is only some preparation work before dynamically dma-map a GTT memory. But I understand from this series' perspective, compared to usrptr (where you actually do the dma-mapping in function kfd_mem_dmamap_usrptr), for gtt memory you leveraged the amdgpu ttm function of dynamic dma-mapping. So maybe the naming here makes sense from that perspective.

Another thing related but not directly to this series: for GTT memory, it is dma-mapped when it is allocated. See function ttm_populate_and_map_pages calling dma_map_page. The question is, will gtt be first dma-unmapping before it is mapped in amdgpu_ttm_backend_bind? It is existing work, not from your series. Maybe there is not issue but I just want to make sure while we are looking at this area.

Regards, Felix

> +} > + > static int > kfd_mem_dmamap_attachment(struct kgd_mem *mem, > struct kfd_mem_attachment *attachment) > @@ -533,6 +543,8 @@ kfd_mem_dmamap_attachment(struct kgd_mem *mem, > return 0; > case KFD_MEM_ATT_USERPTR: > return kfd_mem_dmamap_userptr(mem, attachment); > + case KFD_MEM_ATT_DMABUF: > + return kfd_mem_dmamap_dmabuf(attachment); > default: > WARN_ON_ONCE(1); > } > @@ -562,6 +574,19 @@ kfd_mem_dmaunmap_userptr(struct kgd_mem *mem, > ttm->sg = NULL; > } > > +static void > +kfd_mem_dmaunmap_dmabuf(struct kfd_mem_attachment *attachment) > +{ > + struct ttm_operation_ctx ctx = {.interruptible = true}; > + struct amdgpu_bo *bo = attachment->bo_va->base.bo; > + > + amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_CPU); > + ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); > + /* FIXME: This does not guarantee that amdgpu_ttm_tt_unpopulate is > + * called > + */ > +} > + > static void > kfd_mem_dmaunmap_attachment(struct kgd_mem *mem, > struct kfd_mem_attachment *attachment) > @@ -572,6 +597,9 @@ kfd_mem_dmaunmap_attachment(struct kgd_mem *mem, > case KFD_MEM_ATT_USERPTR: > kfd_mem_dmaunmap_userptr(mem, attachment); > break; > + case KFD_MEM_ATT_DMABUF: > + kfd_mem_dmaunmap_dmabuf(attachment); > + break; > default: > WARN_ON_ONCE(1); > } > @@ -605,6 +633,38 @@ kfd_mem_attach_userptr(struct amdgpu_device *adev, struct kgd_mem *mem, > return 0; > } > > +static int > +kfd_mem_attach_dmabuf(struct amdgpu_device *adev, struct kgd_mem *mem, > + struct amdgpu_bo **bo) > +{ > + struct drm_gem_object *gobj; > + > + if (!mem->dmabuf) { > + mem->dmabuf = amdgpu_gem_prime_export(&mem->bo->tbo.base, > + mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ? > + DRM_RDWR : 0); > + if (IS_ERR(mem->dmabuf)) { > + mem->dmabuf = NULL; > + return PTR_ERR(mem->dmabuf); > + } > + } > + > + gobj = amdgpu_gem_prime_import(&adev->ddev, mem->dmabuf); > + if (IS_ERR(gobj)) > + return PTR_ERR(gobj); > + > + /* Import takes an extra reference on the dmabuf. Drop it now to > + * avoid leaking it. We only need the one reference in > + * kgd_mem->dmabuf. > + */ > + dma_buf_put(mem->dmabuf); > + > + *bo = gem_to_amdgpu_bo(gobj); > + (*bo)->parent = amdgpu_bo_ref(mem->bo); > + > + return 0; > +} > + > /* kfd_mem_attach - Add a BO to a VM > * > * Everything that needs to bo done only once when a BO is first added > @@ -662,8 +722,20 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem, > ret = kfd_mem_attach_userptr(adev, mem, &bo[i]); > if (ret) > goto unwind; > + } else if (mem->domain == AMDGPU_GEM_DOMAIN_GTT && > + mem->bo->tbo.type != ttm_bo_type_sg) { > + /* GTT BOs use DMA-mapping ability of dynamic-attach > + * DMA bufs. TODO: The same should work for VRAM on > + * large-BAR GPUs. > + */ > + attachment[i]->type = KFD_MEM_ATT_DMABUF; > + ret = kfd_mem_attach_dmabuf(adev, mem, &bo[i]); > + if (ret) > + goto unwind; > } else { > - /* FIXME: Need to DMA-map other BO types */ > + /* FIXME: Need to DMA-map other BO types: > + * large-BAR VRAM, doorbells, MMIO remap > + */ > attachment[i]->type = KFD_MEM_ATT_SHARED; > bo[i] = mem->bo; > drm_gem_object_get(&bo[i]->tbo.base); > @@ -1522,6 +1594,8 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu( > > /* Free the BO*/ > drm_vma_node_revoke(&mem->bo->tbo.base.vma_node, drm_priv); > + if (mem->dmabuf) > + dma_buf_put(mem->dmabuf); > drm_gem_object_put(&mem->bo->tbo.base); > mutex_destroy(&mem->lock); > kfree(mem); > -- > 2.31.1 > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx >

[AMD Official Use Only - Internal Distribution Only]

Acked-by: Ramesh Errabolu ramesh.errabolu@amd.com

-----Original Message----- From: amd-gfx amd-gfx-bounces@lists.freedesktop.org On Behalf Of Kuehling, Felix Sent: Tuesday, April 27, 2021 10:09 AM To: Zeng, Oak Oak.Zeng@amd.com; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: Re: [PATCH v2 08/10] drm/amdgpu: Add DMA mapping of GTT BOs

Am 2021-04-27 um 10:29 a.m. schrieb Zeng, Oak:

Regards, Oak

On 2021-04-26, 11:56 PM, "Kuehling, Felix" Felix.Kuehling@amd.com wrote:

Am 2021-04-26 um 8:35 p.m. schrieb Zeng, Oak:
> Regards,
> Oak 
>
>  
>
> On 2021-04-21, 9:31 PM, "amd-gfx on behalf of Felix Kuehling" <amd-gfx-bounces@lists.freedesktop.org on behalf of Felix.Kuehling@amd.com> wrote:
>
>     Use DMABufs with dynamic attachment to DMA-map GTT BOs on other GPUs.
>
>     Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>     ---
>      drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  2 +
>      .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 76 ++++++++++++++++++-
>      2 files changed, 77 insertions(+), 1 deletion(-)
>
>     diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>     index 63668433f5a6..b706e5a54782 100644
>     --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>     +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
>     @@ -41,6 +41,7 @@ struct amdgpu_device;
>      enum kfd_mem_attachment_type {
>      	KFD_MEM_ATT_SHARED,	/* Share kgd_mem->bo or another attachment's */
>      	KFD_MEM_ATT_USERPTR,	/* SG bo to DMA map pages from a userptr bo */
>     +	KFD_MEM_ATT_DMABUF,	/* DMAbuf to DMA map TTM BOs */
>      };
>
>      struct kfd_mem_attachment {
>     @@ -56,6 +57,7 @@ struct kfd_mem_attachment {
>      struct kgd_mem {
>      	struct mutex lock;
>      	struct amdgpu_bo *bo;
>     +	struct dma_buf *dmabuf;
>      	struct list_head attachments;
>      	/* protected by amdkfd_process_info.lock */
>      	struct ttm_validate_buffer validate_list;
>     diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>     index 9eeedd0c7920..18a1f9222a59 100644
>     --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>     +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>     @@ -524,6 +524,16 @@ kfd_mem_dmamap_userptr(struct kgd_mem *mem,
>      	return ret;
>      }
>
>     +static int
>     +kfd_mem_dmamap_dmabuf(struct kfd_mem_attachment *attachment)
>     +{
>     +	struct ttm_operation_ctx ctx = {.interruptible = true};
>     +	struct amdgpu_bo *bo = attachment->bo_va->base.bo;
>     +
>     +	amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT);
>     +	return ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
> How does this work? The function name says this is dma mapping a 

buffer but from the implementation, it is just a placement and validation

Conceptually, calling ttm_bo_validate ensures that the BO is in the
specified domain, in this case GTT. Before calling validate, it can be
in the CPU domain, which means it may be swapped to disk so it's not GPU
accessible. For a DMABuf attachment, the CPU domain means, that the
DMABuf is not attached because the underlying memory object may be on
the move or swapped out.

The actual implementation of the dmabuf attachment is currently in
amdgpu_ttm_populate/unpopulate. This is incorrect. Patch 10 in this
series fixes that to move the actual dmabuf attachment into
amdgpu_ttm_backend_bind/unbind, which is called from amdgpu_bo_move when
a BO is moved between the CPU and GTT domains.

Thanks for the explanation. One more thing I don't quite understand: before this series, GTT memory should already has been validated somewhere before GTT memory is mapped to GPU. You added GTT memory validation here - will this validation be duplicated?

When you have N GPUs there are now N BOs involved. Each GPU needs its own BO because it needs its own DMA mapping. There will be one actual GTT BO that allocates physical pages in TTM. The other BOs are dmabuf imports that DMA-map the same physical pages for access by the other GPUs.

The validate call here validates one of the dmabuf imports. This does not duplicate the validation of the underlying TTM BO with the actual physical memory allocation.

The function naming kfd_mem_dmamap_dmabuf is still confusing since it seems to me it is only some preparation work before dynamically dma-map a GTT memory.

No, this series is not just preparation. It implements DMA mapping of BOs for multiple GPUs. TTM already handles DMA mapping of the memory for the device where the memory was allocated. (Yes, even GTT memory is associated with a specific GPU even though it's physically in system memory). What this patch series adds, is additional DMA mappings for the other GPUs. Without this patch, we were using the DMA mapping for GPU-1 in the page table of GPU-X, which is incorrect. It works in many cases where the DMA mapping is a direct mapping:

* IOMMU disabled * IOMMU in passthrough mode

But it breaks when you have multiple GPUs with an IOMMU that's not disabled or in passthrough mode.

But I understand from this series' perspective, compared to usrptr (where you actually do the dma-mapping in function kfd_mem_dmamap_usrptr), for gtt memory you leveraged the amdgpu ttm function of dynamic dma-mapping. So maybe the naming here makes sense from that perspective.

Yes.

Another thing related but not directly to this series: for GTT memory, it is dma-mapped when it is allocated. See function ttm_populate_and_map_pages calling dma_map_page. The question is, will gtt be first dma-unmapping before it is mapped in amdgpu_ttm_backend_bind? It is existing work, not from your series. Maybe there is not issue but I just want to make sure while we are looking at this area.

Right. The problem is, that the DMA mappings only work for a specific device. Using the same DMA mapping on multiple devices is broken. The reason we got away with it for a long time is, that we were running with IOMMU disabled or in passthrough mode.

Regards,   Felix

Regards,
  Felix


>     +}
>     +
>      static int
>      kfd_mem_dmamap_attachment(struct kgd_mem *mem,
>      			  struct kfd_mem_attachment *attachment)
>     @@ -533,6 +543,8 @@ kfd_mem_dmamap_attachment(struct kgd_mem *mem,
>      		return 0;
>      	case KFD_MEM_ATT_USERPTR:
>      		return kfd_mem_dmamap_userptr(mem, attachment);
>     +	case KFD_MEM_ATT_DMABUF:
>     +		return kfd_mem_dmamap_dmabuf(attachment);
>      	default:
>      		WARN_ON_ONCE(1);
>      	}
>     @@ -562,6 +574,19 @@ kfd_mem_dmaunmap_userptr(struct kgd_mem *mem,
>      	ttm->sg = NULL;
>      }
>
>     +static void
>     +kfd_mem_dmaunmap_dmabuf(struct kfd_mem_attachment *attachment)
>     +{
>     +	struct ttm_operation_ctx ctx = {.interruptible = true};
>     +	struct amdgpu_bo *bo = attachment->bo_va->base.bo;
>     +
>     +	amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_CPU);
>     +	ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
>     +	/* FIXME: This does not guarantee that amdgpu_ttm_tt_unpopulate is
>     +	 * called
>     +	 */
>     +}
>     +
>      static void
>      kfd_mem_dmaunmap_attachment(struct kgd_mem *mem,
>      			    struct kfd_mem_attachment *attachment)
>     @@ -572,6 +597,9 @@ kfd_mem_dmaunmap_attachment(struct kgd_mem *mem,
>      	case KFD_MEM_ATT_USERPTR:
>      		kfd_mem_dmaunmap_userptr(mem, attachment);
>      		break;
>     +	case KFD_MEM_ATT_DMABUF:
>     +		kfd_mem_dmaunmap_dmabuf(attachment);
>     +		break;
>      	default:
>      		WARN_ON_ONCE(1);
>      	}
>     @@ -605,6 +633,38 @@ kfd_mem_attach_userptr(struct amdgpu_device *adev, struct kgd_mem *mem,
>      	return 0;
>      }
>
>     +static int
>     +kfd_mem_attach_dmabuf(struct amdgpu_device *adev, struct kgd_mem *mem,
>     +		      struct amdgpu_bo **bo)
>     +{
>     +	struct drm_gem_object *gobj;
>     +
>     +	if (!mem->dmabuf) {
>     +		mem->dmabuf = amdgpu_gem_prime_export(&mem->bo->tbo.base,
>     +			mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ?
>     +				DRM_RDWR : 0);
>     +		if (IS_ERR(mem->dmabuf)) {
>     +			mem->dmabuf = NULL;
>     +			return PTR_ERR(mem->dmabuf);
>     +		}
>     +	}
>     +
>     +	gobj = amdgpu_gem_prime_import(&adev->ddev, mem->dmabuf);
>     +	if (IS_ERR(gobj))
>     +		return PTR_ERR(gobj);
>     +
>     +	/* Import takes an extra reference on the dmabuf. Drop it now to
>     +	 * avoid leaking it. We only need the one reference in
>     +	 * kgd_mem->dmabuf.
>     +	 */
>     +	dma_buf_put(mem->dmabuf);
>     +
>     +	*bo = gem_to_amdgpu_bo(gobj);
>     +	(*bo)->parent = amdgpu_bo_ref(mem->bo);
>     +
>     +	return 0;
>     +}
>     +
>      /* kfd_mem_attach - Add a BO to a VM
>       *
>       * Everything that needs to bo done only once when a BO is first added
>     @@ -662,8 +722,20 @@ static int kfd_mem_attach(struct amdgpu_device *adev, struct kgd_mem *mem,
>      			ret = kfd_mem_attach_userptr(adev, mem, &bo[i]);
>      			if (ret)
>      				goto unwind;
>     +		} else if (mem->domain == AMDGPU_GEM_DOMAIN_GTT &&
>     +			   mem->bo->tbo.type != ttm_bo_type_sg) {
>     +			/* GTT BOs use DMA-mapping ability of dynamic-attach
>     +			 * DMA bufs. TODO: The same should work for VRAM on
>     +			 * large-BAR GPUs.
>     +			 */
>     +			attachment[i]->type = KFD_MEM_ATT_DMABUF;
>     +			ret = kfd_mem_attach_dmabuf(adev, mem, &bo[i]);
>     +			if (ret)
>     +				goto unwind;
>      		} else {
>     -			/* FIXME: Need to DMA-map other BO types */
>     +			/* FIXME: Need to DMA-map other BO types:
>     +			 * large-BAR VRAM, doorbells, MMIO remap
>     +			 */
>      			attachment[i]->type = KFD_MEM_ATT_SHARED;
>      			bo[i] = mem->bo;
>      			drm_gem_object_get(&bo[i]->tbo.base);
>     @@ -1522,6 +1594,8 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
>
>      	/* Free the BO*/
>      	drm_vma_node_revoke(&mem->bo->tbo.base.vma_node, drm_priv);
>     +	if (mem->dmabuf)
>     +		dma_buf_put(mem->dmabuf);
>      	drm_gem_object_put(&mem->bo->tbo.base);
>      	mutex_destroy(&mem->lock);
>      	kfree(mem);
>     -- 
>     2.31.1
>
>     _______________________________________________
>     amd-gfx mailing list
>     amd-gfx@lists.freedesktop.org
>     https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Cphilip.yang%40amd.com%7Cf3c9c824ef6447cbe26808d9098e6606%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637551329471854747%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=P7SYxlhPoYfyGCNaiiCA%2BaqJS%2BxvGZEMIZkuvqCpCLI%3D&reserved=0
>

_______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...

[AMD Official Use Only - Internal Distribution Only]

Acked-by: Ramesh Errabolu ramesh.errabolu@amd.com

-----Original Message----- From: amd-gfx amd-gfx-bounces@lists.freedesktop.org On Behalf Of Kuehling, Felix Sent: Wednesday, April 21, 2021 8:31 PM To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Subject: [PATCH v2 09/10] drm/ttm: Don't count pages in SG BOs against pages_limit

Pages in SG BOs were not allocated by TTM. So don't count them against TTM's pages limit.

Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com --- drivers/gpu/drm/ttm/ttm_tt.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c index 5d8820725b75..e8b8c3257392 100644 --- a/drivers/gpu/drm/ttm/ttm_tt.c +++ b/drivers/gpu/drm/ttm/ttm_tt.c @@ -317,9 +317,12 @@ int ttm_tt_populate(struct ttm_device *bdev, if (ttm_tt_is_populated(ttm)) return 0;

- atomic_long_add(ttm->num_pages, &ttm_pages_allocated); - if (bdev->pool.use_dma32) - atomic_long_add(ttm->num_pages, &ttm_dma32_pages_allocated); + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { + atomic_long_add(ttm->num_pages, &ttm_pages_allocated); + if (bdev->pool.use_dma32) + atomic_long_add(ttm->num_pages, + &ttm_dma32_pages_allocated); + }

while (atomic_long_read(&ttm_pages_allocated) > ttm_pages_limit || atomic_long_read(&ttm_dma32_pages_allocated) > @@ -350,9 +353,12 @@ int ttm_tt_populate(struct ttm_device *bdev, return 0;

error: - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); - if (bdev->pool.use_dma32) - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); + if (bdev->pool.use_dma32) + atomic_long_sub(ttm->num_pages, + &ttm_dma32_pages_allocated); + } return ret; } EXPORT_SYMBOL(ttm_tt_populate); @@ -382,9 +388,12 @@ void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm) else ttm_pool_free(&bdev->pool, ttm);

- atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); - if (bdev->pool.use_dma32) - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); + if (bdev->pool.use_dma32) + atomic_long_sub(ttm->num_pages, + &ttm_dma32_pages_allocated); + }

ttm->page_flags &= ~TTM_PAGE_FLAG_PRIV_POPULATED; } -- 2.31.1

_______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.free...

Am 22.04.21 um 03:30 schrieb Felix Kuehling:

The dmabuf attachment should be updated by moving the SG BO to DOMAIN_CPU and back to DOMAIN_GTT. This does not necessarily invoke the populate/unpopulate callbacks. Do this in backend_bind/unbind instead.

Signed-off-by: Felix Kuehling Felix.Kuehling@amd.com

Reviewed-by: Christian König christian.koenig@amd.com

---

.../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 3 -- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 51 +++++++++---------- 2 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index 18a1f9222a59..68e6ce8dcf33 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -582,9 +582,6 @@ kfd_mem_dmaunmap_dmabuf(struct kfd_mem_attachment *attachment)

amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_CPU); ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);

• /* FIXME: This does not guarantee that amdgpu_ttm_tt_unpopulate is

• * called
  

• */
  

  }

  static void

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 7e7d8330d64b..fc2a8d681dbc 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -910,7 +910,23 @@ static int amdgpu_ttm_backend_bind(struct ttm_device *bdev, DRM_ERROR("failed to pin userptr\n"); return r; }

• } else if (ttm->page_flags & TTM_PAGE_FLAG_SG) {

• if (!ttm->sg) {
  

• 	struct dma_buf_attachment *attach;
  

• 	struct sg_table *sgt;
  

• 	attach = gtt->gobj->import_attach;
  

• 	sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
  

• 	if (IS_ERR(sgt))
  

• 		return PTR_ERR(sgt);
  

• 	ttm->sg = sgt;
  

• }
  

• drm_prime_sg_to_dma_addr_array(ttm->sg, gtt->ttm.dma_address,
  

• 			       ttm->num_pages);
  

  }
• if (!ttm->num_pages) { WARN(1, "nothing to bind %u pages for mreg %p back %p!\n", ttm->num_pages, bo_mem, ttm);

@@ -1037,8 +1053,15 @@ static void amdgpu_ttm_backend_unbind(struct ttm_device *bdev, int r;

/* if the pages have userptr pinning then clear that first */

• if (gtt->userptr)

• if (gtt->userptr) { amdgpu_ttm_tt_unpin_userptr(bdev, ttm);

• } else if (ttm->sg && gtt->gobj->import_attach) {

• struct dma_buf_attachment *attach;
  

• attach = gtt->gobj->import_attach;
  

• dma_buf_unmap_attachment(attach, ttm->sg, DMA_BIDIRECTIONAL);
  

• ttm->sg = NULL;
  

• }

  if (!gtt->bound) return;

@@ -1125,23 +1148,8 @@ static int amdgpu_ttm_tt_populate(struct ttm_device *bdev, return 0; }

• if (ttm->page_flags & TTM_PAGE_FLAG_SG) {

• if (!ttm->sg) {
  

• 	struct dma_buf_attachment *attach;
  

• 	struct sg_table *sgt;
  

• 	attach = gtt->gobj->import_attach;
  

• 	sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
  

• 	if (IS_ERR(sgt))
  

• 		return PTR_ERR(sgt);
  

• 	ttm->sg = sgt;
  

• }
  

• drm_prime_sg_to_dma_addr_array(ttm->sg, gtt->ttm.dma_address,
  

• 			       ttm->num_pages);
  

• if (ttm->page_flags & TTM_PAGE_FLAG_SG) return 0;

• }

  return ttm_pool_alloc(&adev->mman.bdev.pool, ttm, ctx); }

@@ -1165,15 +1173,6 @@ static void amdgpu_ttm_tt_unpopulate(struct ttm_device *bdev, return; }

• if (ttm->sg && gtt->gobj->import_attach) {

• struct dma_buf_attachment *attach;
  

• attach = gtt->gobj->import_attach;
  

• dma_buf_unmap_attachment(attach, ttm->sg, DMA_BIDIRECTIONAL);
  

• ttm->sg = NULL;
  

• return;
  

• }
• if (ttm->page_flags & TTM_PAGE_FLAG_SG) return;