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 <[email protected]> >>>> --- >>>> 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.
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: >>>> /* >>> >
