[PATCH v2 2/3] drm/amd/amdgpu: Add ISP driver support
Nirujogi, Pratap
Pratap.Nirujogi at amd.com
Thu May 9 22:13:04 UTC 2024
[AMD Official Use Only - General]
Hi Mario / Alex,
Please see inline for below comment, others I addressed and submitted patch v3.
RE: I think this is the wrong place for this isp_ip_block and also it should be named differently. Maybe it should be named isp_ip_block_v4_1 and then in a different file?
Maybe Alex can confirm how he would rather see it done.
>> In amdgpu_isp all we are doing is: (a) load isp fw (b) add isp as MFD device; and anything specific to IP version 4.1.x is taken care in the v4l2 isp driver. We can consider adding private implementation for each ISP ip version in amdgpu if it requires special handling on top of the generic amdgpu_isp + mfd changes. Please let us know your thoughts.
Thanks,
Pratap
-----Original Message-----
From: Limonciello, Mario <Mario.Limonciello at amd.com>
Sent: Thursday, May 9, 2024 3:55 PM
To: Nirujogi, Pratap <Pratap.Nirujogi at amd.com>; amd-gfx at lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Limonciello, Mario <Mario.Limonciello at amd.com>; Chan, Benjamin (Koon Pan) <Benjamin.Chan at amd.com>; Li, King <King.Li at amd.com>; Du, Bin <Bin.Du at amd.com>
Subject: Re: [PATCH v2 2/3] drm/amd/amdgpu: Add ISP driver support
On 5/9/2024 14:35, Pratap Nirujogi wrote:
> Add the isp driver in amdgpu to support ISP device on the APUs that
> supports ISP IP block. ISP hw block is used for camera front-end, pre
> and post processing operations.
>
> Signed-off-by: Pratap Nirujogi <pratap.nirujogi at amd.com<mailto:pratap.nirujogi at amd.com>>
> Change-Id: I67ef206e5eca1fe74e495c3262746be495e17d09
Ttypically we strip the Change-Id for things that were in Gerrit originally.
> ---
> Changes in v2:
> - Remove adding IORESOURCE_IRQ
> - Remove noisy info prints
> - Avoid isp version numbers while naming
> - Fix incorrect argument description
> - Replace // with /* */ for comemnts
>
> drivers/gpu/drm/amd/amdgpu/Makefile | 3 +
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c | 287 ++++++++++++++++++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h | 54 ++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 3 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 5 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h | 1 +
> 7 files changed, 357 insertions(+)
> create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c
> create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile
> b/drivers/gpu/drm/amd/amdgpu/Makefile
> index de7b76327f5b..12ba76025cb7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
> @@ -324,4 +324,7 @@ amdgpu-y += $(AMD_DISPLAY_FILES)
>
> endif
>
> +# add isp block
> +amdgpu-y += amdgpu_isp.o
> +
> obj-$(CONFIG_DRM_AMDGPU)+= amdgpu.o
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index eb60d28a3a13..6d7f9ef53269 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -112,6 +112,7 @@
> #include "amdgpu_xcp.h"
> #include "amdgpu_seq64.h"
> #include "amdgpu_reg_state.h"
> +#include "amdgpu_isp.h"
>
> #define MAX_GPU_INSTANCE 64
>
> @@ -1045,6 +1046,9 @@ struct amdgpu_device {
> /* display related functionality */
> struct amdgpu_display_manager dm;
>
> + /* isp */
> + struct amdgpu_isp isp;
> +
> /* mes */
> bool enable_mes;
> bool enable_mes_kiq;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c
> new file mode 100644
> index 000000000000..c28d90c25b10
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c
> @@ -0,0 +1,287 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (C) 2024 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.
> + *
> + */
> +
> +#include <linux/irqdomain.h>
> +#include <linux/pm_domain.h>
> +#include <linux/platform_device.h>
> +#include <sound/designware_i2s.h>
> +#include <sound/pcm.h>
> +#include <linux/acpi.h>
> +#include <linux/firmware.h>
This list of headers should be sorted alphabetically.
> +
> +#include "amdgpu_smu.h"
> +#include "atom.h"
> +#include "amdgpu_isp.h"
> +#include "smu_internal.h"
> +#include "smu_v11_5_ppsmc.h"
> +#include "smu_v11_5_pmfw.h"
> +
> +#define mmDAGB0_WRCLI5_V4_1 0x6811C
> +#define mmDAGB0_WRCLI9_V4_1 0x6812C
> +#define mmDAGB0_WRCLI10_V4_1 0x68130
> +#define mmDAGB0_WRCLI14_V4_1 0x68140
> +#define mmDAGB0_WRCLI19_V4_1 0x68154
> +#define mmDAGB0_WRCLI20_V4_1 0x68158
> +
> +static int isp_sw_init(void *handle)
> +{
> + struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> +
> + adev->isp.parent = adev->dev;
> +
> + adev->isp.cgs_device = amdgpu_cgs_create_device(adev);
> + if (!adev->isp.cgs_device)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int isp_sw_fini(void *handle)
> +{
> + struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> +
> + if (adev->isp.cgs_device)
> + amdgpu_cgs_destroy_device(adev->isp.cgs_device);
> +
> + return 0;
> +}
> +
> +/**
> + * isp_hw_init - start and test isp block
> + *
> + * @handle: handle for amdgpu_device pointer
> + *
> + */
> +static int isp_hw_init(void *handle)
> +{
> + int r;
> + u64 isp_base;
> + struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> +
Spurious new line here
> + const struct amdgpu_ip_block *ip_block =
> + amdgpu_device_ip_get_ip_block(adev, AMD_IP_BLOCK_TYPE_ISP);
> +
> + if (!ip_block)
> + return -EINVAL;
> +
> + if (adev->rmmio_size == 0 || adev->rmmio_size < 0x5289)
> + return -EINVAL;
> +
> + isp_base = adev->rmmio_base;
> +
> + adev->isp.isp_cell = kcalloc(1, sizeof(struct mfd_cell), GFP_KERNEL);
> + if (!adev->isp.isp_cell) {
> + r = -ENOMEM;
> + DRM_ERROR("%s: isp mfd cell alloc failed\n", __func__);
> + goto failure;
> + }
> +
> + adev->isp.isp_res = kcalloc(1, sizeof(struct resource), GFP_KERNEL);
> + if (!adev->isp.isp_res) {
> + r = -ENOMEM;
> + DRM_ERROR("%s: isp mfd res alloc failed\n", __func__);
> + goto failure;
> + }
> +
> + adev->isp.isp_pdata = kzalloc(sizeof(*adev->isp.isp_pdata), GFP_KERNEL);
> + if (!adev->isp.isp_pdata) {
> + r = -ENOMEM;
> + DRM_ERROR("%s: isp platform data alloc failed\n", __func__);
> + goto failure;
> + }
> +
> + /* initialize isp platform data */
> + adev->isp.isp_pdata->adev = (void *)adev;
> + adev->isp.isp_pdata->asic_type = adev->asic_type;
> + adev->isp.isp_pdata->base_rmmio_size = adev->rmmio_size;
> +
> + adev->isp.isp_res[0].name = "isp_reg";
> + adev->isp.isp_res[0].flags = IORESOURCE_MEM;
> + adev->isp.isp_res[0].start = isp_base;
> + adev->isp.isp_res[0].end = isp_base + ISP_REGS_OFFSET_END;
> +
> + adev->isp.isp_cell[0].name = "amd_isp_capture";
> + adev->isp.isp_cell[0].num_resources = 1;
> + adev->isp.isp_cell[0].resources = &adev->isp.isp_res[0];
> + adev->isp.isp_cell[0].platform_data = adev->isp.isp_pdata;
> + adev->isp.isp_cell[0].pdata_size = sizeof(struct isp_platform_data);
> +
> + r = mfd_add_hotplug_devices(adev->isp.parent, adev->isp.isp_cell, 1);
> + if (r) {
> + DRM_ERROR("%s: add mfd hotplug device failed\n", __func__);
> + goto failure;
> + }
> +
> + /* Temporary WA added to disable MMHUB TLSi until the GART initialization
> + is ready to support MMHUB TLSi and SAW for ISP HW to access GART memory
> + using the TLSi path */
Linux "multi-line comment style" is
/*
* Foo the bar
* Bar the foo
*/
> + cgs_write_register(adev->isp.cgs_device, mmDAGB0_WRCLI5_V4_1 >> 2, 0xFE5FEAA8);
> + cgs_write_register(adev->isp.cgs_device, mmDAGB0_WRCLI9_V4_1 >> 2, 0xFE5FEAA8);
> + cgs_write_register(adev->isp.cgs_device, mmDAGB0_WRCLI10_V4_1 >> 2, 0xFE5FEAA8);
> + cgs_write_register(adev->isp.cgs_device, mmDAGB0_WRCLI14_V4_1 >> 2, 0xFE5FEAA8);
> + cgs_write_register(adev->isp.cgs_device, mmDAGB0_WRCLI19_V4_1 >> 2, 0xFE5FEAA8);
> + cgs_write_register(adev->isp.cgs_device, mmDAGB0_WRCLI20_V4_1 >> 2,
> +0xFE5FEAA8);
> +
> + return 0;
> +
> +failure:
> +
> + kfree(adev->isp.isp_pdata);
> + kfree(adev->isp.isp_res);
> + kfree(adev->isp.isp_cell);
> +
> + return r;
> +}
> +
> +/**
> + * isp_hw_fini - stop the hardware block
> + *
> + * @handle: handle for amdgpu_device pointer
> + *
> + */
> +static int isp_hw_fini(void *handle)
> +{
> + struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> +
> + /* remove isp mfd device */
> + mfd_remove_devices(adev->isp.parent);
> +
> + kfree(adev->isp.isp_res);
> + kfree(adev->isp.isp_cell);
> + kfree(adev->isp.isp_pdata);
> +
> + return 0;
> +}
> +
> +static int isp_suspend(void *handle)
> +{
> + return 0;
> +}
> +
> +static int isp_resume(void *handle)
> +{
> + return 0;
> +}
> +
> +static int isp_load_fw_by_psp(struct amdgpu_device *adev) {
> + const struct common_firmware_header *hdr;
> + char ucode_prefix[30];
> + char fw_name[40];
> + int r = 0;
> +
> + /* get isp fw binary name and path */
> + amdgpu_ucode_ip_version_decode(adev, ISP_HWIP, ucode_prefix,
> + sizeof(ucode_prefix));
> + snprintf(fw_name, sizeof(fw_name), "amdgpu/%s.bin", ucode_prefix);
> +
> + /* read isp fw */
> + r = amdgpu_ucode_request(adev, &adev->isp.fw, fw_name);
> + if (r) {
> + DRM_ERROR("%s: failed to read isp fw %s\n", __func__, fw_name);
I think you should lose this DRM_ERROR() statement.
when isp_early_init() is called with the file missing you'll see this error as well as the DRM_WARN() from isp_early_init().
We should show exactly one message for problems like this. Since it's not fatal I think we should only show the DRM_WARN() message.
> + amdgpu_ucode_release(&adev->isp.fw);
> + return r;
> + }
> +
> + hdr = (const struct common_firmware_header *)adev->isp.fw->data;
> +
> + adev->firmware.ucode[AMDGPU_UCODE_ID_ISP].ucode_id =
> + AMDGPU_UCODE_ID_ISP;
> + adev->firmware.ucode[AMDGPU_UCODE_ID_ISP].fw = adev->isp.fw;
> +
> + adev->firmware.fw_size +=
> + ALIGN(le32_to_cpu(hdr->ucode_size_bytes), PAGE_SIZE);
> +
> + return r;
> +}
> +
> +static int isp_early_init(void *handle) {
> + int ret = 0;
> + struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> +
> + ret = isp_load_fw_by_psp(adev);
> + if (ret) {
> + DRM_WARN("%s: isp fw load failed %d\n", __func__, ret);
> + /* allow amdgpu init to proceed though isp fw load fails */
> + ret = 0;
> + }
> +
> + return ret;
> +}
> +
> +static bool isp_is_idle(void *handle) {
> + return true;
> +}
> +
> +static int isp_wait_for_idle(void *handle) {
> + return 0;
> +}
> +
> +static int isp_soft_reset(void *handle) {
> + return 0;
> +}
> +
> +static int isp_set_clockgating_state(void *handle,
> + enum amd_clockgating_state state) {
> + return 0;
> +}
> +
> +static int isp_set_powergating_state(void *handle,
> + enum amd_powergating_state state) {
> + return 0;
> +}
> +
> +static const struct amd_ip_funcs isp_ip_funcs = {
> + .name = "isp_ip",
> + .early_init = isp_early_init,
> + .late_init = NULL,
> + .sw_init = isp_sw_init,
> + .sw_fini = isp_sw_fini,
> + .hw_init = isp_hw_init,
> + .hw_fini = isp_hw_fini,
> + .suspend = isp_suspend,
> + .resume = isp_resume,
> + .is_idle = isp_is_idle,
> + .wait_for_idle = isp_wait_for_idle,
> + .soft_reset = isp_soft_reset,
> + .set_clockgating_state = isp_set_clockgating_state,
> + .set_powergating_state = isp_set_powergating_state, };
> +
> +const struct amdgpu_ip_block_version isp_ip_block = {
> + .type = AMD_IP_BLOCK_TYPE_ISP,
> + .major = 4,
> + .minor = 1,
> + .rev = 0,
> + .funcs = &isp_ip_funcs,
> +};
I think this is the wrong place for this isp_ip_block and also it should be named differently. Maybe it should be named isp_ip_block_v4_1 and then in a different file?
Maybe Alex can confirm how he would rather see it done.
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h
> new file mode 100644
> index 000000000000..2adcb4b4dc6e
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (C) 2024 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 __AMDGPU_ISP_H__
> +#define __AMDGPU_ISP_H__
> +
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/core.h>
> +
> +#define ISP_REGS_OFFSET_END 0x629A4
> +
> +struct isp_platform_data {
> + void *adev;
> + u32 asic_type;
> + resource_size_t base_rmmio_size;
> +};
> +
> +struct amdgpu_isp {
> + struct device *parent;
> + struct cgs_device *cgs_device;
> + struct mfd_cell *isp_cell;
> + struct resource *isp_res;
> + struct isp_platform_data *isp_pdata;
> + unsigned int harvest_config;
> + const struct firmware *fw;
> +};
> +
> +extern const struct amdgpu_ip_block_version isp_ip_block;
> +
> +#endif /* __AMDGPU_ISP_H__ */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> index 37820dd03cab..b4bd943a7cc8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> @@ -2539,6 +2539,9 @@ static int psp_get_fw_type(struct amdgpu_firmware_info *ucode,
> case AMDGPU_UCODE_ID_JPEG_RAM:
> *type = GFX_FW_TYPE_JPEG_RAM;
> break;
> + case AMDGPU_UCODE_ID_ISP:
> + *type = GFX_FW_TYPE_ISP;
> + break;
> case AMDGPU_UCODE_ID_MAXIMUM:
> default:
> return -EINVAL;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> index 75ece8a2f96b..a9de78bb96e2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> @@ -712,6 +712,8 @@ const char *amdgpu_ucode_name(enum AMDGPU_UCODE_ID ucode_id)
> return "RS64_MEC_P2_STACK";
> case AMDGPU_UCODE_ID_CP_RS64_MEC_P3_STACK:
> return "RS64_MEC_P3_STACK";
> + case AMDGPU_UCODE_ID_ISP:
> + return "ISP";
> default:
> return "UNKNOWN UCODE";
> }
> @@ -1411,6 +1413,9 @@ void amdgpu_ucode_ip_version_decode(struct amdgpu_device *adev, int block_type,
> case VPE_HWIP:
> ip_name = "vpe";
> break;
> + case ISP_HWIP:
> + ip_name = "isp";
> + break;
> default:
> BUG();
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
> index a3c04f711099..db745ab7b0c8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
> @@ -523,6 +523,7 @@ enum AMDGPU_UCODE_ID {
> AMDGPU_UCODE_ID_UMSCH_MM_CMD_BUFFER,
> AMDGPU_UCODE_ID_P2S_TABLE,
> AMDGPU_UCODE_ID_JPEG_RAM,
> + AMDGPU_UCODE_ID_ISP,
> AMDGPU_UCODE_ID_MAXIMUM,
> };
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20240509/c144d1ec/attachment-0001.htm>
More information about the amd-gfx
mailing list