Add core temperatures for Rockchip RK3588 SoC #1411

segabor · 2024-03-11T07:17:06Z

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 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 · 2024-03-11T07:18:42Z

This PR addresses #1404

BenBE · 2024-03-11T20:37:00Z

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

BenBE · 2024-03-11T20:39:23Z

linux/LibSensors.c

+         /* Map temperature values to Rockchip cores
+          *
+          *   littlecore -> cores 1..4
+          *   bigcore0   -> cores 5,6
+          *   bigcore1   -> cores 7,8
+          */
+	 if (existingCPUs == 8 && String_eq(chip->prefix, "littlecore_thermal")) {
+            data[1] = temp;
+            data[2] = temp;
+            data[3] = temp;
+            data[4] = temp;
+            coreTempCount+=4;
+            continue;
+         }
+         if (existingCPUs == 8 && String_eq(chip->prefix, "bigcore0_thermal")) {
+            data[5] = temp;
+            data[6] = temp;
+            coreTempCount+=2;
+            continue;
+         }
+         if (existingCPUs == 8 && String_eq(chip->prefix, "bigcore1_thermal")) {
+            data[7] = temp;
+            data[8] = temp;
+            coreTempCount+=2;
+            continue;
+         }


A better solution here, instead of hard-coding CPU counts would be to determine the cores associated with each of the groups and assigning the values from there.

Also: There's some indentation issue …

I'll fix the indentation. You're right about the code, it's just a POC. I like @Explorer09 's idea about syncing with the team behind libsensors.

Explorer09 · 2024-03-12T13:48:56Z

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.

BenBE · 2024-03-13T23:41:12Z

@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.

Wer-Wolf · 2024-03-14T02:14:34Z

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.

BenBE · 2024-03-14T08:06:59Z

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 · 2024-03-14T08:11:32Z

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

BenBE · 2024-03-23T19:04:47Z

The latest rebase looks strange …

segabor · 2024-03-23T19:46:40Z

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

BenBE

The mess seems fixed now …

linux/LibSensors.c

BenBE

Minor code style stuff and a small suggestion.

linux/LibSensors.c

isarrider · 2024-08-29T11:24:33Z

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

isarrider · 2024-08-29T13:49:15Z

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

farlongsignal · 2024-09-18T19:41:29Z

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.

BenBE · 2024-09-18T20:25:30Z

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.

This was referenced Mar 11, 2024

cant show cpu temperature on Orangepi 5（with rk3588s） #1160

Closed

Can't display cpu temperature on rock pi 4 #941

Closed

BenBE added the Linux 🐧 Linux related issues label Mar 11, 2024

BenBE linked an issue Mar 11, 2024 that may be closed by this pull request

Turing RK1: missing temperature values #1404

Closed

BenBE reviewed Mar 11, 2024

View reviewed changes

This was linked to issues Mar 11, 2024

cant show cpu temperature on Orangepi 5（with rk3588s） #1160

Closed

Can't display cpu temperature on rock pi 4 #941

Closed

segabor mentioned this pull request Mar 13, 2024

Question: obtain cores to virtual CPUs mapping from libsensors lm-sensors/lm-sensors#485

Open

segabor force-pushed the rk3588_temp branch from a317cf5 to 4746338 Compare March 14, 2024 19:32

segabor force-pushed the rk3588_temp branch from 38ca040 to 03f9005 Compare March 23, 2024 19:45

BenBE requested changes Mar 23, 2024

View reviewed changes

linux/LibSensors.c Outdated Show resolved Hide resolved

segabor force-pushed the rk3588_temp branch from 03f9005 to 38a340a Compare March 24, 2024 04:51

BenBE reviewed Mar 24, 2024

View reviewed changes

linux/LibSensors.c Outdated Show resolved Hide resolved

linux/LibSensors.c Outdated Show resolved Hide resolved

linux/LibSensors.c Outdated Show resolved Hide resolved

linux/LibSensors.c Outdated Show resolved Hide resolved

segabor force-pushed the rk3588_temp branch from d2d4ada to d303994 Compare March 27, 2024 08:27

segabor force-pushed the rk3588_temp branch from d303994 to a0f6114 Compare April 10, 2024 13:12

segabor force-pushed the rk3588_temp branch from a0f6114 to dd3d1e7 Compare May 3, 2024 19:40

segabor force-pushed the rk3588_temp branch from dd3d1e7 to 768405d Compare May 18, 2024 19:42

segabor force-pushed the rk3588_temp branch from 768405d to 7ea51e1 Compare June 16, 2024 05:48

BenBE reviewed Jun 16, 2024

View reviewed changes

linux/LibSensors.c Outdated Show resolved Hide resolved

segabor force-pushed the rk3588_temp branch from 7ea51e1 to 662d800 Compare August 29, 2024 12:36

segabor force-pushed the rk3588_temp branch from 662d800 to bf0ec79 Compare August 29, 2024 12:40

Show core temperatures of Rockchip RK3588 SoC

7bf11d3

BenBE force-pushed the rk3588_temp branch from bf0ec79 to 7bf11d3 Compare August 29, 2024 12:48

BenBE approved these changes Aug 29, 2024

View reviewed changes

BenBE added this to the 3.4.0 milestone Aug 29, 2024

segabor marked this pull request as ready for review August 29, 2024 12:52

BenBE merged commit 6de810f into htop-dev:main Aug 29, 2024
18 checks passed

BenBE mentioned this pull request Aug 30, 2024

Missing temperature on last 2 cores (rk3588) #1530

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add core temperatures for Rockchip RK3588 SoC #1411

Add core temperatures for Rockchip RK3588 SoC #1411

segabor commented Mar 11, 2024 •

edited

Loading

segabor commented Mar 11, 2024

BenBE commented Mar 11, 2024

BenBE Mar 11, 2024

segabor Mar 13, 2024

Explorer09 commented Mar 12, 2024

BenBE commented Mar 13, 2024

Wer-Wolf commented Mar 14, 2024

BenBE commented Mar 14, 2024

BenBE commented Mar 14, 2024

BenBE commented Mar 23, 2024

segabor commented Mar 23, 2024

BenBE left a comment

BenBE left a comment

isarrider commented Aug 29, 2024

isarrider commented Aug 29, 2024

farlongsignal commented Sep 18, 2024

BenBE commented Sep 18, 2024 •

edited

Loading

Add core temperatures for Rockchip RK3588 SoC #1411

Add core temperatures for Rockchip RK3588 SoC #1411

Conversation

segabor commented Mar 11, 2024 • edited Loading

SoC Thermal Drivers

Future Additions

References

segabor commented Mar 11, 2024

BenBE commented Mar 11, 2024

BenBE Mar 11, 2024

Choose a reason for hiding this comment

segabor Mar 13, 2024

Choose a reason for hiding this comment

Explorer09 commented Mar 12, 2024

BenBE commented Mar 13, 2024

Wer-Wolf commented Mar 14, 2024

BenBE commented Mar 14, 2024

BenBE commented Mar 14, 2024

BenBE commented Mar 23, 2024

segabor commented Mar 23, 2024

BenBE left a comment

Choose a reason for hiding this comment

BenBE left a comment

Choose a reason for hiding this comment

isarrider commented Aug 29, 2024

isarrider commented Aug 29, 2024

farlongsignal commented Sep 18, 2024

BenBE commented Sep 18, 2024 • edited Loading

segabor commented Mar 11, 2024 •

edited

Loading

BenBE commented Sep 18, 2024 •

edited

Loading