Re: [PATCH] drm/amd/pm: fix amdgpu_pm_info power display units

[Lazar, Lijo]

On 18-Jun-26 10:51 AM, Yang Wang wrote:
amdgpu_pm_info displayed power sensor readings with the wrong fractional unit.
It treated the low byte of the raw sensor value as the decimal part of watts,
while that field represents milliwatts in the decoded value. As a result,
debugfs could report misleading SoC power when the remainder was not already
a two-digit centiwatt value.

Example with query = 0x00000354:

   raw field        value
   ---------------------
   query >> 8       3 W
   query & 0xff     84 mW
   decoded power    3084 mW

   output           value
   ---------------------
   before           3.84 W
   after            3.08 W

Fixes: f0b8f65b4825 ("drm/amd/amdgpu: fix the GPU power print error in pm info")

Signed-off-by: Yang Wang <kevinyang.w...@amd.com>
---
  drivers/gpu/drm/amd/pm/amdgpu_pm.c | 21 ++++++++++++---------
  1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c 
b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
index f69bb77ecbfe..719b743adc37 100644
--- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
@@ -41,6 +41,8 @@
  
  #define DEVICE_ATTR_IS(_name)		(attr_id == device_attr_id__##_name)
  
+#define power_2_mwatt(power)	(((power) >> 8) * 1000 + ((power) & 0xff))
+
  struct od_attribute {
        struct kobj_attribute   attribute;
        struct list_head        entry;
@@ -3354,7 +3356,6 @@ static int amdgpu_hwmon_get_power(struct device *dev,
                                  enum amd_pp_sensors sensor)
  {
        struct amdgpu_device *adev = dev_get_drvdata(dev);
-       unsigned int uw;
        u32 query = 0;
        int r;
  
@@ -3363,9 +3364,7 @@ static int amdgpu_hwmon_get_power(struct device *dev,
                return r;
  
  	/* convert to microwatts */
-       uw = (query >> 8) * 1000000 + (query & 0xff) * 1000;
-
-       return uw;
+       return power_2_mwatt(query) * 1000;
  }
  
  static ssize_t amdgpu_hwmon_show_power_avg(struct device *dev,
@@ -4908,7 +4907,7 @@ static int amdgpu_debugfs_pm_info_pp(struct seq_file *m, 
struct amdgpu_device *a
  {
        uint32_t mp1_ver = amdgpu_ip_version(adev, MP1_HWIP, 0);
        uint32_t gc_ver = amdgpu_ip_version(adev, GC_HWIP, 0);
-       uint32_t value;
+       uint32_t value, mwatt, centiwatt;
        uint64_t value64 = 0;
        uint32_t query = 0;
        int size;
@@ -4933,17 +4932,21 @@ static int amdgpu_debugfs_pm_info_pp(struct seq_file 
*m, struct amdgpu_device *a
                seq_printf(m, "\t%u mV (VDDNB)\n", value);
        size = sizeof(uint32_t);
        if (!amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_GPU_AVG_POWER, (void 
*)&query, &size)) {
+               mwatt = power_2_mwatt(query);
+               centiwatt = DIV_ROUND_CLOSEST(mwatt, 10);

Instead of doing multiply and then divide twice, isn't it just better to 
wrap it under two macros and use?

power_w(value) / fractional_mw(value)

Here, it would then be fractional_mw(val)/10.

Thanks,
Lijo

                if (adev->flags & AMD_IS_APU)
-                       seq_printf(m, "\t%u.%02u W (average SoC including CPU)\n", query 
>> 8, query & 0xff);
+                       seq_printf(m, "\t%u.%02u W (average SoC including 
CPU)\n", centiwatt / 100, centiwatt % 100);
                else
-                       seq_printf(m, "\t%u.%02u W (average SoC)\n", query >> 8, 
query & 0xff);
+                       seq_printf(m, "\t%u.%02u W (average SoC)\n", centiwatt 
/ 100, centiwatt % 100);
        }
        size = sizeof(uint32_t);
        if (!amdgpu_dpm_read_sensor(adev, AMDGPU_PP_SENSOR_GPU_INPUT_POWER, (void 
*)&query, &size)) {
+               mwatt = power_2_mwatt(query);
+               centiwatt = DIV_ROUND_CLOSEST(mwatt, 10);
                if (adev->flags & AMD_IS_APU)
-                       seq_printf(m, "\t%u.%02u W (current SoC including CPU)\n", query 
>> 8, query & 0xff);
+                       seq_printf(m, "\t%u.%02u W (current SoC including 
CPU)\n", centiwatt / 100, centiwatt % 100);
                else
-                       seq_printf(m, "\t%u.%02u W (current SoC)\n", query >> 8, 
query & 0xff);
+                       seq_printf(m, "\t%u.%02u W (current SoC)\n", centiwatt 
/ 100, centiwatt % 100);
        }
        size = sizeof(value);
        seq_printf(m, "\n");