Hi Mario,
On 6/27/2025 4:27 PM, Mario Limonciello wrote:
On 6/27/2025 3:17 PM, Pratap Nirujogi wrote:
Accessing amdgpu internal data structures "struct amdgpu_device"
and "struct amdgpu_bo" in ISP V4L2 driver to alloc/free GART
buffers is not recommended.
Add new amdgpu_isp helper functions thats takes opaque params
from ISP V4L2 driver and calls the amdgpu internal functions
amdgpu_bo_create_isp_user() and amdgpu_bo_create_kernel() to
alloc/free GART buffers.
Signed-off-by: Pratap Nirujogi <pratap.niruj...@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c | 73 ++++++++++++++++++++++
drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h | 7 +--
drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 4 --
drivers/gpu/drm/amd/amdgpu/isp.h | 47 ++++++++++++++
4 files changed, 121 insertions(+), 10 deletions(-)
create mode 100644 drivers/gpu/drm/amd/amdgpu/isp.h
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c b/drivers/gpu/
drm/amd/amdgpu/amdgpu_isp.c
index 43fc941dfa57..1b776c966bf2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c
@@ -33,6 +33,8 @@
#include "isp_v4_1_0.h"
#include "isp_v4_1_1.h"
+#define ISP_MC_ADDR_ALIGN (1024 * 32)
+
/**
* isp_hw_init - start and test isp block
*
@@ -141,6 +143,77 @@ static int isp_set_powergating_state(struct
amdgpu_ip_block *ip_block,
return 0;
}
+int isp_user_buffer_alloc(struct device *dev, void *dmabuf,
+ void **buf_obj, u64 *buf_addr)
As these are exported symbols intended for use outside of amdgpu, I
think adding kernel doc explaining how they work and what the arguments
are is a good idea.
I recall you had some of this on the old exports.
sure, will add the function headers as I did before for
amdgpu_bo_create_isp_user() / amdgpu_bo_free_isp_user / etc.
+{
+ struct platform_device *ispdev = to_platform_device(dev);
+ struct mfd_cell *mfd_cell = &ispdev->mfd_cell[0];
What happens if a caller calls this function on a system without an ISP?
Couldn't this be a NULL pointer deref?
So I think you need to catch the NULL ispdev case.
yeah, I completely agree, its required to add the checks to ensure dev
handle points to valid AMDGPU_ISP device. Will address this in the next
v3 version.
+ const struct isp_platform_data *isp_pdata;
+ struct amdgpu_device *adev;
+ struct amdgpu_bo *bo;
+ u64 gpu_addr;
+ int ret;
+
+ isp_pdata = mfd_cell->platform_data;
+ adev = isp_pdata->adev;
+
+ ret = amdgpu_bo_create_isp_user(adev, dmabuf,
+ AMDGPU_GEM_DOMAIN_GTT, &bo, &gpu_addr);
+ if (ret) {
+ drm_err(&adev->ddev, "failed to alloc gart user buffer (%d)",
ret);
+ return ret;
+ }
+
+ *buf_obj = (void *)bo;
+ *buf_addr = gpu_addr;
As this is from an external caller I think you should either WARN_ON()
at the beginning of the function or guard:
if (buf_obj)
*buf_obj =
if (buf_addr)
*buf_addr =
thanks, will add the checks in the next v3.
+
+ return 0;
+}
+EXPORT_SYMBOL(isp_user_buffer_alloc);
+
+void isp_user_buffer_free(void *buf_obj)
Same comment for kernel doc
Ack.
+{
+ amdgpu_bo_free_isp_user(buf_obj);
+}
+EXPORT_SYMBOL(isp_user_buffer_free);
+
+int isp_kernel_buffer_alloc(struct device *dev, u64 size,
+ void **buf_obj, u64 *gpu_addr, void **cpu_addr)
Same comment for kernel doc
Ack.
+{
+ struct platform_device *ispdev = to_platform_device(dev);
Same comment for NULL ispdev
Ack.
+ struct amdgpu_bo **bo = (struct amdgpu_bo **)buf_obj;
+ struct mfd_cell *mfd_cell = &ispdev->mfd_cell[0];
+ const struct isp_platform_data *isp_pdata;
+ struct amdgpu_device *adev;
+ int ret;
+
+ isp_pdata = mfd_cell->platform_data;
+ adev = isp_pdata->adev;
+
+ ret = amdgpu_bo_create_kernel(adev,
+ size,
+ ISP_MC_ADDR_ALIGN,
+ AMDGPU_GEM_DOMAIN_GTT,
+ bo,
+ gpu_addr,
+ cpu_addr);
+ if (ret) {
+ drm_err(&adev->ddev, "failed to alloc gart kernel buffer
(%d)", ret);
+ return ret;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(isp_kernel_buffer_alloc);
+
+void isp_kernel_buffer_free(void **buf_obj, u64 *gpu_addr, void
**cpu_addr)
Same comment for kernel doc
Ack.
+{
+ struct amdgpu_bo **bo = (struct amdgpu_bo **)buf_obj;
+
+ amdgpu_bo_free_kernel(bo, gpu_addr, cpu_addr);
+}
+EXPORT_SYMBOL(isp_kernel_buffer_free);
+
static const struct amd_ip_funcs isp_ip_funcs = {
.name = "isp_ip",
.early_init = isp_early_init,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h b/drivers/gpu/
drm/amd/amdgpu/amdgpu_isp.h
index 1d1c4b1ec7e7..cf26d283469e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h
@@ -29,17 +29,12 @@
#define __AMDGPU_ISP_H__
#include <linux/pm_domain.h>
+#include "isp.h"
#define ISP_REGS_OFFSET_END 0x629A4
struct amdgpu_isp;
-struct isp_platform_data {
- void *adev;
- u32 asic_type;
- resource_size_t base_rmmio_size;
-};
-
struct isp_funcs {
int (*hw_init)(struct amdgpu_isp *isp);
int (*hw_fini)(struct amdgpu_isp *isp);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/
drm/amd/amdgpu/amdgpu_object.c
index c5fda18967c8..122a88294883 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -352,7 +352,6 @@ int amdgpu_bo_create_kernel(struct amdgpu_device
*adev,
return 0;
}
-EXPORT_SYMBOL(amdgpu_bo_create_kernel);
/**
* amdgpu_bo_create_isp_user - create user BO for isp
@@ -421,7 +420,6 @@ int amdgpu_bo_create_isp_user(struct amdgpu_device
*adev,
return r;
}
-EXPORT_SYMBOL(amdgpu_bo_create_isp_user);
/**
* amdgpu_bo_create_kernel_at - create BO for kernel use at specific
location
@@ -525,7 +523,6 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo **bo,
u64 *gpu_addr,
if (cpu_addr)
*cpu_addr = NULL;
}
-EXPORT_SYMBOL(amdgpu_bo_free_kernel);
/**
* amdgpu_bo_free_isp_user - free BO for isp use
@@ -548,7 +545,6 @@ void amdgpu_bo_free_isp_user(struct amdgpu_bo *bo)
}
amdgpu_bo_unref(&bo);
}
-EXPORT_SYMBOL(amdgpu_bo_free_isp_user);
/* Validate bo size is bit bigger than the request domain */
static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
diff --git a/drivers/gpu/drm/amd/amdgpu/isp.h b/drivers/gpu/drm/amd/
amdgpu/isp.h
new file mode 100644
index 000000000000..6c8214ea28e1
--- /dev/null
+++ b/drivers/gpu/drm/amd/amdgpu/isp.h
I don't believe this is right location for this header. It needs to be
included by drivers outside of drm doesn't it?
So I think it should be in one of these locations:
include/drm/amd_isp.h
include/drm/amd/isp.h
yes, this file isp.h is included in the ISP V4L2 drivers, will take care
of moving to include/drm/amd/isp.h in the next version.
Thanks,
Pratap
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright (C) 2025 Advanced Micro Devices, Inc. All rights reserved.
+ * All Rights Reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person
obtaining a
+ * copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sub license, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT
SHALL
+ * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR
ANY CLAIM,
+ * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE
OR THE
+ * USE OR OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial
portions
+ * of the Software.
+ *
+ */
+
+#ifndef __ISP_H__
+#define __ISP_H__
+
+struct isp_platform_data {
+ void *adev;
+ u32 asic_type;
+ resource_size_t base_rmmio_size;
+};
+
+int isp_user_buffer_alloc(struct device *dev, void *dmabuf,
+ void **buf_obj, u64 *buf_addr);
+
+void isp_user_buffer_free(void *buf_obj);
+
+int isp_kernel_buffer_alloc(struct device *dev, u64 size,
+ void **buf_obj, u64 *gpu_addr, void **cpu_addr);
+
+void isp_kernel_buffer_free(void **buf_obj, u64 *gpu_addr, void
**cpu_addr);
+
+#endif
