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.
> ---
> 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
