feat: settings confirmation (#163)

The goal of this PR is to have a confirmation dialog that the user has to click before the settings are saved onto disk. This is useful because when tweaking some settings (especially memory frequency), it's possible to crash your system, and if the settings are saved right away you end up with a soft-bricked system that will apply the broken settings on the next daemon start.
This commit is contained in:
Ilya Zlobintsev 2023-05-14 12:55:24 +03:00 committed by GitHub
parent 8c489a3fb9
commit 7f47339072
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 295 additions and 76 deletions

View File

@ -7,7 +7,7 @@ use anyhow::{anyhow, Context};
use nix::unistd::getuid;
use schema::{
amdgpu_sysfs::gpu_handle::{power_profile_mode::PowerProfileModesTable, PerformanceLevel},
request::SetClocksCommand,
request::{ConfirmCommand, SetClocksCommand},
ClocksInfo, DeviceInfo, DeviceListEntry, DeviceStats, FanCurveMap, Request, Response,
SystemInfo,
};
@ -104,13 +104,12 @@ impl DaemonClient {
id: &str,
enabled: bool,
curve: Option<FanCurveMap>,
) -> anyhow::Result<()> {
self.make_request::<()>(Request::SetFanControl { id, enabled, curve })?
.inner()?;
Ok(())
) -> anyhow::Result<u64> {
self.make_request(Request::SetFanControl { id, enabled, curve })?
.inner()
}
pub fn set_power_cap(&self, id: &str, cap: Option<f64>) -> anyhow::Result<()> {
pub fn set_power_cap(&self, id: &str, cap: Option<f64>) -> anyhow::Result<u64> {
self.make_request(Request::SetPowerCap { id, cap })?.inner()
}
@ -129,7 +128,7 @@ impl DaemonClient {
&self,
id: &str,
performance_level: PerformanceLevel,
) -> anyhow::Result<()> {
) -> anyhow::Result<u64> {
self.make_request(Request::SetPerformanceLevel {
id,
performance_level,
@ -137,15 +136,29 @@ impl DaemonClient {
.inner()
}
pub fn set_clocks_value(&self, id: &str, command: SetClocksCommand) -> anyhow::Result<()> {
pub fn set_clocks_value(&self, id: &str, command: SetClocksCommand) -> anyhow::Result<u64> {
self.make_request(Request::SetClocksValue { id, command })?
.inner()
}
pub fn set_power_profile_mode(&self, id: &str, index: Option<u16>) -> anyhow::Result<()> {
pub fn batch_set_clocks_value(
&self,
id: &str,
commands: Vec<SetClocksCommand>,
) -> anyhow::Result<u64> {
self.make_request(Request::BatchSetClocksValue { id, commands })?
.inner()
}
pub fn set_power_profile_mode(&self, id: &str, index: Option<u16>) -> anyhow::Result<u64> {
self.make_request(Request::SetPowerProfileMode { id, index })?
.inner()
}
pub fn confirm_pending_config(&self, command: ConfirmCommand) -> anyhow::Result<()> {
self.make_request(Request::ConfirmPendingConfig(command))?
.inner()
}
}
fn get_socket_path() -> Option<PathBuf> {

View File

@ -1,6 +1,6 @@
use crate::server::gpu_controller::fan_control::FanCurve;
use anyhow::Context;
use lact_schema::amdgpu_sysfs::gpu_handle::PerformanceLevel;
use lact_schema::{amdgpu_sysfs::gpu_handle::PerformanceLevel, request::SetClocksCommand};
use nix::unistd::getuid;
use serde::{Deserialize, Serialize};
use serde_with::skip_serializing_none;
@ -10,12 +10,24 @@ use tracing::debug;
const FILE_NAME: &str = "config.yaml";
const DEFAULT_ADMIN_GROUPS: [&str; 2] = ["wheel", "sudo"];
#[derive(Debug, Clone, Serialize, Deserialize, Default, PartialEq)]
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
pub struct Config {
pub daemon: Daemon,
#[serde(default = "default_apply_settings_timer")]
pub apply_settings_timer: u64,
pub gpus: HashMap<String, Gpu>,
}
impl Default for Config {
fn default() -> Self {
Self {
daemon: Daemon::default(),
apply_settings_timer: default_apply_settings_timer(),
gpus: HashMap::new(),
}
}
}
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
pub struct Daemon {
pub log_level: String,
@ -64,6 +76,28 @@ impl Gpu {
.iter()
.any(Option::is_some)
}
pub fn apply_clocks_command(&mut self, command: &SetClocksCommand) {
match command {
SetClocksCommand::MaxCoreClock(clock) => self.max_core_clock = Some(*clock),
SetClocksCommand::MaxMemoryClock(clock) => self.max_memory_clock = Some(*clock),
SetClocksCommand::MaxVoltage(voltage) => self.max_voltage = Some(*voltage),
SetClocksCommand::MinCoreClock(clock) => self.min_core_clock = Some(*clock),
SetClocksCommand::MinMemoryClock(clock) => self.min_memory_clock = Some(*clock),
SetClocksCommand::MinVoltage(voltage) => self.min_voltage = Some(*voltage),
SetClocksCommand::VoltageOffset(offset) => self.voltage_offset = Some(*offset),
SetClocksCommand::Reset => {
self.min_core_clock = None;
self.min_memory_clock = None;
self.min_voltage = None;
self.max_core_clock = None;
self.max_memory_clock = None;
self.max_voltage = None;
assert!(!self.is_core_clocks_used());
}
}
}
}
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
@ -119,6 +153,10 @@ fn get_path() -> PathBuf {
}
}
fn default_apply_settings_timer() -> u64 {
5
}
#[cfg(test)]
mod tests {
use super::{Config, Daemon, FanControlSettings, Gpu};
@ -141,6 +179,7 @@ mod tests {
},
)]
.into(),
..Default::default()
};
let data = serde_yaml::to_string(&config).unwrap();
let deserialized_config: Config = serde_yaml::from_str(&data).unwrap();

View File

@ -407,6 +407,9 @@ impl GpuController {
self.handle.set_active_power_profile_mode(mode_index)?;
}
// Reset the clocks table in case the settings get reverted back to not having a clocks value configured
self.handle.reset_clocks_table().ok();
if config.is_core_clocks_used() {
let mut table = self.handle.get_clocks_table()?;

View File

@ -3,17 +3,17 @@ use crate::config::{self, Config, FanControlSettings};
use anyhow::{anyhow, Context};
use lact_schema::{
amdgpu_sysfs::gpu_handle::{power_profile_mode::PowerProfileModesTable, PerformanceLevel},
request::SetClocksCommand,
request::{ConfirmCommand, SetClocksCommand},
ClocksInfo, DeviceInfo, DeviceListEntry, DeviceStats, FanCurveMap,
};
use std::{
collections::HashMap,
env,
path::PathBuf,
sync::{Arc, RwLock},
sync::{Arc, Mutex, RwLock},
time::Duration,
};
use tokio::time::sleep;
use tokio::{sync::oneshot, time::sleep};
use tracing::{debug, error, info, trace, warn};
const CONTROLLERS_LOAD_RETRY_ATTEMPTS: u8 = 5;
@ -23,6 +23,7 @@ const CONTROLLERS_LOAD_RETRY_INTERVAL: u64 = 1;
pub struct Handler {
pub config: Arc<RwLock<Config>>,
pub gpu_controllers: Arc<HashMap<String, GpuController>>,
confirm_config_tx: Arc<Mutex<Option<oneshot::Sender<ConfirmCommand>>>>,
}
impl<'a> Handler {
@ -46,6 +47,7 @@ impl<'a> Handler {
let handler = Self {
gpu_controllers: Arc::new(controllers),
config: Arc::new(RwLock::new(config)),
confirm_config_tx: Arc::new(Mutex::new(None)),
};
handler.load_config().await;
@ -70,31 +72,39 @@ impl<'a> Handler {
&self,
id: String,
f: F,
) -> anyhow::Result<()> {
let current_config = self
.config
.read()
) -> anyhow::Result<u64> {
if self
.confirm_config_tx
.try_lock()
.map_err(|err| anyhow!("{err}"))?
.gpus
.get(&id)
.cloned()
.unwrap_or_default();
.is_some()
{
return Err(anyhow!(
"There is an unconfirmed configuration change pending"
));
}
let mut new_config = current_config.clone();
let (gpu_config, apply_timer) = {
let config = self.config.read().map_err(|err| anyhow!("{err}"))?;
let apply_timer = config.apply_settings_timer;
let gpu_config = config.gpus.get(&id).cloned().unwrap_or_default();
(gpu_config, apply_timer)
};
let mut new_config = gpu_config.clone();
f(&mut new_config);
let controller = self.controller_by_id(&id)?;
match controller.apply_config(&new_config).await {
Ok(()) => {
let mut config_guard = self.config.write().unwrap();
config_guard.gpus.insert(id, new_config);
config_guard.save()?;
Ok(())
self.wait_config_confirm(id, gpu_config, new_config, apply_timer)
.await?;
Ok(apply_timer)
}
Err(apply_err) => {
error!("Could not apply settings: {apply_err:#}");
match controller.apply_config(&current_config).await {
error!("could not apply settings: {apply_err:#}");
match controller.apply_config(&gpu_config).await {
Ok(()) => Err(apply_err.context("Could not apply settings")),
Err(err) => Err(anyhow!("Could not apply settings, and could not reset to default settings: {err:#}")),
}
@ -102,6 +112,65 @@ impl<'a> Handler {
}
}
/// Should be called after applying new config without writing it
async fn wait_config_confirm(
&self,
id: String,
previous_config: config::Gpu,
new_config: config::Gpu,
apply_timer: u64,
) -> anyhow::Result<()> {
let (tx, rx) = oneshot::channel();
*self
.confirm_config_tx
.try_lock()
.map_err(|err| anyhow!("{err}"))? = Some(tx);
let handler = self.clone();
tokio::task::spawn(async move {
let controller = handler
.controller_by_id(&id)
.expect("GPU controller disappeared");
tokio::select! {
_ = tokio::time::sleep(Duration::from_secs(apply_timer)) => {
info!("no confirmation received, reverting settings");
if let Err(err) = controller.apply_config(&previous_config).await {
error!("could not revert settings: {err:#}");
}
}
result = rx => {
match result {
Ok(ConfirmCommand::Confirm) => {
info!("saving updated config");
let mut config_guard = handler.config.write().unwrap();
config_guard.gpus.insert(id, new_config);
if let Err(err) = config_guard.save() {
error!("{err}");
}
}
Ok(ConfirmCommand::Revert) | Err(_) => {
if let Err(err) = controller.apply_config(&previous_config).await {
error!("could not revert settings: {err:#}");
}
}
}
}
}
match handler.confirm_config_tx.try_lock() {
Ok(mut guard) => *guard = None,
Err(err) => error!("{err}"),
}
});
Ok(())
}
fn controller_by_id(&self, id: &str) -> anyhow::Result<&GpuController> {
Ok(self
.gpu_controllers
@ -145,7 +214,7 @@ impl<'a> Handler {
id: &str,
enabled: bool,
curve: Option<FanCurveMap>,
) -> anyhow::Result<()> {
) -> anyhow::Result<u64> {
let settings = match curve {
Some(raw_curve) => {
let curve = FanCurve(raw_curve);
@ -175,7 +244,7 @@ impl<'a> Handler {
.await
}
pub async fn set_power_cap(&'a self, id: &str, maybe_cap: Option<f64>) -> anyhow::Result<()> {
pub async fn set_power_cap(&'a self, id: &str, maybe_cap: Option<f64>) -> anyhow::Result<u64> {
self.edit_gpu_config(id.to_owned(), |gpu_config| {
gpu_config.power_cap = maybe_cap;
})
@ -186,7 +255,7 @@ impl<'a> Handler {
&self,
id: &str,
level: PerformanceLevel,
) -> anyhow::Result<()> {
) -> anyhow::Result<u64> {
self.edit_gpu_config(id.to_owned(), |gpu_config| {
gpu_config.performance_level = Some(level);
})
@ -197,28 +266,25 @@ impl<'a> Handler {
&self,
id: &str,
command: SetClocksCommand,
) -> anyhow::Result<()> {
) -> anyhow::Result<u64> {
if let SetClocksCommand::Reset = command {
self.controller_by_id(id)?.handle.reset_clocks_table()?;
}
self.edit_gpu_config(id.to_owned(), |gpu_config| match command {
SetClocksCommand::MaxCoreClock(clock) => gpu_config.max_core_clock = Some(clock),
SetClocksCommand::MaxMemoryClock(clock) => gpu_config.max_memory_clock = Some(clock),
SetClocksCommand::MaxVoltage(voltage) => gpu_config.max_voltage = Some(voltage),
SetClocksCommand::MinCoreClock(clock) => gpu_config.min_core_clock = Some(clock),
SetClocksCommand::MinMemoryClock(clock) => gpu_config.min_memory_clock = Some(clock),
SetClocksCommand::MinVoltage(voltage) => gpu_config.min_voltage = Some(voltage),
SetClocksCommand::VoltageOffset(offset) => gpu_config.voltage_offset = Some(offset),
SetClocksCommand::Reset => {
gpu_config.min_core_clock = None;
gpu_config.min_memory_clock = None;
gpu_config.min_voltage = None;
gpu_config.max_core_clock = None;
gpu_config.max_memory_clock = None;
gpu_config.max_voltage = None;
self.edit_gpu_config(id.to_owned(), |gpu_config| {
gpu_config.apply_clocks_command(&command);
})
.await
}
assert!(!gpu_config.is_core_clocks_used());
pub async fn batch_set_clocks_value(
&self,
id: &str,
commands: Vec<SetClocksCommand>,
) -> anyhow::Result<u64> {
self.edit_gpu_config(id.to_owned(), |gpu_config| {
for command in commands {
gpu_config.apply_clocks_command(&command);
}
})
.await
@ -232,13 +298,31 @@ impl<'a> Handler {
Ok(modes_table)
}
pub async fn set_power_profile_mode(&self, id: &str, index: Option<u16>) -> anyhow::Result<()> {
pub async fn set_power_profile_mode(
&self,
id: &str,
index: Option<u16>,
) -> anyhow::Result<u64> {
self.edit_gpu_config(id.to_owned(), |gpu_config| {
gpu_config.power_profile_mode_index = index;
})
.await
}
pub fn confirm_pending_config(&self, command: ConfirmCommand) -> anyhow::Result<()> {
if let Some(tx) = self
.confirm_config_tx
.try_lock()
.map_err(|err| anyhow!("{err}"))?
.take()
{
tx.send(command)
.map_err(|_| anyhow!("Could not confirm config"))
} else {
Err(anyhow!("No pending config changes"))
}
}
pub async fn cleanup(self) {
let disable_clocks_cleanup = self
.config

View File

@ -99,10 +99,16 @@ async fn handle_request<'a>(request: Request<'a>, handler: &'a Handler) -> anyho
Request::SetClocksValue { id, command } => {
ok_response(handler.set_clocks_value(id, command).await?)
}
Request::BatchSetClocksValue { id, commands } => {
ok_response(handler.batch_set_clocks_value(id, commands).await?)
}
Request::SetPowerProfileMode { id, index } => {
ok_response(handler.set_power_profile_mode(id, index).await?)
}
Request::EnableOverdrive => ok_response(system::enable_overdrive()?),
Request::ConfirmPendingConfig(command) => {
ok_response(handler.confirm_pending_config(command)?)
}
}
}

View File

@ -8,7 +8,7 @@ use apply_revealer::ApplyRevealer;
use glib::clone;
use gtk::{gio::ApplicationFlags, prelude::*, *};
use header::Header;
use lact_client::schema::request::SetClocksCommand;
use lact_client::schema::request::{ConfirmCommand, SetClocksCommand};
use lact_client::schema::DeviceStats;
use lact_client::DaemonClient;
use lact_daemon::MODULE_CONF_PATH;
@ -101,7 +101,9 @@ impl App {
let gpu_id = current_gpu_id.read().unwrap();
match app.daemon_client.set_clocks_value(&gpu_id, SetClocksCommand::Reset) {
match app.daemon_client.set_clocks_value(&gpu_id, SetClocksCommand::Reset)
.and_then(|_| app.daemon_client.confirm_pending_config(ConfirmCommand::Confirm))
{
Ok(()) => {
app.set_initial(&gpu_id);
}
@ -275,6 +277,7 @@ impl App {
fn apply_settings(&self, current_gpu_id: Arc<RwLock<String>>) -> anyhow::Result<()> {
debug!("applying settings");
// TODO: Ask confirmation for everything, not just clocks
let gpu_id = current_gpu_id.read().unwrap();
@ -282,17 +285,27 @@ impl App {
self.daemon_client
.set_power_cap(&gpu_id, Some(cap))
.context("Failed to set power cap")?;
self.daemon_client
.confirm_pending_config(ConfirmCommand::Confirm)
.context("Could not commit config")?;
}
// Reset the power profile mode for switching to/from manual performance level
self.daemon_client
.set_power_profile_mode(&gpu_id, None)
.context("Could not set default power profile mode")?;
self.daemon_client
.confirm_pending_config(ConfirmCommand::Confirm)
.context("Could not commit config")?;
if let Some(level) = self.root_stack.oc_page.get_performance_level() {
self.daemon_client
.set_performance_level(&gpu_id, level)
.context("Failed to set power profile")?;
self.daemon_client
.confirm_pending_config(ConfirmCommand::Confirm)
.context("Could not commit config")?;
let mode_index = self
.root_stack
@ -302,6 +315,9 @@ impl App {
self.daemon_client
.set_power_profile_mode(&gpu_id, mode_index)
.context("Could not set active power profile mode")?;
self.daemon_client
.confirm_pending_config(ConfirmCommand::Confirm)
.context("Could not commit config")?;
}
if let Some(thermals_settings) = self.root_stack.thermals_page.get_thermals_settings() {
@ -314,52 +330,50 @@ impl App {
thermals_settings.curve,
)
.context("Could not set fan control")?;
self.daemon_client
.confirm_pending_config(ConfirmCommand::Confirm)
.context("Could not commit config")?;
}
let clocks_settings = self.root_stack.oc_page.clocks_frame.get_settings();
let mut clocks_commands = Vec::new();
debug!("applying clocks settings {clocks_settings:#?}");
if let Some(clock) = clocks_settings.min_core_clock {
self.daemon_client
.set_clocks_value(&gpu_id, SetClocksCommand::MinCoreClock(clock))
.context("Could not set the minimum core clock")?;
clocks_commands.push(SetClocksCommand::MinCoreClock(clock));
}
if let Some(clock) = clocks_settings.min_memory_clock {
self.daemon_client
.set_clocks_value(&gpu_id, SetClocksCommand::MinMemoryClock(clock))
.context("Could not set the minimum memory clock")?;
clocks_commands.push(SetClocksCommand::MinMemoryClock(clock));
}
if let Some(voltage) = clocks_settings.min_voltage {
self.daemon_client
.set_clocks_value(&gpu_id, SetClocksCommand::MinVoltage(voltage))
.context("Could not set the minimum voltage")?;
clocks_commands.push(SetClocksCommand::MinVoltage(voltage));
}
if let Some(clock) = clocks_settings.max_core_clock {
self.daemon_client
.set_clocks_value(&gpu_id, SetClocksCommand::MaxCoreClock(clock))
.context("Could not set the maximum core clock")?;
clocks_commands.push(SetClocksCommand::MaxCoreClock(clock));
}
if let Some(clock) = clocks_settings.max_memory_clock {
self.daemon_client
.set_clocks_value(&gpu_id, SetClocksCommand::MaxMemoryClock(clock))
.context("Could not set the maximum memory clock")?;
clocks_commands.push(SetClocksCommand::MaxMemoryClock(clock));
}
if let Some(voltage) = clocks_settings.max_voltage {
self.daemon_client
.set_clocks_value(&gpu_id, SetClocksCommand::MaxVoltage(voltage))
.context("Could not set the maximum voltage")?;
clocks_commands.push(SetClocksCommand::MaxVoltage(voltage));
}
if let Some(offset) = clocks_settings.voltage_offset {
self.daemon_client
.set_clocks_value(&gpu_id, SetClocksCommand::VoltageOffset(offset))
.context("Could not set the voltage offset")?;
clocks_commands.push(SetClocksCommand::VoltageOffset(offset));
}
if !clocks_commands.is_empty() {
let delay = self
.daemon_client
.batch_set_clocks_value(&gpu_id, clocks_commands)
.context("Could not commit clocks settins")?;
self.ask_confirmation(gpu_id.clone(), delay);
}
self.set_initial(&gpu_id);
@ -400,6 +414,50 @@ impl App {
diag.hide();
}));
}
fn ask_confirmation(&self, gpu_id: String, mut delay: u64) {
let text = confirmation_text(delay);
let dialog = MessageDialog::builder()
.title("Confirm settings")
.text(text)
.message_type(MessageType::Question)
.buttons(ButtonsType::YesNo)
.build();
glib::source::timeout_add_local(
Duration::from_secs(1),
clone!(@strong dialog, @strong self as app, @strong gpu_id => move || {
delay -= 1;
let text = confirmation_text(delay);
dialog.set_text(Some(&text));
if delay == 0 {
dialog.hide();
app.set_initial(&gpu_id);
Continue(false)
} else {
Continue(true)
}
}),
);
dialog.run_async(clone!(@strong self as app => move |diag, response| {
let command = match response {
ResponseType::Yes => ConfirmCommand::Confirm,
ResponseType::No => ConfirmCommand::Revert,
_ => unreachable!(),
};
diag.hide();
if let Err(err) = app.daemon_client.confirm_pending_config(command) {
show_error(&app.window, err);
}
app.set_initial(&gpu_id);
}));
}
}
enum GuiUpdateMsg {
@ -420,3 +478,7 @@ fn show_error(parent: &ApplicationWindow, err: anyhow::Error) {
diag.hide();
})
}
fn confirmation_text(seconds_left: u64) -> String {
format!("Do you want to keep the new settings? (Reverting in {seconds_left} seconds)")
}

View File

@ -37,11 +37,23 @@ pub enum Request<'a> {
id: &'a str,
command: SetClocksCommand,
},
BatchSetClocksValue {
id: &'a str,
commands: Vec<SetClocksCommand>,
},
SetPowerProfileMode {
id: &'a str,
index: Option<u16>,
},
EnableOverdrive,
ConfirmPendingConfig(ConfirmCommand),
}
#[derive(Serialize, Deserialize, Debug, PartialEq)]
#[serde(tag = "command", rename_all = "snake_case")]
pub enum ConfirmCommand {
Confirm,
Revert,
}
#[derive(Serialize, Deserialize, Debug, PartialEq)]