[AMD Official Use Only - Internal Distribution Only]
Acked-by: Ramesh Errabolu <ramesh.errab...@amd.com>
-----Original Message-----
From: amd-gfx <amd-gfx-boun...@lists.freedesktop.org> On Behalf Of Kuehling,
Felix
Sent: Monday, April 26, 2021 10:48 PM
To: Zeng, Oak <oak.z...@amd.com>; amd-...@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-boun...@lists.freedesktop.org on behalf of felix.kuehl...@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.kuehl...@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&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-...@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%7C81ace888057c4deed29308d9092f3d71%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637550920766005746%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=hfBbfa5v8vjLGxacmrsOS3nLZ72ORvakcGUnHEDBQig%3D&reserved=0
