[PATCH v2] drm/amdgpu/pm: Fix the power source flag error

[Lazar, Lijo]

On 1/18/2024 2:31 PM, Ma, Jun wrote:
> 
> 
> On 1/18/2024 4:38 PM, Lazar, Lijo wrote:
>> On 1/18/2024 12:57 PM, Ma Jun wrote:
>>> The power source flag should be updated when
>>> [1] System receives an interrupt indicating that the power source
>>> has changed.
>>> [2] System resumes from suspend or runtime suspend
>>>
>>> Signed-off-by: Ma Jun <Jun.Ma2 at amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c      | 13 +++----------
>>>    drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c |  2 ++
>>>    drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c |  2 ++
>>>    3 files changed, 7 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> index c16703868e5c..a54663f2e2ab 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> @@ -24,6 +24,7 @@
>>>    
>>>    #include <linux/firmware.h>
>>>    #include <linux/pci.h>
>>> +#include <linux/power_supply.h>
>>>    #include <linux/reboot.h>
>>>    
>>>    #include "amdgpu.h"
>>> @@ -817,16 +818,8 @@ static int smu_late_init(void *handle)
>>>    	 * handle the switch automatically. Driver involvement
>>>    	 * is unnecessary.
>>>    	 */
>>> -	if (!smu->dc_controlled_by_gpio) {
>>> -		ret = smu_set_power_source(smu,
>>> -					   adev->pm.ac_power ? SMU_POWER_SOURCE_AC :
>>> -					   SMU_POWER_SOURCE_DC);
>>> -		if (ret) {
>>> -			dev_err(adev->dev, "Failed to switch to %s mode!\n",
>>> -				adev->pm.ac_power ? "AC" : "DC");
>>> -			return ret;
>>> -		}
>>> -	}
>>> +	adev->pm.ac_power = power_supply_is_system_supplied() > 0;
>>> +	smu_set_ac_dc(smu);
>>
>> In the older logic, driver initialization/resume fails if the message
>> fails. This one doesn't care about the return value. Is there a reason
>> to ignore and continue?
> 
> I think printing an error message in smu_set_ac_dc() is enough,
> and stopping the driver initialization/resume seems a bit excessive.
> 

FW not responding to a message usually means FW is not in a good state 
which could later affect the system anyway. Since there are other FW 
interactions after this probably ignoring this is fine.

BTW, what is the issue seen after resume when power source is not set 
correctly? If that issue creates real problems, then it's worth 
considering keeping the FW informed about the real power source, and 
fail if it doesn't succeed.

Thanks,
Lijo

> Regards,
> Ma Jun
>>
>> Thanks,
>> Lijo
>>>    
>>>    	if ((amdgpu_ip_version(adev, MP1_HWIP, 0) == IP_VERSION(13, 0, 1)) ||
>>>    	    (amdgpu_ip_version(adev, MP1_HWIP, 0) == IP_VERSION(13, 0, 3)))
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
>>> index 2e7f8d5cfc28..8047150fddd4 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
>>> @@ -1442,10 +1442,12 @@ static int smu_v11_0_irq_process(struct amdgpu_device *adev,
>>>    			case 0x3:
>>>    				dev_dbg(adev->dev, "Switched to AC mode!\n");
>>>    				schedule_work(&smu->interrupt_work);
>>> +				adev->pm.ac_power = true;
>>>    				break;
>>>    			case 0x4:
>>>    				dev_dbg(adev->dev, "Switched to DC mode!\n");
>>>    				schedule_work(&smu->interrupt_work);
>>> +				adev->pm.ac_power = false;
>>>    				break;
>>>    			case 0x7:
>>>    				/*
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
>>> index 771a3d457c33..c486182ff275 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
>>> @@ -1379,10 +1379,12 @@ static int smu_v13_0_irq_process(struct amdgpu_device *adev,
>>>    			case 0x3:
>>>    				dev_dbg(adev->dev, "Switched to AC mode!\n");
>>>    				smu_v13_0_ack_ac_dc_interrupt(smu);
>>> +				adev->pm.ac_power = true;
>>>    				break;
>>>    			case 0x4:
>>>    				dev_dbg(adev->dev, "Switched to DC mode!\n");
>>>    				smu_v13_0_ack_ac_dc_interrupt(smu);
>>> +				adev->pm.ac_power = false;
>>>    				break;
>>>    			case 0x7:
>>>    				/*
>>