[AMD Official Use Only - AMD Internal Distribution Only]
-----Original Message-----
From: Limonciello, Mario <mario.limoncie...@amd.com>
Sent: Wednesday, September 3, 2025 2:43 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>; Alexey Zagorodnikov <xglo...@gmail.com>
Subject: Re: [PATCH] drm/amd/amdgpu: Declare isp firmware binary file
On 9/3/25 1:33 PM, Nirujogi, Pratap wrote:
> Declare isp firmware file isp_4_1_1.bin required by isp4.1.1 device.
>
> Suggested-by: Alexey Zagorodnikov <xglo...@gmail.com>
> Signed-off-by: Pratap Nirujogi <pratap.niruj...@amd.com>
I think you should split this into multiple patches. Just one patch to add the
declaration, and another patch to change the error handling.
Hi Mario, yes, this can be split into two patches (a) update fw load sequence
(b) declare isp firmware file. Will address it in v2.
Thanks,
Pratap
> ---
> 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 | 14 +++++++++++++-
> drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.h | 2 +-
> 6 files changed, 37 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..1e48d94e8706 100644
> --- a/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/isp_v4_1_1.c
> @@ -29,6 +29,8 @@
> #include "amdgpu.h"
> #include "isp_v4_1_1.h"
>
> +MODULE_FIRMWARE("amdgpu/isp_4_1_1.bin");
> +
> #define ISP_PERFORMANCE_STATE_LOW 0
> #define ISP_PERFORMANCE_STATE_HIGH 1
>
> @@ -369,7 +371,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
