From 93e153079dd2b16d7e067fe513d10cc3bd123455 Mon Sep 17 00:00:00 2001 From: Jeremy Whiting Date: Tue, 18 Jun 2024 14:52:56 -0600 Subject: [PATCH] Rework the CPUGovernors enum a bit. Change to CPUScalingGovernors and use strum crate to remove some cruft. --- Cargo.lock | 29 +++++ Cargo.toml | 1 + com.steampowered.SteamOSManager1.xml | 6 +- src/bin/steamosctl.rs | 38 ++++--- src/manager/root.rs | 12 +-- src/manager/user.rs | 29 +++-- src/power.rs | 151 ++++++++------------------- src/proxy.rs | 10 +- 8 files changed, 126 insertions(+), 150 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3192a27..9c84373 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -920,6 +920,12 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "rustversion" +version = "1.0.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "955d28af4278de8121b7ebeb796b6a45735dc01436d898801014aced2773a3d6" + [[package]] name = "serde" version = "1.0.201" @@ -1027,6 +1033,7 @@ dependencies = [ "libc", "nix", "serde", + "strum", "tempfile", "tokio", "tokio-stream", @@ -1040,6 +1047,28 @@ dependencies = [ "zbus_xml", ] +[[package]] +name = "strum" +version = "0.26.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5d8cec3501a5194c432b2b7976db6b7d10ec95c253208b45f83f7136aa985e29" +dependencies = [ + "strum_macros", +] + +[[package]] +name = "strum_macros" +version = "0.26.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4c6bee85a5a24955dc440386795aa378cd9cf82acd5f764469152d2270e581be" +dependencies = [ + "heck", + "proc-macro2", + "quote", + "rustversion", + "syn 2.0.63", +] + [[package]] name = "syn" version = "1.0.109" diff --git a/Cargo.toml b/Cargo.toml index 42b5154..efe83af 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,3 +29,4 @@ tracing-subscriber = { version = "0.3", default-features = false, features = ["e udev = "0.8" xdg = "2.5" zbus = { version = "4", default-features = false, features = ["tokio"] } +strum = { version = "0.26", features = ["derive"] } diff --git a/com.steampowered.SteamOSManager1.xml b/com.steampowered.SteamOSManager1.xml index 09391a3..1910750 100644 --- a/com.steampowered.SteamOSManager1.xml +++ b/com.steampowered.SteamOSManager1.xml @@ -312,10 +312,10 @@ Enumerate the supported cpu governors on the system. - A list of supported governors (a dictionary of values to names) + A list of supported governor names Version available: 9 --> - + - + diff --git a/src/bin/steamosctl.rs b/src/bin/steamosctl.rs index 3236f1d..a8e3438 100644 --- a/src/bin/steamosctl.rs +++ b/src/bin/steamosctl.rs @@ -11,7 +11,7 @@ use itertools::Itertools; use std::ops::Deref; use steamos_manager::cec::HdmiCecState; use steamos_manager::hardware::FanControlState; -use steamos_manager::power::{CPUGovernor, GPUPerformanceLevel, GPUPowerProfile}; +use steamos_manager::power::{CPUScalingGovernor, GPUPerformanceLevel, GPUPowerProfile}; use steamos_manager::proxy::ManagerProxy; use steamos_manager::wifi::{WifiBackend, WifiDebugMode, WifiPowerManagement}; use zbus::fdo::PropertiesProxy; @@ -45,16 +45,16 @@ enum Commands { /// Get the fan control state GetFanControlState, - /// Get the CPU governors supported on this device - GetCpuGovernors, + /// Get the available CPU scaling governors supported on this device + GetAvailableCpuScalingGovernors, /// Get the current CPU governor - GetCpuGovernor, + GetCpuScalingGovernor, - /// Set the current CPU governor - SetCpuGovernor { + /// Set the current CPU Scaling governor + SetCpuScalingGovernor { /// Valid governors are get-cpu-governors. - governor: CPUGovernor, + governor: CPUScalingGovernor, }, /// Get the GPU power profiles supported on this device @@ -215,29 +215,27 @@ async fn main() -> Result<()> { Err(_) => println!("Got unknown value {state} from backend"), } } - Commands::GetCpuGovernors => { - let governors = proxy.cpu_governors().await?; + Commands::GetAvailableCpuScalingGovernors => { + let governors = proxy.available_cpu_scaling_governors().await?; println!("Governors:\n"); - for key in governors.keys().sorted() { - let name = &governors[key]; - println!("{key}: {name}"); + for name in governors { + println!("{name}"); } } - Commands::GetCpuGovernor => { - let governor = proxy.cpu_governor().await?; - let governor_type = CPUGovernor::try_from(governor); + Commands::GetCpuScalingGovernor => { + let governor = proxy.cpu_scaling_governor().await?; + let governor_type = CPUScalingGovernor::try_from(governor.as_str()); match governor_type { - Ok(t) => { - let name = t.to_string(); - println!("CPU Governor: {governor} {name}"); + Ok(_) => { + println!("CPU Governor: {governor}"); } Err(_) => { println!("Unknown CPU governor or unable to get type from {governor}"); } } } - Commands::SetCpuGovernor { governor } => { - proxy.set_cpu_governor(*governor as u32).await?; + Commands::SetCpuScalingGovernor { governor } => { + proxy.set_cpu_scaling_governor(governor.to_string()).await?; } Commands::GetGPUPowerProfiles => { let profiles = proxy.gpu_power_profiles().await?; diff --git a/src/manager/root.rs b/src/manager/root.rs index cc99190..6c16346 100644 --- a/src/manager/root.rs +++ b/src/manager/root.rs @@ -20,8 +20,8 @@ use crate::daemon::DaemonCommand; use crate::error::{to_zbus_error, to_zbus_fdo_error}; use crate::hardware::{variant, FanControl, FanControlState, HardwareVariant}; use crate::power::{ - set_cpu_governor, set_gpu_clocks, set_gpu_performance_level, set_gpu_power_profile, - set_tdp_limit, CPUGovernor, GPUPerformanceLevel, GPUPowerProfile, + set_cpu_scaling_governor, set_gpu_clocks, set_gpu_performance_level, set_gpu_power_profile, + set_tdp_limit, CPUScalingGovernor, GPUPerformanceLevel, GPUPowerProfile, }; use crate::process::{run_script, script_output, ProcessManager}; use crate::wifi::{ @@ -191,11 +191,11 @@ impl SteamOSManager { .map_err(to_zbus_fdo_error) } - async fn set_cpu_governor(&self, governor: u32) -> fdo::Result<()> { - let governor = CPUGovernor::try_from(governor).map_err(to_zbus_fdo_error)?; - set_cpu_governor(governor) + async fn set_cpu_scaling_governor(&self, governor: String) -> fdo::Result<()> { + let g = CPUScalingGovernor::try_from(governor.as_str()).map_err(to_zbus_fdo_error)?; + set_cpu_scaling_governor(g) .await - .inspect_err(|message| error!("Error setting CPU governor: {message}")) + .inspect_err(|message| error!("Error setting CPU scaling governor: {message}")) .map_err(to_zbus_fdo_error) } diff --git a/src/manager/user.rs b/src/manager/user.rs index 1fd7863..c4b3cf9 100644 --- a/src/manager/user.rs +++ b/src/manager/user.rs @@ -20,8 +20,8 @@ use crate::daemon::DaemonCommand; use crate::error::{to_zbus_error, to_zbus_fdo_error, zbus_to_zbus_fdo}; use crate::hardware::check_support; use crate::power::{ - get_cpu_governor, get_cpu_governors, get_gpu_clocks, get_gpu_performance_level, - get_gpu_power_profile, get_gpu_power_profiles, get_tdp_limit, + get_available_cpu_scaling_governors, get_cpu_scaling_governor, get_gpu_clocks, + get_gpu_performance_level, get_gpu_power_profile, get_gpu_power_profiles, get_tdp_limit, }; use crate::wifi::{get_wifi_backend, get_wifi_power_management_state}; use crate::API_VERSION; @@ -192,18 +192,31 @@ impl SteamOSManager { } #[zbus(property(emits_changed_signal = "false"))] - async fn cpu_governors(&self) -> fdo::Result> { - get_cpu_governors().await.map_err(to_zbus_fdo_error) + async fn available_cpu_scaling_governors(&self) -> fdo::Result> { + let governors = get_available_cpu_scaling_governors() + .await + .map_err(to_zbus_fdo_error)?; + let mut result = Vec::new(); + for g in governors { + result.push(g.to_string()); + } + Ok(result) } #[zbus(property(emits_changed_signal = "false"))] - async fn cpu_governor(&self) -> fdo::Result { - get_cpu_governor().await.map_err(to_zbus_fdo_error) + async fn cpu_scaling_governor(&self) -> fdo::Result { + let governor = get_cpu_scaling_governor() + .await + .map_err(to_zbus_fdo_error)?; + Ok(governor.to_string()) } #[zbus(property)] - async fn set_cpu_governor(&self, governor: u32) -> zbus::Result<()> { - setter!(self, "SetCpuGovernor", &(governor)) + async fn set_cpu_scaling_governor(&self, governor: String) -> zbus::Result<()> { + self.proxy + .call("SetCpuScalingGovernor", &(governor)) + .await + .map_err(to_zbus_error) } #[zbus(property(emits_changed_signal = "false"))] diff --git a/src/power.rs b/src/power.rs index 598fbd5..f8c2abe 100644 --- a/src/power.rs +++ b/src/power.rs @@ -10,9 +10,10 @@ use std::collections::HashMap; use std::fmt; use std::path::PathBuf; use std::str::FromStr; +use strum::{Display, EnumString}; use tokio::fs::{self, File}; use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; -use tracing::error; +use tracing::{error, warn}; use crate::hardware::is_deck; use crate::{path, write_synced}; @@ -21,7 +22,7 @@ const GPU_HWMON_PREFIX: &str = "/sys/class/hwmon"; const GPU_HWMON_NAME: &str = "amdgpu"; const GPU_DRM_PREFIX: &str = "/sys/class/drm"; const GPU_VENDOR: &str = "0x1002"; -const CPU_GOVERNOR_PREFIX: &str = "/sys/devices/system/cpu/cpufreq"; +const CPU_PREFIX: &str = "/sys/devices/system/cpu/cpufreq"; const CPU0_NAME: &str = "policy0"; const CPU_POLICY_NAME: &str = "policy"; @@ -29,8 +30,8 @@ const CPU_POLICY_NAME: &str = "policy"; const GPU_POWER_PROFILE_SUFFIX: &str = "device/pp_power_profile_mode"; const GPU_PERFORMANCE_LEVEL_SUFFIX: &str = "device/power_dpm_force_performance_level"; const GPU_CLOCKS_SUFFIX: &str = "device/pp_od_clk_voltage"; -const CPU_GOVERNOR_SUFFIX: &str = "scaling_governor"; -const CPU_GOVERNORS_SUFFIX: &str = "scaling_available_governors"; +const CPU_SCALING_GOVERNOR_SUFFIX: &str = "scaling_governor"; +const CPU_SCALING_AVAILABLE_GOVERNORS_SUFFIX: &str = "scaling_available_governors"; const TDP_LIMIT1: &str = "power1_cap"; const TDP_LIMIT2: &str = "power2_cap"; @@ -149,9 +150,9 @@ impl fmt::Display for GPUPerformanceLevel { } } -#[derive(Hash, Eq, PartialEq, Debug, Copy, Clone)] -#[repr(u32)] -pub enum CPUGovernor { +#[derive(Display, EnumString, Hash, Eq, PartialEq, Debug, Copy, Clone)] +#[strum(serialize_all = "lowercase")] +pub enum CPUScalingGovernor { Conservative, OnDemand, UserSpace, @@ -160,49 +161,6 @@ pub enum CPUGovernor { SchedUtil, } -impl TryFrom for CPUGovernor { - type Error = &'static str; - fn try_from(v: u32) -> Result { - match v { - x if x == CPUGovernor::Conservative as u32 => Ok(CPUGovernor::Conservative), - x if x == CPUGovernor::OnDemand as u32 => Ok(CPUGovernor::OnDemand), - x if x == CPUGovernor::UserSpace as u32 => Ok(CPUGovernor::UserSpace), - x if x == CPUGovernor::PowerSave as u32 => Ok(CPUGovernor::PowerSave), - x if x == CPUGovernor::Performance as u32 => Ok(CPUGovernor::Performance), - x if x == CPUGovernor::SchedUtil as u32 => Ok(CPUGovernor::SchedUtil), - _ => Err("No enum match for value {v}"), - } - } -} - -impl FromStr for CPUGovernor { - type Err = Error; - fn from_str(input: &str) -> Result { - Ok(match input { - "conservative" => CPUGovernor::Conservative, - "ondemand" => CPUGovernor::OnDemand, - "userspace" => CPUGovernor::UserSpace, - "powersave" => CPUGovernor::PowerSave, - "performance" => CPUGovernor::Performance, - "schedutil" => CPUGovernor::SchedUtil, - v => bail!("No enum match for value {v}"), - }) - } -} - -impl fmt::Display for CPUGovernor { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match self { - CPUGovernor::Conservative => write!(f, "conservative"), - CPUGovernor::OnDemand => write!(f, "ondemand"), - CPUGovernor::UserSpace => write!(f, "userspace"), - CPUGovernor::PowerSave => write!(f, "powersave"), - CPUGovernor::Performance => write!(f, "performance"), - CPUGovernor::SchedUtil => write!(f, "schedutil"), - } - } -} - async fn read_gpu_sysfs_contents() -> Result { // check which profile is current and return if possible let base = find_gpu_prefix().await?; @@ -212,24 +170,24 @@ async fn read_gpu_sysfs_contents() -> Result { } async fn read_cpu_governor_sysfs_available_contents() -> Result { - let base = path(CPU_GOVERNOR_PREFIX); - fs::read_to_string(base.join(CPU0_NAME).join(CPU_GOVERNORS_SUFFIX)) - .await - .map_err(|message| anyhow!("Error opening sysfs file for reading {message}")) + let base = path(CPU_PREFIX); + Ok(fs::read_to_string( + base.join(CPU0_NAME) + .join(CPU_SCALING_AVAILABLE_GOVERNORS_SUFFIX) + ) + .await?) } async fn read_cpu_governor_sysfs_contents() -> Result { // Read contents of policy0 path - let base = path(CPU_GOVERNOR_PREFIX); - let full_path = base.join(CPU0_NAME).join(CPU_GOVERNOR_SUFFIX); - fs::read_to_string(full_path) - .await - .map_err(|message| anyhow!("Error opening sysfs file for reading {message}")) + let base = path(CPU_PREFIX); + let full_path = base.join(CPU0_NAME).join(CPU_SCALING_GOVERNOR_SUFFIX); + Ok(fs::read_to_string(full_path).await?) } async fn write_cpu_governor_sysfs_contents(contents: String) -> Result<()> { - // Iterate over all cpuX paths - let mut dir = fs::read_dir(path(CPU_GOVERNOR_PREFIX)).await?; + // Iterate over all policyX paths + let mut dir = fs::read_dir(path(CPU_PREFIX)).await?; let mut wrote_stuff = false; loop { let base = match dir.next_entry().await? { @@ -238,34 +196,24 @@ async fn write_cpu_governor_sysfs_contents(contents: String) -> Result<()> { .file_name() .into_string() .map_err(|_| anyhow!("Unable to convert path to string"))?; - if file_name.starts_with(CPU_POLICY_NAME) { - entry.path() - } else { - // Not a policy path, so move on + if !file_name.starts_with(CPU_POLICY_NAME) { continue; } + entry.path() } None => { - if wrote_stuff { - return Ok(()); - } else { - bail!("No data written, unable to find any policyX sysfs paths") - } + ensure!( + wrote_stuff, + "No data written, unable to find any policyX sysfs paths" + ); + return Ok(()); } }; - let file_name = base.join(CPU_GOVERNOR_SUFFIX); - println!( - "Trying to write to file at path: {:?}", - file_name.as_os_str() - ); - let mut myfile = File::create(file_name) - .await - .inspect_err(|message| error!("Error opening sysfs file for writing: {message}"))?; - // Write contents to each one wrote_stuff = true; - myfile.write_all(contents.as_bytes()).await?; - myfile.sync_data().await?; + write_synced(base.join(CPU_SCALING_GOVERNOR_SUFFIX), contents.as_bytes()) + .await + .inspect_err(|message| error!("Error writing to sysfs file: {message}"))? } } @@ -360,41 +308,35 @@ pub(crate) async fn set_gpu_performance_level(level: GPUPerformanceLevel) -> Res .inspect_err(|message| error!("Error writing to sysfs file: {message}")) } -pub(crate) async fn get_cpu_governors() -> Result> { +pub(crate) async fn get_available_cpu_scaling_governors() -> Result> { let contents = read_cpu_governor_sysfs_available_contents().await?; // Get the list of supported governors from cpu0 - let mut result = HashMap::new(); + let mut result = Vec::new(); let words = contents.split_whitespace(); for word in words { - match CPUGovernor::from_str(word) { - Ok(v) => { - result.insert(v as u32, word.to_string()); - } - Err(message) => bail!("Error parsing governor {message}"), - }; + match CPUScalingGovernor::from_str(word) { + Ok(governor) => result.push(governor), + Err(message) => warn!("Error parsing governor {message}"), + } } Ok(result) } -pub(crate) async fn get_cpu_governor() -> Result { +pub(crate) async fn get_cpu_scaling_governor() -> Result { // get the current governor from cpu0 (assume all others are the same) let contents = read_cpu_governor_sysfs_contents().await?; - let governor = match CPUGovernor::from_str(contents.trim()) { - Ok(v) => v, - Err(_) => bail!("Error converting CPU governor sysfs file contents to enumeration"), - }; - - Ok(governor as u32) + let contents = contents.trim(); + CPUScalingGovernor::from_str(contents) + .map_err(|message| anyhow!("Error converting CPU scaling governor sysfs file contents to enumeration: {message}")) } -pub(crate) async fn set_cpu_governor(governor: CPUGovernor) -> Result<()> { +pub(crate) async fn set_cpu_scaling_governor(governor: CPUScalingGovernor) -> Result<()> { // Set the given governor on all cpus let name = governor.to_string(); - write_cpu_governor_sysfs_contents(name).await?; - Ok(()) + write_cpu_governor_sysfs_contents(name).await } pub(crate) async fn set_gpu_clocks(clocks: u32) -> Result<()> { @@ -803,13 +745,7 @@ CCLK_RANGE in Core0: #[test] fn cpu_governor_roundtrip() { - enum_roundtrip!(CPUGovernor { - 0: u32 = Conservative, - 1: u32 = OnDemand, - 2: u32 = UserSpace, - 3: u32 = PowerSave, - 4: u32 = Performance, - 5: u32 = SchedUtil, + enum_roundtrip!(CPUScalingGovernor { "conservative": str = Conservative, "ondemand": str = OnDemand, "userspace": str = UserSpace, @@ -817,8 +753,7 @@ CCLK_RANGE in Core0: "performance": str = Performance, "schedutil": str = SchedUtil, }); - assert!(CPUGovernor::try_from(6).is_err()); - assert!(CPUGovernor::from_str("usersave").is_err()); + assert!(CPUScalingGovernor::from_str("usersave").is_err()); } #[test] diff --git a/src/proxy.rs b/src/proxy.rs index 60c4421..9895c2f 100644 --- a/src/proxy.rs +++ b/src/proxy.rs @@ -65,15 +65,15 @@ trait Manager { #[zbus(property)] fn set_gpu_performance_level(&self, value: u32) -> zbus::Result<()>; - /// CpuGovernor property + /// CpuScalingGovernor property #[zbus(property)] - fn cpu_governor(&self) -> zbus::Result; + fn cpu_scaling_governor(&self) -> zbus::Result; #[zbus(property)] - fn set_cpu_governor(&self, value: u32) -> zbus::Result<()>; + fn set_cpu_scaling_governor(&self, value: String) -> zbus::Result<()>; - /// CpuGovernors property + /// AvailableCpuScalingGovernors property #[zbus(property)] - fn cpu_governors(&self) -> zbus::Result>; + fn available_cpu_scaling_governors(&self) -> zbus::Result>; /// GpuPowerProfile property #[zbus(property)]