[AMD Official Use Only - AMD Internal Distribution Only]
-----Original Message-----
From: Limonciello, Mario <mario.limoncie...@amd.com>
Sent: Wednesday, September 3, 2025 4:40 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>; xglo...@gmail.com
Subject: Re: [PATCH v2 1/2] drm/amd/amdgpu: Move isp firmware load into
isp_v4_1_x modules
On 9/3/25 3:35 PM, Nirujogi, Pratap wrote:
> Move isp firmware load from generic amdgpu_isp driver to isp version
> specific driver modules isp_v4_1_0 and isp_v4_1_1.
I don't really understand why to do this change. Isn't it just more code
duplication with this patch?
Hi Mario, I have added this to show the reference of calling the fw load in the
same file where MODULE_FIRMWARE is added. This aligns with the approach
followed in other drivers (amdgpu_vcn, gfx_v11_0 etc.).
Thanks,
Pratap
>
> Signed-off-by: Pratap Nirujogi <pratap.niruj...@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c | 22 +++++++++-------------
> drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h | 2 ++
> drivers/gpu/drm/amd/amdgpu/isp_v4_1_0.c | 12 +++++++++++-
> drivers/gpu/drm/amd/amdgpu/isp_v4_1_0.h | 2 +-
> drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c | 12 +++++++++++-
> drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.h | 2 +-
> 6 files changed, 35 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c
> index 9cddbf50442a..90af35ee8f6e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.c
> @@ -68,7 +68,7 @@ static int isp_hw_fini(struct amdgpu_ip_block *ip_block)
> return -ENODEV;
> }
>
> -static int isp_load_fw_by_psp(struct amdgpu_device *adev)
> +int isp_load_fw_by_psp(struct amdgpu_device *adev)
> {
> const struct common_firmware_header *hdr;
> char ucode_prefix[10];
> @@ -80,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);
> if (r) {
> amdgpu_ucode_release(&adev->isp.fw);
> return r;
> @@ -103,27 +103,23 @@ static int isp_early_init(struct amdgpu_ip_block
> *ip_block)
>
> struct amdgpu_device *adev = ip_block->adev;
> struct amdgpu_isp *isp = &adev->isp;
> + int r;
> +
> + isp->adev = adev;
> + isp->parent = adev->dev;
>
> switch (amdgpu_ip_version(adev, ISP_HWIP, 0)) {
> case IP_VERSION(4, 1, 0):
> - isp_v4_1_0_set_isp_funcs(isp);
> + r = isp_v4_1_0_set_isp_funcs(isp);
> break;
> case IP_VERSION(4, 1, 1):
> - isp_v4_1_1_set_isp_funcs(isp);
> + r = isp_v4_1_1_set_isp_funcs(isp);
> break;
> default:
> return -EINVAL;
> }
>
> - isp->adev = adev;
> - isp->parent = adev->dev;
> -
> - if (isp_load_fw_by_psp(adev)) {
> - DRM_DEBUG_DRIVER("%s: isp fw load failed\n", __func__);
> - return -ENOENT;
> - }
> -
> - return 0;
> + return r;
> }
>
> static bool isp_is_idle(struct amdgpu_ip_block *ip_block) diff --git
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h
> index d6f4ffa4c97c..36750123ed46 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_isp.h
> @@ -57,4 +57,6 @@ struct amdgpu_isp {
> extern const struct amdgpu_ip_block_version isp_v4_1_0_ip_block;
> extern const struct amdgpu_ip_block_version isp_v4_1_1_ip_block;
>
> +int isp_load_fw_by_psp(struct amdgpu_device *adev);
> +
> #endif /* __AMDGPU_ISP_H__ */
> diff --git a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_0.c
> b/drivers/gpu/drm/amd/amdgpu/isp_v4_1_0.c
> index 0027a639c7e6..926947a612a4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/isp_v4_1_0.c
> @@ -185,7 +185,17 @@ static const struct isp_funcs isp_v4_1_0_funcs = {
> .hw_fini = isp_v4_1_0_hw_fini,
> };
>
> -void isp_v4_1_0_set_isp_funcs(struct amdgpu_isp *isp)
> +int isp_v4_1_0_set_isp_funcs(struct amdgpu_isp *isp)
> {
> + struct amdgpu_device *adev = isp->adev;
> +
> isp->funcs = &isp_v4_1_0_funcs;
> +
> + /* load isp firmware */
> + if (isp_load_fw_by_psp(adev)) {
> + drm_err(&adev->ddev, "isp fw load failed\n");
> + return -ENOENT;
> + }
> +
> + return 0;
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_0.h
> b/drivers/gpu/drm/amd/amdgpu/isp_v4_1_0.h
> index 4d239198edd0..5e43ba064708 100644
> --- a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_0.h
> +++ b/drivers/gpu/drm/amd/amdgpu/isp_v4_1_0.h
> @@ -45,6 +45,6 @@
> #define ISP410_GPIO_SENSOR_OFFSET 0x6613C
> #define ISP410_GPIO_SENSOR_SIZE 0x54
>
> -void isp_v4_1_0_set_isp_funcs(struct amdgpu_isp *isp);
> +int isp_v4_1_0_set_isp_funcs(struct amdgpu_isp *isp);
>
> #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
> b/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
> index a887df520414..9766c6056dc4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
> @@ -369,7 +369,17 @@ static const struct isp_funcs isp_v4_1_1_funcs = {
> .hw_fini = isp_v4_1_1_hw_fini,
> };
>
> -void isp_v4_1_1_set_isp_funcs(struct amdgpu_isp *isp)
> +int isp_v4_1_1_set_isp_funcs(struct amdgpu_isp *isp)
> {
> + struct amdgpu_device *adev = isp->adev;
> +
> isp->funcs = &isp_v4_1_1_funcs;
> +
> + /* load isp firmware */
> + if (isp_load_fw_by_psp(adev)) {
> + drm_err(&adev->ddev, "isp fw load failed\n");
> + return -ENOENT;
> + }
> +
> + return 0;
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.h
> b/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.h
> index fe45d70d87f1..b221d8b81983 100644
> --- a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.h
> +++ b/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.h
> @@ -44,6 +44,6 @@
> #define ISP411_GPIO_SENSOR_OFFSET 0x6613C
> #define ISP411_GPIO_SENSOR_SIZE 0x54
>
> -void isp_v4_1_1_set_isp_funcs(struct amdgpu_isp *isp);
> +int isp_v4_1_1_set_isp_funcs(struct amdgpu_isp *isp);
>
> #endif
