On 9/4/25 11:35 AM, Nirujogi, Pratap wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> -----Original Message-----
> From: Alex Deucher <alexdeuc...@gmail.com>
> Sent: Thursday, September 4, 2025 9:41 AM
> To: Limonciello, Mario <mario.limoncie...@amd.com>
> Cc: Nirujogi, Pratap <pratap.niruj...@amd.com>;
> amd-gfx@lists.freedesktop.org; Deucher, Alexander
> <alexander.deuc...@amd.com>; Koenig, Christian <christian.koe...@amd.com>;
> 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
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Wed, Sep 3, 2025 at 5:14 PM Limonciello, Mario <mario.limoncie...@amd.com>
> wrote:
>>
>> 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 think it depends a bit on the individual IP. VCN has all of the firmware
> handling in a common helper because it's common for all VCN versions. Each
> GFX IP has has its own firmware handling because there are IP specific
> handlings required. I think it's fine to keep the ISP firwmare handling in
> once place unless you feel it would make sense to have it per IP version for
> some reason going forward.
>
> Hi Alex, thanks for your feedback. I see amdgpu_isp implementation is closely
> related amdgpu_vcn and I'm fine moving the declaration of all ISP firmware
> files to amdgpu_isp file instead of keeping them in IP specific source files.
> If this approach is acceptable, I’ll submit a new patch that adds
> MODULE_FIRMWARE in amdgpu_isp and reverts all other changes introduced in v2.
IMO - the MODULE_FIRMWARE() macro location doesn't really matter. All
the files link together and the macro is used to put the information on
the module itself. That is you could just have your v2 2/2 patch and
things should be fine.
It feels more logical to me to put it in the IP specific version, but
totally up to you.
>
> Thanks,
> Pratap
>
> Alex
>
>>
>> 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
>>>
>>
