Re: [PATCH v5 2/5] drm/amd: Add an unwind for failures in amdgpu_device_ip_suspend_phase1()

["Mario Limonciello (AMD) (kernel.org)" <superm1@kernel.org>]

On 10/30/2025 10:18 AM, Alex Deucher wrote:
> On Thu, Oct 30, 2025 at 11:16 AM Mario Limonciello (AMD) (kernel.org)
> <superm1@kernel.org> wrote:
>>
>>
>>
>> On 10/30/2025 10:14 AM, Alex Deucher wrote:
>>> Patches 2-4 are:
>>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>>
>> Thanks!
>>
>> How about patch 1?  Patch 4 builds on it, so if that doesn't go in there
>> is another unwind step needed.
> 
> Oh, yeah, feel free to add my RB on that one as well, I guess it's not
> quite the same as the one I sent out originally.

OK Thanks.  Will queue up 1-4 and will drop #5 based on your comments 
from v4.

> 
> Alex
> 
>>
>>>
>>> On Sun, Oct 26, 2025 at 12:36 AM Mario Limonciello (AMD)
>>> <superm1@kernel.org> wrote:
>>>>
>>>> If any hardware IPs involved with the first phase of suspend fail, unwind
>>>> all steps to restore back to original state.
>>>>
>>>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 ++++++++++++++++--
>>>>    1 file changed, 16 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> index f6850b86e96f..b9ea91b2c92f 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> @@ -178,6 +178,7 @@ struct amdgpu_init_level amdgpu_init_minimal_xgmi = {
>>>>                   BIT(AMD_IP_BLOCK_TYPE_COMMON) | BIT(AMD_IP_BLOCK_TYPE_IH) |
>>>>                   BIT(AMD_IP_BLOCK_TYPE_PSP)
>>>>    };
>>>> +static int amdgpu_device_ip_resume_phase3(struct amdgpu_device *adev);
>>>>
>>>>    static void amdgpu_device_load_switch_state(struct amdgpu_device *adev);
>>>>
>>>> @@ -3784,7 +3785,7 @@ static void amdgpu_device_delay_enable_gfx_off(struct work_struct *work)
>>>>     */
>>>>    static int amdgpu_device_ip_suspend_phase1(struct amdgpu_device *adev)
>>>>    {
>>>> -       int i, r;
>>>> +       int i, r, rec;
>>>>
>>>>           amdgpu_device_set_pg_state(adev, AMD_PG_STATE_UNGATE);
>>>>           amdgpu_device_set_cg_state(adev, AMD_CG_STATE_UNGATE);
>>>> @@ -3807,10 +3808,23 @@ static int amdgpu_device_ip_suspend_phase1(struct amdgpu_device *adev)
>>>>
>>>>                   r = amdgpu_ip_block_suspend(&adev->ip_blocks[i]);
>>>>                   if (r)
>>>> -                       return r;
>>>> +                       goto unwind;
>>>>           }
>>>>
>>>>           return 0;
>>>> +unwind:
>>>> +       rec = amdgpu_device_ip_resume_phase3(adev);
>>>> +       if (rec)
>>>> +               dev_err(adev->dev,
>>>> +                       "amdgpu_device_ip_resume_phase3 failed during unwind: %d\n",
>>>> +                       rec);
>>>> +
>>>> +       amdgpu_dpm_set_df_cstate(adev, DF_CSTATE_ALLOW);
>>>> +
>>>> +       amdgpu_device_set_pg_state(adev, AMD_PG_STATE_GATE);
>>>> +       amdgpu_device_set_cg_state(adev, AMD_CG_STATE_GATE);
>>>> +
>>>> +       return r;
>>>>    }
>>>>
>>>>    /**
>>>> --
>>>> 2.51.1
>>>>
>>