On 9/3/25 4:01 PM, Nirujogi, Pratap wrote:
> [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.).
>
I guess I don't see a strong argument for doing it, but let's see what
others think.
> 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
>
