On 1/18/2024 1:05 PM, Lazar, Lijo wrote:
> On 1/18/2024 7:54 AM, Ma, Jun wrote:
>> Hi Lijo,
>>
>> On 1/17/2024 5:41 PM, Lazar, Lijo wrote:
>>> On 1/17/2024 2:22 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 | 24 +++++++++++--------
>>>> .../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 2 ++
>>>> .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c | 2 ++
>>>> 3 files changed, 18 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..e16d22e30a8a 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"
>>>> @@ -793,6 +794,17 @@ static int
>>>> smu_apply_default_config_table_settings(struct smu_context *smu)
>>>> return smu_set_config_table(smu, &adev->pm.config_table);
>>>> }
>>>>
>>>> +static void smu_update_power_source(struct amdgpu_device *adev)
>>>> +{
>>>> + if (power_supply_is_system_supplied() > 0)
>>>> + adev->pm.ac_power = true;
>>>> + else
>>>> + adev->pm.ac_power = false;
>>>> +
>>>> + if (is_support_sw_smu(adev))
>>>> + smu_set_ac_dc(adev->powerplay.pp_handle);
>>>> +}
>>>> +
>>>> static int smu_late_init(void *handle)
>>>> {
>>>> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>>> @@ -817,16 +829,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;
>>>> - }
>>>> - }
>>>
>>> For this part of the change - driver already updates FW with the initial
>>> detected power state or the last detected power state before going for
>>> suspend. Isn't that good enough?
>>>
>>
>> The power source may change during system suspend, so it's necessary to
>> detect the
>> power source again before system reads the power related data, such as
>> max_power_limit.
>>
>
> Ok. For dGPUs, then refresh of power state after resume will be required
> in GPIO controlled case also. Since FW is reloaded, FW may not detect it
> as a power source change.
>
> Rather than creating another wrapper function, it is simpler to do
> something like
> adev->pm.ac_power = power_supply_is_system_supplied() > 0;
> ret = smu_set_ac_dc(smu);
Ok, will fix this in v2.
Regards,
Ma Jun
>
> Thanks,
> Lijo
>
>> Regards,
>> Ma Jun
>>
>>
>>> Thanks,
>>> Lijo
>>>
>>>> + if (!smu->dc_controlled_by_gpio)
>>>> + smu_update_power_source(adev);
>>>>
>>>> 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:
>>>> /*
>>>
>
