Add configurable gpu thermal zone #89

senyc · 2024-04-22T01:48:10Z

I noticed a bug where the tempFile was the entire path (instead of just the subpath) leading to some issues. So I fixed that and also added an option to set the entire path via the config since I'm not sure if the path will be the same for everyone.

If the path: device/hwmon/hwmon0/temp1_input won't change across machines then the configuration addition probably isn't needed since you can configure the card.

scorpion-26

Thank you for the PR!

This review is a bit more pedantic, since I want the config variables to behave consistent. I hope you understand :)

Some general points:

Can you make the config variable relative to /sys/class/drm/<card>/? (Since there is already a selector for the specific card)
The config variable + documentation is missing from data/config

scorpion-26 · 2024-04-22T21:22:21Z

src/AMDGPU.h

@@ -40,13 +39,20 @@ namespace AMDGPU

    inline uint32_t GetTemperature()
    {
-        if (!RuntimeConfig::Get().hasAMD)
+        if (!RuntimeConfig::Get().hasAMD && Config::Get().gpuThermalZone.empty())


The additional check shouldn't be here (The check is only intended to fail if AMDGPU is disabled and this method is still called, meaning a developer error)

scorpion-26 · 2024-04-22T21:27:10Z

src/Config.cpp

@@ -252,6 +252,7 @@ void Config::Load(const std::string& overrideConfigLocation)
        AddConfigVar("WidgetsRight", config.widgetsRight, lineView, foundProperty);

        AddConfigVar("CPUThermalZone", config.cpuThermalZone, lineView, foundProperty);
+        AddConfigVar("GPUThermalZone", config.gpuThermalZone, lineView, foundProperty);


The name should incorporate that this is only for AMD GPUs, so there is no confusion whether you actually need to set it. I'd suggest "AmdGPUThermalZone"

scorpion-26 · 2024-04-22T21:30:51Z

module.nix

@@ -38,6 +38,11 @@ in {
                    default = "/sys/devices/pci0000:00/0000:00:18.3/hwmon/hwmon2/temp1_input";
                    description = "path to the cpu thermal sensor, probably something in /sys/device";
                };
+                GPUThermalZone = mkOption {
+                    type = types.nullOr types.str;
+                    default = "";


Missing default. Please use the old value("device/hwmon/hwmon1/temp1_input") for backwards compatibility.

scorpion-26 · 2024-04-22T21:31:55Z

module.nix

+                GPUThermalZone = mkOption {
+                    type = types.nullOr types.str;
+                    default = "";
+                    description = "path to the gpu thermal sensor, probably something in /sys/device";


Please also mention here that this is AMD specific and that the path is relative to /sys/class/drm/<card>/

Yeah lol I totally forgot about nvidia

scorpion-26 · 2024-04-22T21:34:15Z

src/AMDGPU.h

@@ -10,8 +10,7 @@ namespace AMDGPU
    static const char* utilizationFile = "/device/gpu_busy_percent";
    static const char* vramTotalFile = "/device/mem_info_vram_total";
    static const char* vramUsedFile = "/device/mem_info_vram_used";
-    // TODO: Make this configurable
-    static const char* tempFile = "/sys/class/drm/card0/device/hwmon/hwmon1/temp1_input";
+    static const char* tempFile = "/device/hwmon/hwmon0/temp1_input";


Not needed when the config variable has a default.

scorpion-26 · 2024-04-22T21:42:34Z

src/AMDGPU.h

+          file.open(Config::Get().gpuThermalZone);
+        } else {
+          file.open(drmCardPrefix + Config::Get().drmAmdCard + tempFile);
+        }


This fallback code is not needed, when the gpuThermalZone has a default.

scorpion-26 · 2024-04-22T21:54:01Z

src/Config.h

@@ -14,6 +14,7 @@ class Config
                                             "VRAM", "GPU",      "RAM",   "CPU",       "Battery", "Power"};

    std::string cpuThermalZone = "";     // idk, no standard way of doing this.
+    std::string gpuThermalZone = "";


Please put the default value (The old one, same as comment in module.nix) here (Also for backwards compatibility)

senyc · 2024-04-22T22:51:43Z

Thank you for the PR!

This review is a bit more pedantic, since I want the config variables to behave consistent. I hope you understand :)

Some general points:

Can you make the config variable relative to /sys/class/drm/<card>/? (Since there is already a selector for the specific card)

The config variable + documentation is missing from data/config

Yeah for sure. I wasn't sure if that was how you wanted it done since it would be a little different than setting a CPU thermal zone (since for that we set a full path instead of a partial). To remove this difference between gpu/cpu's It might be worth changing things to only use full paths instead of setting the drmAmdCard with extra configuration for the sub path.

For example we could have config options for

AmdGpuUtilizationPath
AmdGpuThermalPath
AmdGpuVramPath

Removing the need for setting an explicit DrmAmdCard which is a little more work for the user, but a lot less confusing then adding arbitrary partial paths.

This would make things more inline with how things are set for the CPU. Let me know what you think, otherwise I can move forward with the points above (:

scorpion-26 · 2024-04-23T19:47:12Z

Hmm, good points. I've thought about it a little and I think I still prefer relative paths, though:

Absolute paths would require either breaking old configs by removing DrmAmdCard or basically duplicating functionality in case AmdGpu* are not set. I'm not particularly fond of either of these options.
The 4 file configs (utilization, total vram, current vram and thermal zone) would all need to be present and each manually configured when changing the card index. IMHO this is a bit too much, especially since probably most (if not all) users will never want to change 3 of the 4 files.

senyc · 2024-04-23T21:39:31Z

Yeah sounds good, using relative paths is good for now, I'll get this updated to use them. It might be worth at some point looking into using a dependency or library that handles these queries in the future, or looking at what other bars/widgets do to solve this, not that it is that big of a deal haha.

…dule

scorpion-26 · 2024-04-24T06:46:58Z

Last thing missing now is an entry in data/config with documentation (Can be copy-pasted from module.nix). Other than that, LGTM.

senyc · 2024-04-24T14:01:38Z

data/config

@@ -32,6 +32,13 @@ WidgetsRight: [Tray, Packages, Audio, Bluetooth, Network, Disk, VRAM, GPU, RAM,
 # The CPU sensor to use
 CPUThermalZone: /sys/devices/pci0000:00/0000:00:18.3/hwmon/hwmon2/temp1_input

+# The card to poll when using AMDGPU. If you don't have an AMD card, you can skip this config.
+# Possible values can be found by querying /sys/class/drm
+DrmAmdCard: card0


I moved this option above to make it easier for a new user to understand the AmdGpuThermalZone comment

scorpion-26 · 2024-04-24T15:31:44Z

LGTM, thanks!

senyc added 3 commits April 21, 2024 17:48

fix: correct amd gpu temp file path

4c34744

feat: add gpuThermalZone configuration option

1e8bdcb

fix: correct nix default

f3fb187

scorpion-26 reviewed Apr 22, 2024

View reviewed changes

senyc added 2 commits April 23, 2024 21:08

feat: specify gpu path is for amd cards, and add drmAmdCard to nix mo…

228e4b6

…dule

feat: use relative path for amdGpuThermalZone

8ee2028

senyc force-pushed the master branch from d8dcb77 to 8ee2028 Compare April 24, 2024 01:15

senyc requested a review from scorpion-26 April 24, 2024 01:17

fix: make defaults match their previous values

615c3bf

docs: adding AmdGpuThermalZone to default config with relating comments

6544063

senyc commented Apr 24, 2024

View reviewed changes

scorpion-26 merged commit b3f037d into scorpion-26:master Apr 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configurable gpu thermal zone #89

Add configurable gpu thermal zone #89

senyc commented Apr 22, 2024

scorpion-26 left a comment

scorpion-26 Apr 22, 2024

scorpion-26 Apr 22, 2024

scorpion-26 Apr 22, 2024

scorpion-26 Apr 22, 2024

senyc Apr 22, 2024

scorpion-26 Apr 22, 2024

scorpion-26 Apr 22, 2024

scorpion-26 Apr 22, 2024

senyc commented Apr 22, 2024

scorpion-26 commented Apr 23, 2024

senyc commented Apr 23, 2024

scorpion-26 commented Apr 24, 2024

senyc Apr 24, 2024

scorpion-26 commented Apr 24, 2024

Add configurable gpu thermal zone #89

Add configurable gpu thermal zone #89

Conversation

senyc commented Apr 22, 2024

scorpion-26 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

senyc commented Apr 22, 2024

scorpion-26 commented Apr 23, 2024

senyc commented Apr 23, 2024

scorpion-26 commented Apr 24, 2024

Choose a reason for hiding this comment

scorpion-26 commented Apr 24, 2024