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@xxxxxxx>
---
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@xxxxxxx>
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:
/*
