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://lists.freedesktop.org/mailman/listinfo/dri-devel
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
