Re: [PATCH v2 5/5] drm/amdgpu: do not resume device in thaw for normal hibernation

["Lazar, Lijo" <lijo.lazar@amd.com>]

On 7/7/2025 2:04 AM, Mario Limonciello wrote:
> On 7/4/2025 6:12 AM, Samuel Zhang wrote:
>> For normal hibernation, GPU do not need to be resumed in thaw since it
>> is not involved in writing the hibernation image. Skip resume in this
>> case can reduce the hibernation time.
> 
> Since you have the measurements would you mind including them in the
> commit message for reference?
> 
>>
>> For cancelled hibernation, GPU need to be resumed.
> 
> If I'm following right you are actually handling two different things in
> this patch aren't you?
> 
> 1) A change in thaw() to only resume on aborted hibernation
> 2) A change in shutdown() to skip running if the in s4 when shutdown()
> is called.
> 
> So I think it would be more logical to split this into two patches.
> 

This is doing only one thing - Keep the device in suspended state for
thaw() operation during a successful hibernation. Splitting into two
could break hibernation during integration of the first part - it will
attempt another suspend during shutdown. I think we don't take care of
consecutive suspend calls.

Thanks,
Lijo

>>
>> Signed-off-by: Samuel Zhang <guoqing.zhang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/
>> drm/amd/amdgpu/amdgpu_drv.c
>> index 4f8632737574..e064816aae4d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -2541,6 +2541,10 @@ amdgpu_pci_shutdown(struct pci_dev *pdev)
>>       if (amdgpu_ras_intr_triggered())
>>           return;
>>   +    /* device maybe not resumed here, return immediately in this
>> case */
>> +    if (adev->in_s4 && adev->in_suspend)
>> +        return;
>> +
>>       /* if we are running in a VM, make sure the device
>>        * torn down properly on reboot/shutdown.
>>        * unfortunately we can't detect certain
>> @@ -2655,6 +2659,10 @@ static int amdgpu_pmops_thaw(struct device *dev)
>>   {
>>       struct drm_device *drm_dev = dev_get_drvdata(dev);
>>   +    /* do not resume device for normal hibernation */
>> +    if (pm_transition.event == PM_EVENT_THAW)
>> +        return 0;
>> +
> 
> Without digging into pm.h documentation I think it's not going to be
> very obvious next time we look at this code that amdgpu_device_resume()
> is only intended for the aborted case.
> 
> How would you feel about a switch/case?
> 
> Something like this:
> 
> switch (pm_transition.event) {
> /* normal hibernation */
> case PM_EVENT_THAW:
>     return 0;
> /* for aborted hibernation */
> case PM_EVENT_RECOVER:
>     return amdgpu_device_resume(drm_dev, true);
> default:
>     return -EOPNOTSUP;
> }
> 
> 
>>       return amdgpu_device_resume(drm_dev, true);
>>   }
>>   
>