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

[Lazar, Lijo]

On 1/19/2024 7:24 AM, Ma, Jun wrote:
> Hi Lijo,
> 
> On 1/18/2024 5:24 PM, Lazar, Lijo wrote:
>> 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.
>>
> I didn't see any serious problems, the only problem is we may read incorrect
> power related data values because of wrong ac_power value.
> 

Thanks for the clarification. Patch is

	Reviewed-by: Lijo Lazar <lijo.lazar at amd.com>

Thanks,
Lijo

> Regards,
> Ma Jun
>> 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:
>>>>>     				/*
>>>>
>>