clbr / radeontop

GNU General Public License v3.0
810 stars 72 forks source link

Implemented temperature sensor reading #117

Open mmsoft3 opened 3 years ago

mmsoft3 commented 3 years ago

GPU temperature is easily available in the drm interface. In FreeBSD, there is no other tool available to monitor GPU temperature.

clbr commented 3 years ago

What is this: "%.1fgit cloneC GPU Temperature %6.2f%%"

Also, this looks like it'd break Radeon.

clbr commented 3 years ago

The indent on the later part in ticks.c is wrong.

clbr commented 3 years ago

Looks like it'd also break amdgpu when on older DRM.

clbr commented 3 years ago

Does the ui.c part even compile? temp is declared before temp_c but it uses temp_c...

clbr commented 3 years ago

Dump part is missing.

Altogether, this needs a lot of work.

mmsoft3 commented 3 years ago

Thanks for your comments. I updated the radeon part, however I cannot test it since I do not have such a board. Also updated the dump part and fixed some typos

clbr commented 3 years ago

Thanks. Please squash the commits into one.

mmsoft3 commented 3 years ago

Looks like it'd also break amdgpu when on older DRM.

How do you think it will break? Temp sensor data has been exposed at the same time as memory and shader clock data that is already used by radeontop

clbr commented 3 years ago

In that case the code was calling a function pointer of random garbage. There was no setting it to NULL and checking for NULL at that point.

mmsoft3 commented 3 years ago

well, this is a common problem also with the other sensors data as well. e.g.: In case, line 80: (!(ret = getsclk_amdgpu(&out32))) gives false for some reason, the function pointer is also uninitialized garbage.

clbr commented 3 years ago

No, it's not; that is initialized in detect.c.

mmsoft3 commented 3 years ago

do you want to have them all similar or should the NULL check remain there?

clbr commented 3 years ago

Existing style would be best.

mmsoft3 commented 3 years ago

as discussed, unified the initialization of function pointers and squashed all commits into one.

Would appreciate to see it in upcoming releases.

clbr commented 3 years ago

Ok, I tested it on radeon. The values are all wrong, and the indent in dump.c is still wrong. The NULL check in dump.c is still present. Also please remove the utf-8 degree symbol in the comment in detect.c.

The values are wrong because you don't take into account k. You must multiply by that, and use a correct formula otherwise.

Have you even tested this? First the non-compiling ui.c, and now the wrong values. The values will be wrong on amdgpu as well, not just radeon, since the formula is the same for both!

mmsoft3 commented 3 years ago

Of course I tested and it’s working not only for me with correct values as you can see here: https://forums.freebsd.org/threads/amdgpu-is-there-a-way-to-read-amd-gpus-temperatures-and-or-power-usage.76245/#post-520161

That’s why I said I can only test for amdgpu, not radeon. So maybe the formula for radeon needs to be different from amdgpu.

What values do you get from a radeon board?

clbr commented 3 years ago

Try changing the tick rate. -t 50 for example.

mmaravillo commented 1 year ago

Temperature reading does not match with that of hardinfo.

image

octal-illumination commented 1 year ago

Any update on this? Would love to see my GPU temperature with radeontop.

wgottwalt commented 6 months ago

Why putting this in radeontop while all the correct radeon temperatures, power usage and so on are available via the hwmon subsystem? I'm pretty sure hwmon and the userspace tool lm_sensors is available on freebsd.