feature: support for multiple temperature sensors #282
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…same ValueID type Changed ValueID from a simple enum to a class that contains two components: the ValueID (as before) and an instance id. This allows to store and address multiple instances of the same value type in gpu::gpuData using unique keys. This commit only adds the new class and updates syntax for some uses of ValueID, without changing any program logic. Overall the new ValueID class should be a drop in for the old enum if only one instance of a ValueID type is needed. Some explicit static_cast's to ints and type conversion to ints in settings.cpp and dialogs/dialog_deinetopbaritem.cpp needed to be updated though. This is prep work for introducing support for multiple temperature sensors when available by hwmon. Sensors will be distinguished by each having a unique ValueID::instance value per sensor for each of the TEMPERATURE_* valueID types.
When temperature sensors via the HWMON backend are supported, detect all available temperature sensors. If the labels provided by hwmon are known, we assign known instance ids to the ValueIDs of the sensor data. As of now, this includes edge, junction and mem temperature, which will be assigned the intance of T_EDGE, T_JUNCTION and T_MEM for all TEMPERATURE_* values. For those instances, getNameOfValueID() now returns the appropriate localized strings for UI labels. For all sensors detected with unknown labels, a free instance id is assigned, the label provided by HWMON is used to construct labels for the UI, as returned by getNameOfValueID().
makes data list aware of potential multiple instances of TEMPERATURE_CURRENT values from multiple temperature sensors. Readings from all sensors will now be listed properly.
makes radeon_profile::resetMinMax() aware of multiple temperature sensors. When the user requests a reset of the min/max values in the settings tab, now the min/max values for all temperature sensors will be reset.
Make topbar items aware of multiple instances of TEMPERATURE_CURRENT values, so user can display sensors readings from any sensor.
When a default topbar schema is created, make sure the main large label for temperature is using the T_EDGE sensor. Also conditionally generate a label pair for T_JUNCTION and T_MEM sensors, when available by the driver.
add a third column, display crit and emergency constants for temp sensors, if available.
allow user to select the temperature sensor to monitor for each TEMPERATURE type event.
Hysteresis was a global setting, applied independently of fan profile. This is somewhat confusing, as one could expect it to be an aspect of a fan profile, since one sets it in the fan profile tab. This adds a new FanProfile struct that can store additional settings besides the temperature steps and adds hysteresis as one of those settings. The global hysteresis value stored in older radeon-profile-settings will still be read and applied to all loaded profiles. Once new config files are written, this values is moved to the xml file and stored as attributes of each fan profile respectively.
Adds a new combo box to the fan profile tab to allow the user to select a temperature sensor as data source for this fan curve. radeon_profile::adjustFanSpeed() will now read the current temperature of the temperature sensor selected in the currently active fan profile. This setting is a per fan profile setting. It is stored in the newly added FanProfile struct and written to and read from the xml config file for each fan profile. By default, the edge temperature sensor is selected. This also applies when an older config file is read without the new config attribute.
|
This has been merged on my fork, with the other commits found under merge-e-world/master. I added some signals/slots to flag modifications in the Fan Control tab and ask to save the modification. That being said, the configuration's saving and restoring processes will need to be modified to properly represent the information and deal with multi-GPU setup. But this is another topic I will work on soon. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Newer radeon graphics cards since vega20 – including AFAIK all navi based cards – expose three temperature sensors via the hwmon: Edge temperature (the "classic" GPU temp), junction temperature and memory temperature. radeon-profile so far only read the first temperature sensor (edge temp).
This set of changes enables detecting all available temperature sensors when hwmon is available. Including any other sensors not listed above - should there ever be any. This data is then made available to the user throughout the UI in the following places:
(On top of that, I also changed the hysteresis setting in the fan control tab to a per profile setting rather than a global one, since that seemed more consistent).
Any card not supporting more than one temperature sensor should still work as it had before. It was so far only tested on a Radeon VII (vega20) though.
For easier review, each of those features is split into its own commit. The most intrusive change is the replacement of the ValueID enum with a struct containing two components in the first commit: the type enum, and a new instance id. This allows to use multiple values of the same ValueID type with each their own unique key in gpu::gpuData. I considered just adding more values to the ValueID enum for junction/mem temperature and their min/max/before values respectively, but this would not allow dynamically creating an arbitrary number of sensors. It would also require to blow up a lot of switch statements with a lot of new cases for each new sensor. Using a key with two components seemed more flexible for future use to me.