Add core temperatures for Rockchip RK3588 SoC #1411
Conversation
|
This PR addresses #1404 |
|
I think, instead of hacks on top of other hacks, the overall libsensors-to-cpucore mapping should be addressed properly. |
linux/LibSensors.c
Outdated
| /* 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; | ||
| } |
There was a problem hiding this comment.
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 …
There was a problem hiding this comment.
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.
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). |
|
@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. |
|
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. |
|
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 … |
|
@segabor Please squash your commits in this PR. Cf. the styleguide for details. |
|
The latest rebase looks strange … |
|
Sorry for the mess @BenBE , hope it's fixed now |
| * bigcore0 -> cores 5,6 | ||
| * bigcore1 -> cores 7,8 | ||
| */ | ||
| if (existingCPUs == 8 && String_eq(chip->prefix, "littlecore_thermal")) { |
There was a problem hiding this comment.
If the first condition is checking for 8 CPUs anyway, why not pull this as an outer layer. That way this automatically groups the three blocks belonging to the Rockchip cores as a side effect.
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_thermalandsoc_thermalvalues.SoC Thermal Drivers
sensorsutility bundled withlm-sensorspackage detected the following thermal driversFuture Additions
References