[AMD Official Use Only - AMD Internal Distribution Only]
Thanks Mario. I will fix the typo too while submitting the patch.
-----Original Message-----
From: Limonciello, Mario <mario.limoncie...@amd.com>
Sent: Sunday, July 6, 2025 9:56 PM
To: Nirujogi, Pratap <pratap.niruj...@amd.com>; amd-gfx@lists.freedesktop.org;
Deucher, Alexander <alexander.deuc...@amd.com>; Koenig, Christian
<christian.koe...@amd.com>; Limonciello, Mario <mario.limoncie...@amd.com>
Cc: Chan, Benjamin (Koon Pan) <benjamin.c...@amd.com>; Du, Bin
<bin...@amd.com>; Rosikopulos, Gjorgji <gjorgji.rosikopu...@amd.com>; Li, King
<king...@amd.com>; Antony, Dominic <dominic.ant...@amd.com>; Jawich, Phil
<phil.jaw...@amd.com>
Subject: Re: [PATCH v4] drm/amd/amdgpu: Add helper functions for isp buffers
On 6/30/2025 5:33 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
s/thats/that/
> 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>
No need to respin for the typo above if this is the only feedback.
Reviewed-by: Mario Limonciello <mario.limoncie...@amd.com>
> ---
> Changes v3 -> v4:
>
> * Remove unrelated change in isp_load_fw_by_psp()
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c | 175 +++++++++++++++++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h | 7 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 4 -
> include/drm/amd/isp.h | 51 ++++++
> 4 files changed, 227 insertions(+), 10 deletions(-)
> create mode 100644 include/drm/amd/isp.h
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c
> index 43fc941dfa57..f9cabeae1c71 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,179 @@ static int isp_set_powergating_state(struct
> amdgpu_ip_block *ip_block,
> return 0;
> }
>
> +static int is_valid_isp_device(struct device *isp_parent, struct
> +device *amdgpu_dev) {
> + if (isp_parent != amdgpu_dev)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +/**
> + * isp_user_buffer_alloc - create user buffer object (BO) for isp
> + *
> + * @dev: isp device handle
> + * @dma_buf: DMABUF handle for isp buffer allocated in system memory
> + * @buf_obj: GPU buffer object handle to initialize
> + * @buf_addr: GPU addr of the pinned BO to initialize
> + *
> + * Imports isp DMABUF to allocate and pin a user BO for isp internal
> +use. It does
> + * GART alloc to generate GPU addr for BO to make it accessible
> +through the
> + * GART aperture for ISP HW.
> + *
> + * This function is exported to allow the V4L2 isp device external to
> +drm device
> + * to create and access the isp user BO.
> + *
> + * Returns:
> + * 0 on success, negative error code otherwise.
> + */
> +int isp_user_buffer_alloc(struct device *dev, void *dmabuf,
> + void **buf_obj, u64 *buf_addr)
> +{
> + struct platform_device *ispdev = to_platform_device(dev);
> + const struct isp_platform_data *isp_pdata;
> + struct amdgpu_device *adev;
> + struct mfd_cell *mfd_cell;
> + struct amdgpu_bo *bo;
> + u64 gpu_addr;
> + int ret;
> +
> + if (WARN_ON(!ispdev))
> + return -ENODEV;
> +
> + if (WARN_ON(!buf_obj))
> + return -EINVAL;
> +
> + if (WARN_ON(!buf_addr))
> + return -EINVAL;
> +
> + mfd_cell = &ispdev->mfd_cell[0];
> + if (!mfd_cell)
> + return -ENODEV;
> +
> + isp_pdata = mfd_cell->platform_data;
> + adev = isp_pdata->adev;
> +
> + ret = is_valid_isp_device(ispdev->dev.parent, adev->dev);
> + if (ret)
> + return ret;
> +
> + 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);
> +
> +/**
> + * isp_user_buffer_free - free isp user buffer object (BO)
> + *
> + * @buf_obj: amdgpu isp user BO to free
> + *
> + * unpin and unref BO for isp internal use.
> + *
> + * This function is exported to allow the V4L2 isp device
> + * external to drm device to free the isp user BO.
> + */
> +void isp_user_buffer_free(void *buf_obj) {
> + amdgpu_bo_free_isp_user(buf_obj);
> +}
> +EXPORT_SYMBOL(isp_user_buffer_free);
> +
> +/**
> + * isp_kernel_buffer_alloc - create kernel buffer object (BO) for isp
> + *
> + * @dev: isp device handle
> + * @size: size for the new BO
> + * @buf_obj: GPU BO handle to initialize
> + * @gpu_addr: GPU addr of the pinned BO
> + * @cpu_addr: CPU address mapping of BO
> + *
> + * Allocates and pins a kernel BO for internal isp firmware use.
> + *
> + * This function is exported to allow the V4L2 isp device
> + * external to drm device to create and access the kernel BO.
> + *
> + * Returns:
> + * 0 on success, negative error code otherwise.
> + */
> +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;
> + const struct isp_platform_data *isp_pdata;
> + struct amdgpu_device *adev;
> + struct mfd_cell *mfd_cell;
> + int ret;
> +
> + if (WARN_ON(!ispdev))
> + return -ENODEV;
> +
> + if (WARN_ON(!buf_obj))
> + return -EINVAL;
> +
> + if (WARN_ON(!gpu_addr))
> + return -EINVAL;
> +
> + if (WARN_ON(!cpu_addr))
> + return -EINVAL;
> +
> + mfd_cell = &ispdev->mfd_cell[0];
> + if (!mfd_cell)
> + return -ENODEV;
> +
> + isp_pdata = mfd_cell->platform_data;
> + adev = isp_pdata->adev;
> +
> + ret = is_valid_isp_device(ispdev->dev.parent, adev->dev);
> + if (ret)
> + return ret;
> +
> + ret = amdgpu_bo_create_kernel(adev,
> + size,
> + ISP_MC_ADDR_ALIGN,
> + AMDGPU_GEM_DOMAIN_GTT,
> + bo,
> + gpu_addr,
> + cpu_addr);
> + if (!cpu_addr || ret) {
> + drm_err(&adev->ddev, "failed to alloc gart kernel buffer (%d)",
> ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(isp_kernel_buffer_alloc);
> +
> +/**
> + * isp_kernel_buffer_free - free isp kernel buffer object (BO)
> + *
> + * @buf_obj: amdgpu isp user BO to free
> + * @gpu_addr: GPU addr of isp kernel BO
> + * @cpu_addr: CPU addr of isp kernel BO
> + *
> + * unmaps and unpin a isp kernel BO.
> + *
> + * This function is exported to allow the V4L2 isp device
> + * external to drm device to free the kernel BO.
> + */
> +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..d6f4ffa4c97c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h
> @@ -28,18 +28,13 @@
> #ifndef __AMDGPU_ISP_H__
> #define __AMDGPU_ISP_H__
>
> +#include <drm/amd/isp.h>
> #include <linux/pm_domain.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/include/drm/amd/isp.h b/include/drm/amd/isp.h new file mode
> 100644 index 000000000000..ec868288abf2
> --- /dev/null
> +++ b/include/drm/amd/isp.h
> @@ -0,0 +1,51 @@
> +/* 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__
> +
> +#include <linux/types.h>
> +
> +struct device;
> +
> +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
