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