[AMD Official Use Only - Internal Distribution Only]
Acked-by: Ramesh Errabolu <ramesh.errabolu@xxxxxxx>
-----Original Message-----
From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Kuehling, Felix
Sent: Monday, April 26, 2021 10:48 PM
To: Zeng, Oak <Oak.Zeng@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx
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@xxxxxxxxxxxxxxxxxxxxx on behalf of Felix.Kuehling@xxxxxxx> 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@xxxxxxx>
> ---
> .../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@xxxxxxxxxxxxxxxxxxxxx
>
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flist
> s.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=04%7C01%7C
> philip.yang%40amd.com%7C81ace888057c4deed29308d9092f3d71%7C3dd8961fe48
> 84e608e11a82d994e183d%7C0%7C0%7C637550920765995756%7CUnknown%7CTWFpbGZ
> sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> D%7C1000&sdata=lsvr1xaDROEZrpHSNqD%2FCN4yDSccsM3WO2UnGXFXVLc%3D&am
> p;reserved=0
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Cphilip.yang%40amd.com%7C81ace888057c4deed29308d9092f3d71%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637550920766005746%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=hfBbfa5v8vjLGxacmrsOS3nLZ72ORvakcGUnHEDBQig%3D&reserved=0
