[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.limoncie...@amd.com> Sent: Thursday, May 9, 2024 3:55 PM To: Nirujogi, Pratap <pratap.niruj...@amd.com>; amd-gfx@lists.freedesktop.org Cc: Deucher, Alexander <alexander.deuc...@amd.com>; Limonciello, Mario <mario.limoncie...@amd.com>; Chan, Benjamin (Koon Pan) <benjamin.c...@amd.com>; Li, King <king...@amd.com>; Du, Bin <bin...@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.niruj...@amd.com<mailto:pratap.niruj...@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, > }; >
