* [PATCH v2 1/4] Revert "drm/prime: Unexport helpers for fd/handle conversion"
@ 2023-11-21 23:11 Felix Kuehling
2023-11-21 23:11 ` [PATCH v2 2/4] drm/prime: Helper to export dmabuf without fd Felix Kuehling
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Felix Kuehling @ 2023-11-21 23:11 UTC (permalink / raw)
To: dri-devel, amd-gfx; +Cc: christian.koenig, Thomas Zimmermann
This reverts commit 71a7974ac7019afeec105a54447ae1dc7216cbb3.
These helper functions are needed for KFD to export and import DMABufs
the right way without duplicating the tracking of DMABufs associated with
GEM objects while ensuring that move notifier callbacks are working as
intended.
CC: Christian König <christian.koenig@amd.com>
CC: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
drivers/gpu/drm/drm_prime.c | 33 ++++++++++++++++++---------------
include/drm/drm_prime.h | 7 +++++++
2 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 63b709a67471..834a5e28abbe 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -278,7 +278,7 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
}
EXPORT_SYMBOL(drm_gem_dmabuf_release);
-/*
+/**
* drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers
* @dev: drm_device to import into
* @file_priv: drm file-private structure
@@ -292,9 +292,9 @@ EXPORT_SYMBOL(drm_gem_dmabuf_release);
*
* Returns 0 on success or a negative error code on failure.
*/
-static int drm_gem_prime_fd_to_handle(struct drm_device *dev,
- struct drm_file *file_priv, int prime_fd,
- uint32_t *handle)
+int drm_gem_prime_fd_to_handle(struct drm_device *dev,
+ struct drm_file *file_priv, int prime_fd,
+ uint32_t *handle)
{
struct dma_buf *dma_buf;
struct drm_gem_object *obj;
@@ -360,6 +360,7 @@ static int drm_gem_prime_fd_to_handle(struct drm_device *dev,
dma_buf_put(dma_buf);
return ret;
}
+EXPORT_SYMBOL(drm_gem_prime_fd_to_handle);
int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv)
@@ -408,7 +409,7 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
return dmabuf;
}
-/*
+/**
* drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
* @dev: dev to export the buffer from
* @file_priv: drm file-private structure
@@ -421,10 +422,10 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
* The actual exporting from GEM object to a dma-buf is done through the
* &drm_gem_object_funcs.export callback.
*/
-static int drm_gem_prime_handle_to_fd(struct drm_device *dev,
- struct drm_file *file_priv, uint32_t handle,
- uint32_t flags,
- int *prime_fd)
+int drm_gem_prime_handle_to_fd(struct drm_device *dev,
+ struct drm_file *file_priv, uint32_t handle,
+ uint32_t flags,
+ int *prime_fd)
{
struct drm_gem_object *obj;
int ret = 0;
@@ -506,6 +507,7 @@ static int drm_gem_prime_handle_to_fd(struct drm_device *dev,
return ret;
}
+EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv)
@@ -864,9 +866,9 @@ EXPORT_SYMBOL(drm_prime_get_contiguous_size);
* @obj: GEM object to export
* @flags: flags like DRM_CLOEXEC and DRM_RDWR
*
- * This is the implementation of the &drm_gem_object_funcs.export functions
- * for GEM drivers using the PRIME helpers. It is used as the default for
- * drivers that do not set their own.
+ * This is the implementation of the &drm_gem_object_funcs.export functions for GEM drivers
+ * using the PRIME helpers. It is used as the default in
+ * drm_gem_prime_handle_to_fd().
*/
struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj,
int flags)
@@ -962,9 +964,10 @@ EXPORT_SYMBOL(drm_gem_prime_import_dev);
* @dev: drm_device to import into
* @dma_buf: dma-buf object to import
*
- * This is the implementation of the gem_prime_import functions for GEM
- * drivers using the PRIME helpers. It is the default for drivers that do
- * not set their own &drm_driver.gem_prime_import.
+ * This is the implementation of the gem_prime_import functions for GEM drivers
+ * using the PRIME helpers. Drivers can use this as their
+ * &drm_driver.gem_prime_import implementation. It is used as the default
+ * implementation in drm_gem_prime_fd_to_handle().
*
* Drivers must arrange to call drm_prime_gem_destroy() from their
* &drm_gem_object_funcs.free hook when using this function.
diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
index a7abf9f3e697..2a1d01e5b56b 100644
--- a/include/drm/drm_prime.h
+++ b/include/drm/drm_prime.h
@@ -60,12 +60,19 @@ enum dma_data_direction;
struct drm_device;
struct drm_gem_object;
+struct drm_file;
/* core prime functions */
struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev,
struct dma_buf_export_info *exp_info);
void drm_gem_dmabuf_release(struct dma_buf *dma_buf);
+int drm_gem_prime_fd_to_handle(struct drm_device *dev,
+ struct drm_file *file_priv, int prime_fd, uint32_t *handle);
+int drm_gem_prime_handle_to_fd(struct drm_device *dev,
+ struct drm_file *file_priv, uint32_t handle, uint32_t flags,
+ int *prime_fd);
+
/* helper functions for exporting */
int drm_gem_map_attach(struct dma_buf *dma_buf,
struct dma_buf_attachment *attach);
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/4] drm/prime: Helper to export dmabuf without fd
2023-11-21 23:11 [PATCH v2 1/4] Revert "drm/prime: Unexport helpers for fd/handle conversion" Felix Kuehling
@ 2023-11-21 23:11 ` Felix Kuehling
2023-11-22 10:32 ` Thomas Zimmermann
2023-11-22 14:48 ` kernel test robot
2023-11-21 23:11 ` [PATCH v2 3/4] drm/amdkfd: Export DMABufs from KFD using GEM handles Felix Kuehling
2023-11-21 23:11 ` [PATCH v2 4/4] drm/amdkfd: Import DMABufs for interop through DRM Felix Kuehling
2 siblings, 2 replies; 8+ messages in thread
From: Felix Kuehling @ 2023-11-21 23:11 UTC (permalink / raw)
To: dri-devel, amd-gfx; +Cc: christian.koenig, Thomas Zimmermann
Change drm_gem_prime_handle_to_fd to drm_gem_prime_handle_to_dmabuf to
export a dmabuf without creating an FD as a user mode handle. This is
more useful for users in kernel mode.
Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
drivers/gpu/drm/drm_prime.c | 63 ++++++++++++++++++-------------------
include/drm/drm_prime.h | 6 ++--
2 files changed, 33 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 834a5e28abbe..d491b5f73eea 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -410,26 +410,25 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
}
/**
- * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
+ * drm_gem_prime_handle_to_dmabuf - PRIME export function for GEM drivers
* @dev: dev to export the buffer from
* @file_priv: drm file-private structure
* @handle: buffer handle to export
* @flags: flags like DRM_CLOEXEC
- * @prime_fd: pointer to storage for the fd id of the create dma-buf
+ * @dma_buf: pointer to storage for the dma-buf reference
*
* This is the PRIME export function which must be used mandatorily by GEM
* drivers to ensure correct lifetime management of the underlying GEM object.
* The actual exporting from GEM object to a dma-buf is done through the
* &drm_gem_object_funcs.export callback.
*/
-int drm_gem_prime_handle_to_fd(struct drm_device *dev,
- struct drm_file *file_priv, uint32_t handle,
- uint32_t flags,
- int *prime_fd)
+struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
+ struct drm_file *file_priv,
+ uint32_t handle, uint32_t flags)
{
struct drm_gem_object *obj;
int ret = 0;
- struct dma_buf *dmabuf;
+ struct dma_buf *dmabuf = NULL;
mutex_lock(&file_priv->prime.lock);
obj = drm_gem_object_lookup(file_priv, handle);
@@ -441,7 +440,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);
if (dmabuf) {
get_dma_buf(dmabuf);
- goto out_have_handle;
+ goto out;
}
mutex_lock(&dev->object_name_lock);
@@ -479,40 +478,22 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
dmabuf, handle);
mutex_unlock(&dev->object_name_lock);
if (ret)
- goto fail_put_dmabuf;
-
-out_have_handle:
- ret = dma_buf_fd(dmabuf, flags);
- /*
- * We must _not_ remove the buffer from the handle cache since the newly
- * created dma buf is already linked in the global obj->dma_buf pointer,
- * and that is invariant as long as a userspace gem handle exists.
- * Closing the handle will clean out the cache anyway, so we don't leak.
- */
- if (ret < 0) {
- goto fail_put_dmabuf;
- } else {
- *prime_fd = ret;
- ret = 0;
- }
-
- goto out;
-
-fail_put_dmabuf:
- dma_buf_put(dmabuf);
+ dma_buf_put(dmabuf);
out:
drm_gem_object_put(obj);
out_unlock:
mutex_unlock(&file_priv->prime.lock);
- return ret;
+ return ret ? ERR_PTR(ret) : dmabuf;
}
-EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
+EXPORT_SYMBOL(drm_gem_prime_handle_to_dmabuf);
int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv)
{
struct drm_prime_handle *args = data;
+ struct dma_buf *dmabuf;
+ int ret;
/* check flags are valid */
if (args->flags & ~(DRM_CLOEXEC | DRM_RDWR))
@@ -523,8 +504,24 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
args->handle, args->flags,
&args->fd);
}
- return drm_gem_prime_handle_to_fd(dev, file_priv, args->handle,
- args->flags, &args->fd);
+ dmabuf = drm_gem_prime_handle_to_dmabuf(dev, file_priv, args->handle,
+ args->flags);
+ if (IS_ERR(dmabuf))
+ return PTR_ERR(dmabuf);
+ ret = dma_buf_fd(dmabuf, args->flags);
+ /*
+ * We must _not_ remove the buffer from the handle cache since the newly
+ * created dma buf is already linked in the global obj->dma_buf pointer,
+ * and that is invariant as long as a userspace gem handle exists.
+ * Closing the handle will clean out the cache anyway, so we don't leak.
+ */
+ if (ret < 0) {
+ dma_buf_put(dmabuf);
+ } else {
+ args->fd = ret;
+ ret = 0;
+ }
+ return ret;
}
/**
diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
index 2a1d01e5b56b..89e839293d14 100644
--- a/include/drm/drm_prime.h
+++ b/include/drm/drm_prime.h
@@ -69,9 +69,9 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf);
int drm_gem_prime_fd_to_handle(struct drm_device *dev,
struct drm_file *file_priv, int prime_fd, uint32_t *handle);
-int drm_gem_prime_handle_to_fd(struct drm_device *dev,
- struct drm_file *file_priv, uint32_t handle, uint32_t flags,
- int *prime_fd);
+struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
+ struct drm_file *file_priv,
+ uint32_t handle, uint32_t flags);
/* helper functions for exporting */
int drm_gem_map_attach(struct dma_buf *dma_buf,
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 3/4] drm/amdkfd: Export DMABufs from KFD using GEM handles
2023-11-21 23:11 [PATCH v2 1/4] Revert "drm/prime: Unexport helpers for fd/handle conversion" Felix Kuehling
2023-11-21 23:11 ` [PATCH v2 2/4] drm/prime: Helper to export dmabuf without fd Felix Kuehling
@ 2023-11-21 23:11 ` Felix Kuehling
2023-11-21 23:11 ` [PATCH v2 4/4] drm/amdkfd: Import DMABufs for interop through DRM Felix Kuehling
2 siblings, 0 replies; 8+ messages in thread
From: Felix Kuehling @ 2023-11-21 23:11 UTC (permalink / raw)
To: dri-devel, amd-gfx; +Cc: Ramesh Errabolu, christian.koenig
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.
Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
Reviewed-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>
---
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 | 29 ++++++++++++++-----
3 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
index b8412202a1b0..aa8b24079070 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 dac983da961d..c1195eb67057 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -33,6 +33,7 @@
#include <linux/mmu_notifier.h>
#include <linux/memremap.h>
#include <kgd_kfd_interface.h>
+#include <drm/drm_client.h>
#include "amdgpu_sync.h"
#include "amdgpu_vm.h"
#include "amdgpu_xcp.h"
@@ -83,6 +84,7 @@ struct kgd_mem {
struct amdgpu_sync sync;
+ uint32_t gem_handle;
bool aql_queue;
bool is_imported;
};
@@ -105,6 +107,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 41fbc4fd0fac..e96e1595791f 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 <drm/drm_exec.h>
@@ -806,13 +807,18 @@ 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;
+
+ bo_adev = amdgpu_ttm_adev(mem->bo->tbo.bdev);
+ dmabuf = drm_gem_prime_handle_to_dmabuf(&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);
+ if (IS_ERR(dmabuf))
+ return PTR_ERR(dmabuf);
+ mem->dmabuf = dmabuf;
}
return 0;
@@ -1779,6 +1785,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;
@@ -1830,6 +1839,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 */
@@ -1942,8 +1953,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
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 4/4] drm/amdkfd: Import DMABufs for interop through DRM
2023-11-21 23:11 [PATCH v2 1/4] Revert "drm/prime: Unexport helpers for fd/handle conversion" Felix Kuehling
2023-11-21 23:11 ` [PATCH v2 2/4] drm/prime: Helper to export dmabuf without fd Felix Kuehling
2023-11-21 23:11 ` [PATCH v2 3/4] drm/amdkfd: Export DMABufs from KFD using GEM handles Felix Kuehling
@ 2023-11-21 23:11 ` Felix Kuehling
2 siblings, 0 replies; 8+ messages in thread
From: Felix Kuehling @ 2023-11-21 23:11 UTC (permalink / raw)
To: dri-devel, amd-gfx; +Cc: Xiaogang . Chen, Ramesh Errabolu, christian.koenig
Use drm_gem_prime_fd_to_handle to import DMABufs for interop. This
ensures that a GEM handle is created on import and that obj->dma_buf
will be set and remain set as long as the object is imported into KFD.
Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
Reviewed-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>
Reviewed-by: Xiaogang.Chen <Xiaogang.Chen@amd.com>
Acked-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 9 ++-
.../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 64 +++++++++++++------
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 15 ++---
3 files changed, 52 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
index c1195eb67057..8da42e0dddcb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
@@ -319,11 +319,10 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *process_info,
struct dma_fence **ef);
int amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct amdgpu_device *adev,
struct kfd_vm_fault_info *info);
-int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
- struct dma_buf *dmabuf,
- uint64_t va, void *drm_priv,
- struct kgd_mem **mem, uint64_t *size,
- uint64_t *mmap_offset);
+int amdgpu_amdkfd_gpuvm_import_dmabuf_fd(struct amdgpu_device *adev, int fd,
+ uint64_t va, void *drm_priv,
+ struct kgd_mem **mem, uint64_t *size,
+ uint64_t *mmap_offset);
int amdgpu_amdkfd_gpuvm_export_dmabuf(struct kgd_mem *mem,
struct dma_buf **dmabuf);
void amdgpu_amdkfd_debug_mem_fence(struct amdgpu_device *adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index e96e1595791f..652657c863ff 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1953,8 +1953,7 @@ 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->is_imported)
- drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
+ drm_gem_handle_delete(adev->kfd.client.file, mem->gem_handle);
if (mem->dmabuf) {
dma_buf_put(mem->dmabuf);
mem->dmabuf = NULL;
@@ -2310,34 +2309,26 @@ int amdgpu_amdkfd_gpuvm_get_vm_fault_info(struct amdgpu_device *adev,
return 0;
}
-int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
- struct dma_buf *dma_buf,
- uint64_t va, void *drm_priv,
- struct kgd_mem **mem, uint64_t *size,
- uint64_t *mmap_offset)
+static int import_obj_create(struct amdgpu_device *adev,
+ struct dma_buf *dma_buf,
+ struct drm_gem_object *obj,
+ uint64_t va, void *drm_priv,
+ struct kgd_mem **mem, uint64_t *size,
+ uint64_t *mmap_offset)
{
struct amdgpu_vm *avm = drm_priv_to_vm(drm_priv);
- struct drm_gem_object *obj;
struct amdgpu_bo *bo;
int ret;
- obj = amdgpu_gem_prime_import(adev_to_drm(adev), dma_buf);
- if (IS_ERR(obj))
- return PTR_ERR(obj);
-
bo = gem_to_amdgpu_bo(obj);
if (!(bo->preferred_domains & (AMDGPU_GEM_DOMAIN_VRAM |
- AMDGPU_GEM_DOMAIN_GTT))) {
+ AMDGPU_GEM_DOMAIN_GTT)))
/* Only VRAM and GTT BOs are supported */
- ret = -EINVAL;
- goto err_put_obj;
- }
+ return -EINVAL;
*mem = kzalloc(sizeof(struct kgd_mem), GFP_KERNEL);
- if (!*mem) {
- ret = -ENOMEM;
- goto err_put_obj;
- }
+ if (!*mem)
+ return -ENOMEM;
ret = drm_vma_node_allow(&obj->vma_node, drm_priv);
if (ret)
@@ -2387,8 +2378,41 @@ int amdgpu_amdkfd_gpuvm_import_dmabuf(struct amdgpu_device *adev,
drm_vma_node_revoke(&obj->vma_node, drm_priv);
err_free_mem:
kfree(*mem);
+ return ret;
+}
+
+int amdgpu_amdkfd_gpuvm_import_dmabuf_fd(struct amdgpu_device *adev, int fd,
+ uint64_t va, void *drm_priv,
+ struct kgd_mem **mem, uint64_t *size,
+ uint64_t *mmap_offset)
+{
+ struct drm_gem_object *obj;
+ uint32_t handle;
+ int ret;
+
+ ret = drm_gem_prime_fd_to_handle(&adev->ddev, adev->kfd.client.file, fd,
+ &handle);
+ if (ret)
+ return ret;
+ obj = drm_gem_object_lookup(adev->kfd.client.file, handle);
+ if (!obj) {
+ ret = -EINVAL;
+ goto err_release_handle;
+ }
+
+ ret = import_obj_create(adev, obj->dma_buf, obj, va, drm_priv, mem, size,
+ mmap_offset);
+ if (ret)
+ goto err_put_obj;
+
+ (*mem)->gem_handle = handle;
+
+ return 0;
+
err_put_obj:
drm_gem_object_put(obj);
+err_release_handle:
+ drm_gem_handle_delete(adev->kfd.client.file, handle);
return ret;
}
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index f6d4748c1980..247281b1c9d4 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -1564,16 +1564,11 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
{
struct kfd_ioctl_import_dmabuf_args *args = data;
struct kfd_process_device *pdd;
- struct dma_buf *dmabuf;
int idr_handle;
uint64_t size;
void *mem;
int r;
- dmabuf = dma_buf_get(args->dmabuf_fd);
- if (IS_ERR(dmabuf))
- return PTR_ERR(dmabuf);
-
mutex_lock(&p->mutex);
pdd = kfd_process_device_data_by_id(p, args->gpu_id);
if (!pdd) {
@@ -1587,10 +1582,10 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
goto err_unlock;
}
- r = amdgpu_amdkfd_gpuvm_import_dmabuf(pdd->dev->adev, dmabuf,
- args->va_addr, pdd->drm_priv,
- (struct kgd_mem **)&mem, &size,
- NULL);
+ r = amdgpu_amdkfd_gpuvm_import_dmabuf_fd(pdd->dev->adev, args->dmabuf_fd,
+ args->va_addr, pdd->drm_priv,
+ (struct kgd_mem **)&mem, &size,
+ NULL);
if (r)
goto err_unlock;
@@ -1601,7 +1596,6 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
}
mutex_unlock(&p->mutex);
- dma_buf_put(dmabuf);
args->handle = MAKE_HANDLE(args->gpu_id, idr_handle);
@@ -1612,7 +1606,6 @@ static int kfd_ioctl_import_dmabuf(struct file *filep,
pdd->drm_priv, NULL);
err_unlock:
mutex_unlock(&p->mutex);
- dma_buf_put(dmabuf);
return r;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/4] drm/prime: Helper to export dmabuf without fd
2023-11-21 23:11 ` [PATCH v2 2/4] drm/prime: Helper to export dmabuf without fd Felix Kuehling
@ 2023-11-22 10:32 ` Thomas Zimmermann
2023-11-22 19:24 ` Felix Kuehling
2023-11-22 14:48 ` kernel test robot
1 sibling, 1 reply; 8+ messages in thread
From: Thomas Zimmermann @ 2023-11-22 10:32 UTC (permalink / raw)
To: Felix Kuehling, dri-devel, amd-gfx; +Cc: christian.koenig
[-- Attachment #1.1: Type: text/plain, Size: 7177 bytes --]
Hi,
my apologies if this sounds picky or annoying. This change appears to be
going in the wrong direction. The goal of the refactoring is to be able
to use drm_driver.gem_prime_import and drm_gem_object_funcs.export for
the additional import/export code; and hence keep the GEM object code in
a single place. Keeping the prime_fd file descriptor within amdkfd will
likely help with that.
Here's my suggestion:
1) Please keep the internal interfaces drm_gem_prime_handle_to_fd()
and drm_gem_prime_fd_to_handle(). They should be called from the _ioctl
entry functions as is. That could be stream-lined in a later patch set.
2) From drm_gem_prime_handle_to_fd() and drm_gem_prime_fd_to_handle(),
create drm_gem_prime_handle_to_dmabuf() and
drm_gem_prime_dmabuf_to_handle(). They should be exported. You can then
keep the file-descriptor code in amdkfd and out of the PRIME helpers.
3) Patches 1 and 2 should be squashed into one.
4) And if I'm not mistaken, the additional import/export code can then
go into drm_driver.gem_prime_import and drm_gem_object_funcs.export,
which are being called from within the PRIME helpers.
That's admittedly quite a bit of refactoring. OR simply go back to v1 of
this patch set, which was consistent at least.
Best regards
Thomas
Am 22.11.23 um 00:11 schrieb Felix Kuehling:
> Change drm_gem_prime_handle_to_fd to drm_gem_prime_handle_to_dmabuf to
> export a dmabuf without creating an FD as a user mode handle. This is
> more useful for users in kernel mode.
>
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
> drivers/gpu/drm/drm_prime.c | 63 ++++++++++++++++++-------------------
> include/drm/drm_prime.h | 6 ++--
> 2 files changed, 33 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 834a5e28abbe..d491b5f73eea 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -410,26 +410,25 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
> }
>
> /**
> - * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
> + * drm_gem_prime_handle_to_dmabuf - PRIME export function for GEM drivers
> * @dev: dev to export the buffer from
> * @file_priv: drm file-private structure
> * @handle: buffer handle to export
> * @flags: flags like DRM_CLOEXEC
> - * @prime_fd: pointer to storage for the fd id of the create dma-buf
> + * @dma_buf: pointer to storage for the dma-buf reference
> *
> * This is the PRIME export function which must be used mandatorily by GEM
> * drivers to ensure correct lifetime management of the underlying GEM object.
> * The actual exporting from GEM object to a dma-buf is done through the
> * &drm_gem_object_funcs.export callback.
> */
> -int drm_gem_prime_handle_to_fd(struct drm_device *dev,
> - struct drm_file *file_priv, uint32_t handle,
> - uint32_t flags,
> - int *prime_fd)
> +struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
> + struct drm_file *file_priv,
> + uint32_t handle, uint32_t flags)
> {
> struct drm_gem_object *obj;
> int ret = 0;
> - struct dma_buf *dmabuf;
> + struct dma_buf *dmabuf = NULL;
>
> mutex_lock(&file_priv->prime.lock);
> obj = drm_gem_object_lookup(file_priv, handle);
> @@ -441,7 +440,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
> dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);
> if (dmabuf) {
> get_dma_buf(dmabuf);
> - goto out_have_handle;
> + goto out;
> }
>
> mutex_lock(&dev->object_name_lock);
> @@ -479,40 +478,22 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
> dmabuf, handle);
> mutex_unlock(&dev->object_name_lock);
> if (ret)
> - goto fail_put_dmabuf;
> -
> -out_have_handle:
> - ret = dma_buf_fd(dmabuf, flags);
> - /*
> - * We must _not_ remove the buffer from the handle cache since the newly
> - * created dma buf is already linked in the global obj->dma_buf pointer,
> - * and that is invariant as long as a userspace gem handle exists.
> - * Closing the handle will clean out the cache anyway, so we don't leak.
> - */
> - if (ret < 0) {
> - goto fail_put_dmabuf;
> - } else {
> - *prime_fd = ret;
> - ret = 0;
> - }
> -
> - goto out;
> -
> -fail_put_dmabuf:
> - dma_buf_put(dmabuf);
> + dma_buf_put(dmabuf);
> out:
> drm_gem_object_put(obj);
> out_unlock:
> mutex_unlock(&file_priv->prime.lock);
>
> - return ret;
> + return ret ? ERR_PTR(ret) : dmabuf;
> }
> -EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
> +EXPORT_SYMBOL(drm_gem_prime_handle_to_dmabuf);
>
> int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file_priv)
> {
> struct drm_prime_handle *args = data;
> + struct dma_buf *dmabuf;
> + int ret;
>
> /* check flags are valid */
> if (args->flags & ~(DRM_CLOEXEC | DRM_RDWR))
> @@ -523,8 +504,24 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
> args->handle, args->flags,
> &args->fd);
> }
> - return drm_gem_prime_handle_to_fd(dev, file_priv, args->handle,
> - args->flags, &args->fd);
> + dmabuf = drm_gem_prime_handle_to_dmabuf(dev, file_priv, args->handle,
> + args->flags);
> + if (IS_ERR(dmabuf))
> + return PTR_ERR(dmabuf);
> + ret = dma_buf_fd(dmabuf, args->flags);
> + /*
> + * We must _not_ remove the buffer from the handle cache since the newly
> + * created dma buf is already linked in the global obj->dma_buf pointer,
> + * and that is invariant as long as a userspace gem handle exists.
> + * Closing the handle will clean out the cache anyway, so we don't leak.
> + */
> + if (ret < 0) {
> + dma_buf_put(dmabuf);
> + } else {
> + args->fd = ret;
> + ret = 0;
> + }
> + return ret;
> }
>
> /**
> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
> index 2a1d01e5b56b..89e839293d14 100644
> --- a/include/drm/drm_prime.h
> +++ b/include/drm/drm_prime.h
> @@ -69,9 +69,9 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf);
>
> int drm_gem_prime_fd_to_handle(struct drm_device *dev,
> struct drm_file *file_priv, int prime_fd, uint32_t *handle);
> -int drm_gem_prime_handle_to_fd(struct drm_device *dev,
> - struct drm_file *file_priv, uint32_t handle, uint32_t flags,
> - int *prime_fd);
> +struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
> + struct drm_file *file_priv,
> + uint32_t handle, uint32_t flags);
>
> /* helper functions for exporting */
> int drm_gem_map_attach(struct dma_buf *dma_buf,
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/4] drm/prime: Helper to export dmabuf without fd
2023-11-21 23:11 ` [PATCH v2 2/4] drm/prime: Helper to export dmabuf without fd Felix Kuehling
@ 2023-11-22 14:48 ` kernel test robot
2023-11-22 14:48 ` kernel test robot
1 sibling, 0 replies; 8+ messages in thread
From: kernel test robot @ 2023-11-22 14:48 UTC (permalink / raw)
To: Felix Kuehling, dri-devel, amd-gfx
Cc: christian.koenig, Thomas Zimmermann, oe-kbuild-all
Hi Felix,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.7-rc2 next-20231122]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Felix-Kuehling/drm-prime-Helper-to-export-dmabuf-without-fd/20231122-071410
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20231121231114.703478-2-Felix.Kuehling%40amd.com
patch subject: [PATCH v2 2/4] drm/prime: Helper to export dmabuf without fd
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20231122/202311221810.IJ6ELH6c-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231122/202311221810.IJ6ELH6c-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311221810.IJ6ELH6c-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/drm_prime.c:428: warning: Excess function parameter 'dma_buf' description in 'drm_gem_prime_handle_to_dmabuf'
vim +428 drivers/gpu/drm/drm_prime.c
319c933c71f3db Daniel Vetter 2013-08-15 411
d3977ac8e10a0b Felix Kuehling 2023-11-21 412 /**
f3facd92ee25e0 Felix Kuehling 2023-11-21 413 * drm_gem_prime_handle_to_dmabuf - PRIME export function for GEM drivers
39cc344acd414e Daniel Vetter 2014-01-22 414 * @dev: dev to export the buffer from
39cc344acd414e Daniel Vetter 2014-01-22 415 * @file_priv: drm file-private structure
39cc344acd414e Daniel Vetter 2014-01-22 416 * @handle: buffer handle to export
39cc344acd414e Daniel Vetter 2014-01-22 417 * @flags: flags like DRM_CLOEXEC
f3facd92ee25e0 Felix Kuehling 2023-11-21 418 * @dma_buf: pointer to storage for the dma-buf reference
39cc344acd414e Daniel Vetter 2014-01-22 419 *
39cc344acd414e Daniel Vetter 2014-01-22 420 * This is the PRIME export function which must be used mandatorily by GEM
39cc344acd414e Daniel Vetter 2014-01-22 421 * drivers to ensure correct lifetime management of the underlying GEM object.
39cc344acd414e Daniel Vetter 2014-01-22 422 * The actual exporting from GEM object to a dma-buf is done through the
d693def4fd1c23 Thomas Zimmermann 2020-09-23 423 * &drm_gem_object_funcs.export callback.
39cc344acd414e Daniel Vetter 2014-01-22 424 */
f3facd92ee25e0 Felix Kuehling 2023-11-21 425 struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
f3facd92ee25e0 Felix Kuehling 2023-11-21 426 struct drm_file *file_priv,
f3facd92ee25e0 Felix Kuehling 2023-11-21 427 uint32_t handle, uint32_t flags)
3248877ea17969 Dave Airlie 2011-11-25 @428 {
3248877ea17969 Dave Airlie 2011-11-25 429 struct drm_gem_object *obj;
219b47339ced80 Dave Airlie 2013-04-22 430 int ret = 0;
f3facd92ee25e0 Felix Kuehling 2023-11-21 431 struct dma_buf *dmabuf = NULL;
3248877ea17969 Dave Airlie 2011-11-25 432
d0b2c5334f41bd Daniel Vetter 2013-08-15 433 mutex_lock(&file_priv->prime.lock);
a8ad0bd84f9860 Chris Wilson 2016-05-09 434 obj = drm_gem_object_lookup(file_priv, handle);
d0b2c5334f41bd Daniel Vetter 2013-08-15 435 if (!obj) {
d0b2c5334f41bd Daniel Vetter 2013-08-15 436 ret = -ENOENT;
d0b2c5334f41bd Daniel Vetter 2013-08-15 437 goto out_unlock;
d0b2c5334f41bd Daniel Vetter 2013-08-15 438 }
d0b2c5334f41bd Daniel Vetter 2013-08-15 439
d0b2c5334f41bd Daniel Vetter 2013-08-15 440 dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);
d0b2c5334f41bd Daniel Vetter 2013-08-15 441 if (dmabuf) {
d0b2c5334f41bd Daniel Vetter 2013-08-15 442 get_dma_buf(dmabuf);
f3facd92ee25e0 Felix Kuehling 2023-11-21 443 goto out;
d0b2c5334f41bd Daniel Vetter 2013-08-15 444 }
3248877ea17969 Dave Airlie 2011-11-25 445
d0b2c5334f41bd Daniel Vetter 2013-08-15 446 mutex_lock(&dev->object_name_lock);
3248877ea17969 Dave Airlie 2011-11-25 447 /* re-export the original imported object */
3248877ea17969 Dave Airlie 2011-11-25 448 if (obj->import_attach) {
219b47339ced80 Dave Airlie 2013-04-22 449 dmabuf = obj->import_attach->dmabuf;
319c933c71f3db Daniel Vetter 2013-08-15 450 get_dma_buf(dmabuf);
219b47339ced80 Dave Airlie 2013-04-22 451 goto out_have_obj;
3248877ea17969 Dave Airlie 2011-11-25 452 }
3248877ea17969 Dave Airlie 2011-11-25 453
319c933c71f3db Daniel Vetter 2013-08-15 454 if (obj->dma_buf) {
319c933c71f3db Daniel Vetter 2013-08-15 455 get_dma_buf(obj->dma_buf);
319c933c71f3db Daniel Vetter 2013-08-15 456 dmabuf = obj->dma_buf;
219b47339ced80 Dave Airlie 2013-04-22 457 goto out_have_obj;
219b47339ced80 Dave Airlie 2013-04-22 458 }
219b47339ced80 Dave Airlie 2013-04-22 459
319c933c71f3db Daniel Vetter 2013-08-15 460 dmabuf = export_and_register_object(dev, obj, flags);
4332bf438bbbc3 Daniel Vetter 2013-08-15 461 if (IS_ERR(dmabuf)) {
3248877ea17969 Dave Airlie 2011-11-25 462 /* normally the created dma-buf takes ownership of the ref,
3248877ea17969 Dave Airlie 2011-11-25 463 * but if that fails then drop the ref
3248877ea17969 Dave Airlie 2011-11-25 464 */
4332bf438bbbc3 Daniel Vetter 2013-08-15 465 ret = PTR_ERR(dmabuf);
d0b2c5334f41bd Daniel Vetter 2013-08-15 466 mutex_unlock(&dev->object_name_lock);
219b47339ced80 Dave Airlie 2013-04-22 467 goto out;
3248877ea17969 Dave Airlie 2011-11-25 468 }
219b47339ced80 Dave Airlie 2013-04-22 469
d0b2c5334f41bd Daniel Vetter 2013-08-15 470 out_have_obj:
d0b2c5334f41bd Daniel Vetter 2013-08-15 471 /*
d0b2c5334f41bd Daniel Vetter 2013-08-15 472 * If we've exported this buffer then cheat and add it to the import list
d0b2c5334f41bd Daniel Vetter 2013-08-15 473 * so we get the correct handle back. We must do this under the
d0b2c5334f41bd Daniel Vetter 2013-08-15 474 * protection of dev->object_name_lock to ensure that a racing gem close
d0b2c5334f41bd Daniel Vetter 2013-08-15 475 * ioctl doesn't miss to remove this buffer handle from the cache.
0ff926c7d4f06f Dave Airlie 2012-05-20 476 */
219b47339ced80 Dave Airlie 2013-04-22 477 ret = drm_prime_add_buf_handle(&file_priv->prime,
319c933c71f3db Daniel Vetter 2013-08-15 478 dmabuf, handle);
d0b2c5334f41bd Daniel Vetter 2013-08-15 479 mutex_unlock(&dev->object_name_lock);
219b47339ced80 Dave Airlie 2013-04-22 480 if (ret)
4332bf438bbbc3 Daniel Vetter 2013-08-15 481 dma_buf_put(dmabuf);
219b47339ced80 Dave Airlie 2013-04-22 482 out:
be6ee102341bc4 Emil Velikov 2020-05-15 483 drm_gem_object_put(obj);
d0b2c5334f41bd Daniel Vetter 2013-08-15 484 out_unlock:
d0b2c5334f41bd Daniel Vetter 2013-08-15 485 mutex_unlock(&file_priv->prime.lock);
d0b2c5334f41bd Daniel Vetter 2013-08-15 486
f3facd92ee25e0 Felix Kuehling 2023-11-21 487 return ret ? ERR_PTR(ret) : dmabuf;
b283e92a2315f9 Daniel Vetter 2019-06-18 488 }
f3facd92ee25e0 Felix Kuehling 2023-11-21 489 EXPORT_SYMBOL(drm_gem_prime_handle_to_dmabuf);
b283e92a2315f9 Daniel Vetter 2019-06-18 490
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/4] drm/prime: Helper to export dmabuf without fd
@ 2023-11-22 14:48 ` kernel test robot
0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2023-11-22 14:48 UTC (permalink / raw)
To: Felix Kuehling, dri-devel, amd-gfx
Cc: oe-kbuild-all, christian.koenig, Thomas Zimmermann
Hi Felix,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.7-rc2 next-20231122]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Felix-Kuehling/drm-prime-Helper-to-export-dmabuf-without-fd/20231122-071410
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20231121231114.703478-2-Felix.Kuehling%40amd.com
patch subject: [PATCH v2 2/4] drm/prime: Helper to export dmabuf without fd
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20231122/202311221810.IJ6ELH6c-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231122/202311221810.IJ6ELH6c-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311221810.IJ6ELH6c-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/drm_prime.c:428: warning: Excess function parameter 'dma_buf' description in 'drm_gem_prime_handle_to_dmabuf'
vim +428 drivers/gpu/drm/drm_prime.c
319c933c71f3db Daniel Vetter 2013-08-15 411
d3977ac8e10a0b Felix Kuehling 2023-11-21 412 /**
f3facd92ee25e0 Felix Kuehling 2023-11-21 413 * drm_gem_prime_handle_to_dmabuf - PRIME export function for GEM drivers
39cc344acd414e Daniel Vetter 2014-01-22 414 * @dev: dev to export the buffer from
39cc344acd414e Daniel Vetter 2014-01-22 415 * @file_priv: drm file-private structure
39cc344acd414e Daniel Vetter 2014-01-22 416 * @handle: buffer handle to export
39cc344acd414e Daniel Vetter 2014-01-22 417 * @flags: flags like DRM_CLOEXEC
f3facd92ee25e0 Felix Kuehling 2023-11-21 418 * @dma_buf: pointer to storage for the dma-buf reference
39cc344acd414e Daniel Vetter 2014-01-22 419 *
39cc344acd414e Daniel Vetter 2014-01-22 420 * This is the PRIME export function which must be used mandatorily by GEM
39cc344acd414e Daniel Vetter 2014-01-22 421 * drivers to ensure correct lifetime management of the underlying GEM object.
39cc344acd414e Daniel Vetter 2014-01-22 422 * The actual exporting from GEM object to a dma-buf is done through the
d693def4fd1c23 Thomas Zimmermann 2020-09-23 423 * &drm_gem_object_funcs.export callback.
39cc344acd414e Daniel Vetter 2014-01-22 424 */
f3facd92ee25e0 Felix Kuehling 2023-11-21 425 struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
f3facd92ee25e0 Felix Kuehling 2023-11-21 426 struct drm_file *file_priv,
f3facd92ee25e0 Felix Kuehling 2023-11-21 427 uint32_t handle, uint32_t flags)
3248877ea17969 Dave Airlie 2011-11-25 @428 {
3248877ea17969 Dave Airlie 2011-11-25 429 struct drm_gem_object *obj;
219b47339ced80 Dave Airlie 2013-04-22 430 int ret = 0;
f3facd92ee25e0 Felix Kuehling 2023-11-21 431 struct dma_buf *dmabuf = NULL;
3248877ea17969 Dave Airlie 2011-11-25 432
d0b2c5334f41bd Daniel Vetter 2013-08-15 433 mutex_lock(&file_priv->prime.lock);
a8ad0bd84f9860 Chris Wilson 2016-05-09 434 obj = drm_gem_object_lookup(file_priv, handle);
d0b2c5334f41bd Daniel Vetter 2013-08-15 435 if (!obj) {
d0b2c5334f41bd Daniel Vetter 2013-08-15 436 ret = -ENOENT;
d0b2c5334f41bd Daniel Vetter 2013-08-15 437 goto out_unlock;
d0b2c5334f41bd Daniel Vetter 2013-08-15 438 }
d0b2c5334f41bd Daniel Vetter 2013-08-15 439
d0b2c5334f41bd Daniel Vetter 2013-08-15 440 dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);
d0b2c5334f41bd Daniel Vetter 2013-08-15 441 if (dmabuf) {
d0b2c5334f41bd Daniel Vetter 2013-08-15 442 get_dma_buf(dmabuf);
f3facd92ee25e0 Felix Kuehling 2023-11-21 443 goto out;
d0b2c5334f41bd Daniel Vetter 2013-08-15 444 }
3248877ea17969 Dave Airlie 2011-11-25 445
d0b2c5334f41bd Daniel Vetter 2013-08-15 446 mutex_lock(&dev->object_name_lock);
3248877ea17969 Dave Airlie 2011-11-25 447 /* re-export the original imported object */
3248877ea17969 Dave Airlie 2011-11-25 448 if (obj->import_attach) {
219b47339ced80 Dave Airlie 2013-04-22 449 dmabuf = obj->import_attach->dmabuf;
319c933c71f3db Daniel Vetter 2013-08-15 450 get_dma_buf(dmabuf);
219b47339ced80 Dave Airlie 2013-04-22 451 goto out_have_obj;
3248877ea17969 Dave Airlie 2011-11-25 452 }
3248877ea17969 Dave Airlie 2011-11-25 453
319c933c71f3db Daniel Vetter 2013-08-15 454 if (obj->dma_buf) {
319c933c71f3db Daniel Vetter 2013-08-15 455 get_dma_buf(obj->dma_buf);
319c933c71f3db Daniel Vetter 2013-08-15 456 dmabuf = obj->dma_buf;
219b47339ced80 Dave Airlie 2013-04-22 457 goto out_have_obj;
219b47339ced80 Dave Airlie 2013-04-22 458 }
219b47339ced80 Dave Airlie 2013-04-22 459
319c933c71f3db Daniel Vetter 2013-08-15 460 dmabuf = export_and_register_object(dev, obj, flags);
4332bf438bbbc3 Daniel Vetter 2013-08-15 461 if (IS_ERR(dmabuf)) {
3248877ea17969 Dave Airlie 2011-11-25 462 /* normally the created dma-buf takes ownership of the ref,
3248877ea17969 Dave Airlie 2011-11-25 463 * but if that fails then drop the ref
3248877ea17969 Dave Airlie 2011-11-25 464 */
4332bf438bbbc3 Daniel Vetter 2013-08-15 465 ret = PTR_ERR(dmabuf);
d0b2c5334f41bd Daniel Vetter 2013-08-15 466 mutex_unlock(&dev->object_name_lock);
219b47339ced80 Dave Airlie 2013-04-22 467 goto out;
3248877ea17969 Dave Airlie 2011-11-25 468 }
219b47339ced80 Dave Airlie 2013-04-22 469
d0b2c5334f41bd Daniel Vetter 2013-08-15 470 out_have_obj:
d0b2c5334f41bd Daniel Vetter 2013-08-15 471 /*
d0b2c5334f41bd Daniel Vetter 2013-08-15 472 * If we've exported this buffer then cheat and add it to the import list
d0b2c5334f41bd Daniel Vetter 2013-08-15 473 * so we get the correct handle back. We must do this under the
d0b2c5334f41bd Daniel Vetter 2013-08-15 474 * protection of dev->object_name_lock to ensure that a racing gem close
d0b2c5334f41bd Daniel Vetter 2013-08-15 475 * ioctl doesn't miss to remove this buffer handle from the cache.
0ff926c7d4f06f Dave Airlie 2012-05-20 476 */
219b47339ced80 Dave Airlie 2013-04-22 477 ret = drm_prime_add_buf_handle(&file_priv->prime,
319c933c71f3db Daniel Vetter 2013-08-15 478 dmabuf, handle);
d0b2c5334f41bd Daniel Vetter 2013-08-15 479 mutex_unlock(&dev->object_name_lock);
219b47339ced80 Dave Airlie 2013-04-22 480 if (ret)
4332bf438bbbc3 Daniel Vetter 2013-08-15 481 dma_buf_put(dmabuf);
219b47339ced80 Dave Airlie 2013-04-22 482 out:
be6ee102341bc4 Emil Velikov 2020-05-15 483 drm_gem_object_put(obj);
d0b2c5334f41bd Daniel Vetter 2013-08-15 484 out_unlock:
d0b2c5334f41bd Daniel Vetter 2013-08-15 485 mutex_unlock(&file_priv->prime.lock);
d0b2c5334f41bd Daniel Vetter 2013-08-15 486
f3facd92ee25e0 Felix Kuehling 2023-11-21 487 return ret ? ERR_PTR(ret) : dmabuf;
b283e92a2315f9 Daniel Vetter 2019-06-18 488 }
f3facd92ee25e0 Felix Kuehling 2023-11-21 489 EXPORT_SYMBOL(drm_gem_prime_handle_to_dmabuf);
b283e92a2315f9 Daniel Vetter 2019-06-18 490
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/4] drm/prime: Helper to export dmabuf without fd
2023-11-22 10:32 ` Thomas Zimmermann
@ 2023-11-22 19:24 ` Felix Kuehling
0 siblings, 0 replies; 8+ messages in thread
From: Felix Kuehling @ 2023-11-22 19:24 UTC (permalink / raw)
To: Thomas Zimmermann, dri-devel, amd-gfx; +Cc: Koenig, Christian
On 2023-11-22 05:32, Thomas Zimmermann wrote:
> Hi,
>
> my apologies if this sounds picky or annoying. This change appears to be
> going in the wrong direction. The goal of the refactoring is to be able
> to use drm_driver.gem_prime_import and drm_gem_object_funcs.export for
> the additional import/export code; and hence keep the GEM object code in
> a single place. Keeping the prime_fd file descriptor within amdkfd will
> likely help with that.
>
> Here's my suggestion:
>
> 1) Please keep the internal interfaces drm_gem_prime_handle_to_fd()
> and drm_gem_prime_fd_to_handle(). They should be called from the _ioctl
> entry functions as is. That could be stream-lined in a later patch set.
>
> 2) From drm_gem_prime_handle_to_fd() and drm_gem_prime_fd_to_handle(),
> create drm_gem_prime_handle_to_dmabuf() and
> drm_gem_prime_dmabuf_to_handle().
Do you mean duplicate the code, or call drm_gem_prime_handle_to_dmabuf
from drm_gem_prime_handle_to_fd?
> They should be exported. You can then
> keep the file-descriptor code in amdkfd and out of the PRIME helpers.
>
> 3) Patches 1 and 2 should be squashed into one.
>
> 4) And if I'm not mistaken, the additional import/export code can then
> go into drm_driver.gem_prime_import and drm_gem_object_funcs.export,
> which are being called from within the PRIME helpers.
I'm not sure what you mean by "additional import/export code" that would
move into those driver callbacks.
>
> That's admittedly quite a bit of refactoring. OR simply go back to v1 of
> this patch set, which was consistent at least.
I think I'd prefer that because I don't really understand what you're
trying to achieve.
Thanks,
Felix
>
> Best regards
> Thomas
>
>
> Am 22.11.23 um 00:11 schrieb Felix Kuehling:
>> Change drm_gem_prime_handle_to_fd to drm_gem_prime_handle_to_dmabuf to
>> export a dmabuf without creating an FD as a user mode handle. This is
>> more useful for users in kernel mode.
>>
>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
>> ---
>> drivers/gpu/drm/drm_prime.c | 63 ++++++++++++++++++-------------------
>> include/drm/drm_prime.h | 6 ++--
>> 2 files changed, 33 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index 834a5e28abbe..d491b5f73eea 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -410,26 +410,25 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
>> }
>>
>> /**
>> - * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
>> + * drm_gem_prime_handle_to_dmabuf - PRIME export function for GEM drivers
>> * @dev: dev to export the buffer from
>> * @file_priv: drm file-private structure
>> * @handle: buffer handle to export
>> * @flags: flags like DRM_CLOEXEC
>> - * @prime_fd: pointer to storage for the fd id of the create dma-buf
>> + * @dma_buf: pointer to storage for the dma-buf reference
>> *
>> * This is the PRIME export function which must be used mandatorily by GEM
>> * drivers to ensure correct lifetime management of the underlying GEM object.
>> * The actual exporting from GEM object to a dma-buf is done through the
>> * &drm_gem_object_funcs.export callback.
>> */
>> -int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>> - struct drm_file *file_priv, uint32_t handle,
>> - uint32_t flags,
>> - int *prime_fd)
>> +struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
>> + struct drm_file *file_priv,
>> + uint32_t handle, uint32_t flags)
>> {
>> struct drm_gem_object *obj;
>> int ret = 0;
>> - struct dma_buf *dmabuf;
>> + struct dma_buf *dmabuf = NULL;
>>
>> mutex_lock(&file_priv->prime.lock);
>> obj = drm_gem_object_lookup(file_priv, handle);
>> @@ -441,7 +440,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>> dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);
>> if (dmabuf) {
>> get_dma_buf(dmabuf);
>> - goto out_have_handle;
>> + goto out;
>> }
>>
>> mutex_lock(&dev->object_name_lock);
>> @@ -479,40 +478,22 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>> dmabuf, handle);
>> mutex_unlock(&dev->object_name_lock);
>> if (ret)
>> - goto fail_put_dmabuf;
>> -
>> -out_have_handle:
>> - ret = dma_buf_fd(dmabuf, flags);
>> - /*
>> - * We must _not_ remove the buffer from the handle cache since the newly
>> - * created dma buf is already linked in the global obj->dma_buf pointer,
>> - * and that is invariant as long as a userspace gem handle exists.
>> - * Closing the handle will clean out the cache anyway, so we don't leak.
>> - */
>> - if (ret < 0) {
>> - goto fail_put_dmabuf;
>> - } else {
>> - *prime_fd = ret;
>> - ret = 0;
>> - }
>> -
>> - goto out;
>> -
>> -fail_put_dmabuf:
>> - dma_buf_put(dmabuf);
>> + dma_buf_put(dmabuf);
>> out:
>> drm_gem_object_put(obj);
>> out_unlock:
>> mutex_unlock(&file_priv->prime.lock);
>>
>> - return ret;
>> + return ret ? ERR_PTR(ret) : dmabuf;
>> }
>> -EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
>> +EXPORT_SYMBOL(drm_gem_prime_handle_to_dmabuf);
>>
>> int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
>> struct drm_file *file_priv)
>> {
>> struct drm_prime_handle *args = data;
>> + struct dma_buf *dmabuf;
>> + int ret;
>>
>> /* check flags are valid */
>> if (args->flags & ~(DRM_CLOEXEC | DRM_RDWR))
>> @@ -523,8 +504,24 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
>> args->handle, args->flags,
>> &args->fd);
>> }
>> - return drm_gem_prime_handle_to_fd(dev, file_priv, args->handle,
>> - args->flags, &args->fd);
>> + dmabuf = drm_gem_prime_handle_to_dmabuf(dev, file_priv, args->handle,
>> + args->flags);
>> + if (IS_ERR(dmabuf))
>> + return PTR_ERR(dmabuf);
>> + ret = dma_buf_fd(dmabuf, args->flags);
>> + /*
>> + * We must _not_ remove the buffer from the handle cache since the newly
>> + * created dma buf is already linked in the global obj->dma_buf pointer,
>> + * and that is invariant as long as a userspace gem handle exists.
>> + * Closing the handle will clean out the cache anyway, so we don't leak.
>> + */
>> + if (ret < 0) {
>> + dma_buf_put(dmabuf);
>> + } else {
>> + args->fd = ret;
>> + ret = 0;
>> + }
>> + return ret;
>> }
>>
>> /**
>> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
>> index 2a1d01e5b56b..89e839293d14 100644
>> --- a/include/drm/drm_prime.h
>> +++ b/include/drm/drm_prime.h
>> @@ -69,9 +69,9 @@ void drm_gem_dmabuf_release(struct dma_buf *dma_buf);
>>
>> int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>> struct drm_file *file_priv, int prime_fd, uint32_t *handle);
>> -int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>> - struct drm_file *file_priv, uint32_t handle, uint32_t flags,
>> - int *prime_fd);
>> +struct dma_buf *drm_gem_prime_handle_to_dmabuf(struct drm_device *dev,
>> + struct drm_file *file_priv,
>> + uint32_t handle, uint32_t flags);
>>
>> /* helper functions for exporting */
>> int drm_gem_map_attach(struct dma_buf *dma_buf,
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-11-22 19:24 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-21 23:11 [PATCH v2 1/4] Revert "drm/prime: Unexport helpers for fd/handle conversion" Felix Kuehling
2023-11-21 23:11 ` [PATCH v2 2/4] drm/prime: Helper to export dmabuf without fd Felix Kuehling
2023-11-22 10:32 ` Thomas Zimmermann
2023-11-22 19:24 ` Felix Kuehling
2023-11-22 14:48 ` kernel test robot
2023-11-22 14:48 ` kernel test robot
2023-11-21 23:11 ` [PATCH v2 3/4] drm/amdkfd: Export DMABufs from KFD using GEM handles Felix Kuehling
2023-11-21 23:11 ` [PATCH v2 4/4] drm/amdkfd: Import DMABufs for interop through DRM Felix Kuehling
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.