[PATCH v2] drm/amd/amdgpu: Add helper functions for isp buffers
Nirujogi, Pratap
pnirujog at amd.com
Fri Jun 27 22:05:35 UTC 2025
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.nirujogi at 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
>
More information about the amd-gfx
mailing list