[AMD Official Use Only - General]
My response is inline.
Regards,
Ramesh
-----Original Message-----
From: Kuehling, Felix <Felix.Kuehling@xxxxxxx>
Sent: Thursday, June 9, 2022 1:10 AM
To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Errabolu, Ramesh <Ramesh.Errabolu@xxxxxxx>
Subject: Re: [PATCH] drm/amdgpu: Unpin MMIO and DOORBELL BOs only after map count goes to zero
On 2022-06-08 07:51, Ramesh Errabolu wrote:
> In existing code MMIO and DOORBELL BOs are unpinned without ensuring
> the condition that their map count has reached zero. Unpinning without
> checking this constraint could lead to an error while BO is being
> freed. The patch fixes this issue.
>
> Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index a1de900ba677..e5dc94b745b1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -1832,13 +1832,6 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
>
> mutex_lock(&mem->lock);
>
> - /* Unpin MMIO/DOORBELL BO's that were pinned during allocation */
> - if (mem->alloc_flags &
> - (KFD_IOC_ALLOC_MEM_FLAGS_DOORBELL |
> - KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP)) {
> - amdgpu_amdkfd_gpuvm_unpin_bo(mem->bo);
> - }
> -
> mapped_to_gpu_memory = mem->mapped_to_gpu_memory;
> is_imported = mem->is_imported;
> mutex_unlock(&mem->lock);
> @@ -1855,7 +1848,7 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
> /* Make sure restore workers don't access the BO any more */
> bo_list_entry = &mem->validate_list;
> mutex_lock(&process_info->lock);
> - list_del(&bo_list_entry->head);
> + list_del_init(&bo_list_entry->head);
Is this an unrelated fix? What is this needed for? I vaguely remember discussing this before, but can't remember the reason.
Ramesh: This fix is unrelated to P2P work. I brought this issue to attention while working on IOMMU support on DKMS branch. Basically a user could call free() before the map count goes to zero. The patch is trying fix that.
Regards,
Felix
> mutex_unlock(&process_info->lock);
>
> /* No more MMU notifiers */
> @@ -1880,6 +1873,12 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
>
> ret = unreserve_bo_and_vms(&ctx, false, false);
>
> + /* Unpin MMIO/DOORBELL BO's that were pinned during allocation */
> + if (mem->alloc_flags &
> + (KFD_IOC_ALLOC_MEM_FLAGS_DOORBELL |
> + KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP))
> + amdgpu_amdkfd_gpuvm_unpin_bo(mem->bo);
> +
> /* Free the sync object */
> amdgpu_sync_free(&mem->sync);
>
