htop icon indicating copy to clipboard operation
htop copied to clipboard

Add core temperatures for Rockchip RK3588 SoC

Open segabor opened this issue 1 year ago • 9 comments

My attempt to map SoC core temperatures to virtual cores. Rather a hack than a well reasoned change. There must be a proper way to determine chipset so temperature mapping can be done if condition is met (a refactor will be needed). Also, data[0] is not set. We have two candidates here, center_thermal and soc_thermal values.

soc_temp_values_under_load

SoC Thermal Drivers

sensors utility bundled with lm-sensors package detected the following thermal drivers

npu_thermal-virtual-0
Adapter: Virtual device
temp1:        +35.2 C  

center_thermal-virtual-0
Adapter: Virtual device
temp1:        +35.2 C  

bigcore1_thermal-virtual-0
Adapter: Virtual device
temp1:        +36.1 C  

soc_thermal-virtual-0
Adapter: Virtual device
temp1:        +36.1 C  (crit = +115.0 C)

nvme-pci-0100
Adapter: PCI adapter
Composite:    +41.9 C  (low  =  -0.1 C, high = +71.8 C)
                       (crit = +89.8 C)

gpu_thermal-virtual-0
Adapter: Virtual device
temp1:        +35.2 C  

littlecore_thermal-virtual-0
Adapter: Virtual device
temp1:        +36.1 C  

bigcore0_thermal-virtual-0
Adapter: Virtual device
temp1:        +36.1 C  

Future Additions

  • [ ] NPU temperature
  • [ ] GPU temperature

References

segabor avatar Mar 11 '24 07:03 segabor

This PR addresses #1404

segabor avatar Mar 11 '24 07:03 segabor

I think, instead of hacks on top of other hacks, the overall libsensors-to-cpucore mapping should be addressed properly.

BenBE avatar Mar 11 '24 20:03 BenBE

I think, instead of hacks on top of other hacks, the overall libsensors-to-cpucore mapping should be addressed properly.

I believe the libsensors-to-cpu-core mapping shouldn't be handled in htop's side. Perhaps we can ask this question in libsensors upstream to see if there is a portable way to describe the mappings.

My impression is that libsensors.conf is supposed to do the job. Or, maybe, ACPI thermal zone (even though I personally hate ACPI). Once we have a "protocol" to read the mapping from, we would no longer need these chipset-specific hacks.

Explorer09 avatar Mar 12 '24 13:03 Explorer09

@Explorer09 Mind to open a discussion with the libsensors people and link it here for reference? It's not just this bug, but quite a few more, that basically boil down to how to perform this mapping. TIA.

BenBE avatar Mar 13 '24 23:03 BenBE

Maybe it would be better to create some sort of naming convention for the temperature channels used for the cpu cores.

I am strongly against adding driver-specific code to libsensors, it was done once and proved to be an maintainance nightmare.

With a proper naming convention in place, sensors.conf can be used to do the necessary adjustments.

Wer-Wolf avatar Mar 14 '24 02:03 Wer-Wolf

We'd also strongly prefer not to deal with any of the driver-specific stuff in htop. And honestly, there are two places things could go: Either into sysfs alongside each sensor providing the core/thread information we need (can figure the rest from procfs -> cpuinfo). Or we get this information from the library we use for fetching the temperature information.

Both have their drawbacks, but the alternative would be, that each user of lmsensors would have to write their own code to figure this out instead, which is effectively worse …

BenBE avatar Mar 14 '24 08:03 BenBE

@segabor Please squash your commits in this PR. Cf. the styleguide for details.

BenBE avatar Mar 14 '24 08:03 BenBE

The latest rebase looks strange …

BenBE avatar Mar 23 '24 19:03 BenBE

Sorry for the mess @BenBE , hope it's fixed now

segabor avatar Mar 23 '24 19:03 segabor

@segabor highly appreacted as I have many rk3588...

isarrider avatar Aug 29 '24 11:08 isarrider

wohoo - so happy that my thank you triggered a merge ;)

isarrider avatar Aug 29 '24 13:08 isarrider

thank you for this, i easily adapted this for Qualcomm SDM845 (Oneplus 6 enchilada running PostmarketOS with mainline kernel), it reports temperatures in similar way. Although i could imagine this getting really redundant if you wanted to support all the other possible Qualcomm chips. Here in case someone wants it.

farlongsignal avatar Sep 18 '24 19:09 farlongsignal

thank you for this, i easily adapted this for Qualcomm SDM845 (Oneplus 6 enchilada running PostmarketOS with mainline kernel), it reports temperatures in similar way. Although i could imagine this getting really redundant if you wanted to support all the other possible Qualcomm chips. Here in case someone wants it.

Could you please link the exact commit for future reference? That way you allow people even when your repo at some point contains more commits to easily find the exact change you are referring to in your comment. TIA.

Also looking at your commit I notice a few improvements that could/should be made:

  • Use sscanf in the code assigning the actual values to avoid redundancy. Alternatively you could String_startsWith(str, "cpu") && String_eq(str + 4, "_thermal") and pull the actual index (with range check) from str[3]-'0'+1 if it's a valid digit.
  • Proper alignment in the list of possible drivers at the top.

BenBE avatar Sep 18 '24 20:09 BenBE