Add support for AMD CPU temp reading using k10temp

[theizzer]

I've noticed while using this extension that it was not showing temperature reading for me. So I decided to investigate and came up with this.

This adds support for temperature reading for AMD processors using 'k10temp' kernel driver. It only shows one (Tctl) or two temps (Tctl & Tdie - for newer CPUs that support it) for the whole CPU so I wrote it that it shows the more accurate reading (Tdie) with Tctl reading as a fallback.

Now I am happy :)

[fflewddur]

Thanks for this contribution! Does AMD's temperature monitor not include the "Package id" line, or some other indicator of the physical socket for the CPU? TopHat will show separate temperatures for each physical CPU, so hard coding that to 0 wouldn't work on multi-CPU systems. Otherwise, this patch looks great.

[theizzer]

I've read the k10temp documentation and I could dig up more information for some CPUs. Not physical cores temps, just temperatures for CCDs, if the CPU has it.

I'll look at it. But yeah, there will be only one "sensor" named "k10temp".

[theizzer]

Okay, so I could prepare it for showing CCD temps even, but I would have no way to test it. For me it's only one temp reading -> Tctl.

[theizzer]

Okay, nevermind - it would require a bit more refactoring to show different temps, since you are using /proc/cpuinfo to parse number of CPUs, which does not work for AMD.

physical id is always 0 for all the cores/threads and that is fine I guess, since there is only one temp reading.

All good for AMD then! :))

EDIT: After some digging in the code I've noticed that you are not talking about per-core temp, but rather per-cpu-socket temperatures. In the /proc/cpuinfo you are looking at physical id and not core id. My bad! :) That said I cannot find how the k10temp driver behaves with more CPU sockets. The documentation only talks about CCD temps.

[fflewddur]

Sorry, I should have clarified: the physical id is for the number of physical processors, not the number of cores. It's pretty uncommon these days, but there are motherboards that support more than one CPU; in these cases, you'll see a physical id of 0 for the first processor, 1 for the second processor, etc. in /proc/cpuinfo, and at least for Intel chips, a corresponding Package id in the hwmon file. That's the id we parse on line 216 (in retrospect, I should rename that variable to make it clear that it refers to the physical processor).

TopHat's menu will show separate lines for each physical CPU, since they can each have different frequencies and temperatures. That's why cpuTempMonitors is a map--the key is the physical ID/package ID for the processor, and the value is the file for reading that processor's temperature. If there is a physical id, package id, or similar line in the k10temp information (likely in a file like temp[n]_label or similar), that's what we should use for the key in the cpuTempMonitors map instead of 0. If you're not sure, could you paste the output of grep -r . /sys/class/hwmon/hwmon*/temp* here?

I completely agree that we should only show one temp for the entire CPU, rather than per-core temps. Thanks for your work on this! 👍

[theizzer]

That's the thing. k10temp uses temp1_input for Tctl and temp2_input for Tdie (if available - just more precise measurement). Additional temp(3-10)_input is used for CCD temps (inside one CPU).

On Family 17h and Family 18h CPUs, additional temperature sensors may report Core Complex Die (CCD) temperatures. Up to 8 such temperatures are reported as temp{3..10}_input, labeled Tccd{1..8}. Actual support depends on the CPU variant. (from https://docs.kernel.org/hwmon/k10temp.html)

This is mine output of grep -r . /sys/class/hwmon/hwmon*/temp*

/sys/class/hwmon/hwmon1/temp1_crit:120000
/sys/class/hwmon/hwmon1/temp1_input:52000
/sys/class/hwmon/hwmon3/temp1_alarm:0
/sys/class/hwmon/hwmon3/temp1_crit:79850
/sys/class/hwmon/hwmon3/temp1_input:32850
/sys/class/hwmon/hwmon3/temp1_label:Composite
/sys/class/hwmon/hwmon3/temp1_max:76850
/sys/class/hwmon/hwmon3/temp1_min:-150
/sys/class/hwmon/hwmon4/temp1_input:40000
/sys/class/hwmon/hwmon4/temp1_label:edge
/sys/class/hwmon/hwmon5/temp1_input:51875
/sys/class/hwmon/hwmon5/temp1_label:Tctl

I don't even have Tdie on my particular CPU. (temp2_input). Not sure how the output would look like on PC with two AMD CPUs -> I would guess there will be hwmon6, but the name of it would still be "k10temp" and temp1_label would be Tctl too for the other CPU :thinking: so actually no idea.

And hwmon4 is iGPU, which is another thing we could add to this extension - GPU temps :)

EDIT: I've looked at another extension that shows temps too.. their code is Intel only too I guess :grimacing: not coretemp --> not a CPU :rofl:

// are we dealing with a CPU?
if (name == 'coretemp') {
    // determine which processor (socket) we are dealing with
    new FileModule.File(hwbase + file + '/temp1_label').read().then(prefix => {
        this._processTempVoltFan(callback, sensor_types, prefix, hwbase + file, file);
    }).catch(err => {
        // this shouldn't be necessary, but just in case temp1_label doesn't exist
        // attempt to fix #266
        this._processTempVoltFan(callback, sensor_types, name, hwbase + file, file);
    });
} else {
    // not a CPU, process all other sensors
    this._processTempVoltFan(callback, sensor_types, name, hwbase + file, file);
}

[fflewddur]

Cool, thanks for helping me work though this! Merging it now :)