Re: [PATCH 4/6] drm/amdkfd: Export DMABufs from KFD using GEM handles

[Felix Kuehling <felix.kuehling@xxxxxxx>]

On 2023-11-07 11:58, Felix Kuehling wrote:
> Create GEM handles for exporting DMABufs using GEM-Prime APIs. The GEM
> handles are created in a drm_client_dev context to avoid exposing them
> in user mode contexts through a DMABuf import.
This patch (and the next one) won't apply upstream because Thomas 
Zimmerman just made drm_gem_prime_fd_to_handle and 
drm_gem_prime_handle_to_fd private because nobody was using them. 
(drm/prime: Unexport helpers for fd/handle conversion)

Is it OK to export those APIs again? Or is there a better way for 
drivers to export/import DMABufs without using the GEM ioctls?

Regards,
  Felix


> Signed-off-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c    | 11 +++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  5 +++
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 33 +++++++++++++++----
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      |  4 +--
>   4 files changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> index 6ab17330a6ed..b0a67f16540a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -142,6 +142,7 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
>   {
>   	int i;
>   	int last_valid_bit;
> +	int ret;
>   
>   	amdgpu_amdkfd_gpuvm_init_mem_limits();
>   
> @@ -160,6 +161,12 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
>   			.enable_mes = adev->enable_mes,
>   		};
>   
> +		ret = drm_client_init(&adev->ddev, &adev->kfd.client, "kfd", NULL);
> +		if (ret) {
> +			dev_err(adev->dev, "Failed to init DRM client: %d\n", ret);
> +			return;
> +		}
> +
>   		/* this is going to have a few of the MSBs set that we need to
>   		 * clear
>   		 */
> @@ -198,6 +205,10 @@ void amdgpu_amdkfd_device_init(struct amdgpu_device *adev)
>   
>   		adev->kfd.init_complete = kgd2kfd_device_init(adev->kfd.dev,
>   							&gpu_resources);
> +		if (adev->kfd.init_complete)
> +			drm_client_register(&adev->kfd.client);
> +		else
> +			drm_client_release(&adev->kfd.client);
>   
>   		amdgpu_amdkfd_total_mem_size += adev->gmc.real_vram_size;
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index 68d534a89942..4caf8cece028 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -32,6 +32,7 @@
>   #include <linux/mmu_notifier.h>
>   #include <linux/memremap.h>
>   #include <kgd_kfd_interface.h>
> +#include <drm/drm_client.h>
>   #include <drm/ttm/ttm_execbuf_util.h>
>   #include "amdgpu_sync.h"
>   #include "amdgpu_vm.h"
> @@ -84,6 +85,7 @@ struct kgd_mem {
>   
>   	struct amdgpu_sync sync;
>   
> +	uint32_t gem_handle;
>   	bool aql_queue;
>   	bool is_imported;
>   };
> @@ -106,6 +108,9 @@ struct amdgpu_kfd_dev {
>   
>   	/* HMM page migration MEMORY_DEVICE_PRIVATE mapping */
>   	struct dev_pagemap pgmap;
> +
> +	/* Client for KFD BO GEM handle allocations */
> +	struct drm_client_dev client;
>   };
>   
>   enum kgd_engine_type {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 0c1cb6048259..4bb8b5fd7598 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -25,6 +25,7 @@
>   #include <linux/pagemap.h>
>   #include <linux/sched/mm.h>
>   #include <linux/sched/task.h>
> +#include <linux/fdtable.h>
>   #include <drm/ttm/ttm_tt.h>
>   
>   #include "amdgpu_object.h"
> @@ -804,13 +805,22 @@ kfd_mem_dmaunmap_attachment(struct kgd_mem *mem,
>   static int kfd_mem_export_dmabuf(struct kgd_mem *mem)
>   {
>   	if (!mem->dmabuf) {
> -		struct dma_buf *ret = amdgpu_gem_prime_export(
> -			&mem->bo->tbo.base,
> +		struct amdgpu_device *bo_adev;
> +		struct dma_buf *dmabuf;
> +		int r, fd;
> +
> +		bo_adev = amdgpu_ttm_adev(mem->bo->tbo.bdev);
> +		r = drm_gem_prime_handle_to_fd(&bo_adev->ddev, bo_adev->kfd.client.file,
> +					       mem->gem_handle,
>   			mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ?
> -				DRM_RDWR : 0);
> -		if (IS_ERR(ret))
> -			return PTR_ERR(ret);
> -		mem->dmabuf = ret;
> +					       DRM_RDWR : 0, &fd);
> +		if (r)
> +			return r;
> +		dmabuf = dma_buf_get(fd);
> +		close_fd(fd);
> +		if (WARN_ON_ONCE(IS_ERR(dmabuf)))
> +			return PTR_ERR(dmabuf);
> +		mem->dmabuf = dmabuf;
>   	}
>   
>   	return 0;
> @@ -1826,6 +1836,9 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>   		pr_debug("Failed to allow vma node access. ret %d\n", ret);
>   		goto err_node_allow;
>   	}
> +	ret = drm_gem_handle_create(adev->kfd.client.file, gobj, &(*mem)->gem_handle);
> +	if (ret)
> +		goto err_gem_handle_create;
>   	bo = gem_to_amdgpu_bo(gobj);
>   	if (bo_type == ttm_bo_type_sg) {
>   		bo->tbo.sg = sg;
> @@ -1877,6 +1890,8 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>   err_pin_bo:
>   err_validate_bo:
>   	remove_kgd_mem_from_kfd_bo_list(*mem, avm->process_info);
> +	drm_gem_handle_delete(adev->kfd.client.file, (*mem)->gem_handle);
> +err_gem_handle_create:
>   	drm_vma_node_revoke(&gobj->vma_node, drm_priv);
>   err_node_allow:
>   	/* Don't unreserve system mem limit twice */
> @@ -1991,8 +2006,12 @@ 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)
> +	if (!mem->is_imported)
> +		drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
> +	if (mem->dmabuf) {
>   		dma_buf_put(mem->dmabuf);
> +		mem->dmabuf = NULL;
> +	}
>   	mutex_destroy(&mem->lock);
>   
>   	/* If this releases the last reference, it will end up calling
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index 06988cf1db51..4417a9863cd0 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1855,8 +1855,8 @@ static uint32_t get_process_num_bos(struct kfd_process *p)
>   	return num_of_bos;
>   }
>   
> -static int criu_get_prime_handle(struct kgd_mem *mem, int flags,
> -				      u32 *shared_fd)
> +static int criu_get_prime_handle(struct kgd_mem *mem,
> +				 int flags, u32 *shared_fd)
>   {
>   	struct dma_buf *dmabuf;
>   	int ret;