[PATCH] drm/amd/amdgpu: Add helper functions for isp buffers
Nirujogi, Pratap
pnirujog at amd.com
Fri Jun 27 19:37:27 UTC 2025
Hi Mario,
On 6/27/2025 3:19 PM, Mario Limonciello wrote:
> On 6/27/2025 2:13 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 | 75 +++++++++++++++++++++-
>> 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, 122 insertions(+), 11 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..bbbca85d95d9 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
>> *
>> @@ -78,7 +80,7 @@ static int isp_load_fw_by_psp(struct amdgpu_device
>> *adev)
>> /* read isp fw */
>> r = amdgpu_ucode_request(adev, &adev->isp.fw,
>> AMDGPU_UCODE_OPTIONAL,
>> - "amdgpu/%s.bin", ucode_prefix);
>> + "amdgpu/%s.bin", ucode_prefix);
>
> Unrelated change, it should be it's own commit.
>
yeah, agreed, will remove here and will submit a new patch later for
checkpatch suggestions.
>> if (r) {
>> amdgpu_ucode_release(&adev->isp.fw);
>> return r;
>> @@ -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)
>> +{
>> + struct platform_device *ispdev = to_platform_device(dev);
>> + struct mfd_cell *mfd_cell = &ispdev->mfd_cell[0];
>> + 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;
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(isp_user_buffer_alloc);
>> +
>> +void isp_user_buffer_free(void *buf_obj)
>> +{
>> + 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)
>> +{
>> + struct platform_device *ispdev = to_platform_device(dev);
>> + 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 (!cpu_addr || ret) {
>
> Aren't these two different errors that deserve their own messages? IE
> how do you end up with a success return call but no cpu address?
>
thanks, checking for ret status is good enough, need not check for
cpu_addr explicitly. It is already covered in
amdgpu_bo_create_reserved(). Will fix it in the next verison.
https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c#L296
Thanks,
Pratap
>> + 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)
>> +{
>> + 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
>> @@ -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