From a63c091797bd352de7254c0a286829ec94a5e965 Mon Sep 17 00:00:00 2001 From: IlyaVrum Date: Mon, 24 Aug 2026 02:14:23 +0200 Subject: [PATCH] power: coalesce backlight helper writes, stop blocking on every step Every Screen.StepUp/StepDown call ran up to three synchronous g_spawn_command_line_sync() round-trips (get-brightness, get-max-brightness, then 'pkexec csd-backlight-helper --set-brightness' with a polkit round-trip), blocking the daemon's main loop for 60-400 ms per step. Keyboard autorepeat produces steps every ~33 ms, so holding a brightness key piled the calls up in the daemon's D-Bus queue: brightness kept 'running' for seconds after the key was released, and pressing the opposite key had no effect until the whole tail of the queue was processed. (Single presses were fine - one cycle completes before the next press.) Fix, following the approach gnome-settings-daemon took in gsd-backlight: - keep the last requested value (backlight_helper_target) and step from it instead of re-reading the hardware, answering the D-Bus call immediately; - write asynchronously (g_spawn_async + child watch) with a one-slot queue: at most one helper runs at a time, and when it finishes only the latest requested value is written. A burst of N steps now costs one or two helper invocations instead of N; - cache get-max-brightness - the maximum never changes at runtime, yet it was re-read through the helper on every single step; - GetPercentage reports the target while writes are in flight, so OSD and applet sliders stay in sync with what the user requested; - fix backlight_set_percentage falling through to the helper even after a successful xrandr write (the step functions already had the corresponding goto). Measured on a Mint 22.3 laptop (intel_backlight): a burst of 15 StepDown calls used to take ~1.4 s of serialized pkexec spawns and left the queue draining long after; now every call answers in ~1 ms, the burst results in exactly two helper writes (first + coalesced final value), and an opposite step right after the burst takes effect immediately. Co-Authored-By: Claude Fable 5 --- plugins/power/csd-power-manager.c | 192 +++++++++++++++++++++--------- 1 file changed, 135 insertions(+), 57 deletions(-) diff --git a/plugins/power/csd-power-manager.c b/plugins/power/csd-power-manager.c index 5754a85..d542b54 100644 --- a/plugins/power/csd-power-manager.c +++ b/plugins/power/csd-power-manager.c @@ -174,6 +174,10 @@ struct CsdPowerManagerPrivate gboolean skip_unsupported_xrandr; gboolean backlight_helper_force; gchar* backlight_helper_preference_args; + gint backlight_helper_target; + gint backlight_helper_max; + gboolean backlight_helper_write_in_flight; + gint backlight_helper_pending; gint kbd_brightness_now; gint kbd_brightness_max; gint kbd_brightness_old; @@ -2526,49 +2530,107 @@ backlight_helper_get_value (const gchar *argument, CsdPowerManager* manager, } /** - * backlight_helper_set_value: - * - * Sets a brightness value using the PolicyKit helper. - * - * Return value: Success. If FALSE then @error is set. + * Cached get-max-brightness: the maximum never changes at runtime, yet the + * old code spawned the helper to re-read it on every single step. **/ -static gboolean -backlight_helper_set_value (const gchar *argument, - gint value, - CsdPowerManager* manager, - GError **error) +static gint +backlight_helper_get_max (CsdPowerManager *manager, GError **error) +{ + gint64 max; + + if (manager->priv->backlight_helper_max > 0) + return manager->priv->backlight_helper_max; + + max = backlight_helper_get_value ("get-max-brightness", manager, error); + if (max > 0) + manager->priv->backlight_helper_max = max; + return max; +} + +static void backlight_helper_flush_pending (CsdPowerManager *manager); + +static void +backlight_helper_write_done_cb (GPid pid, gint status, gpointer user_data) +{ + CsdPowerManager *manager = CSD_POWER_MANAGER (user_data); + + g_spawn_close_pid (pid); + manager->priv->backlight_helper_write_in_flight = FALSE; + + if (manager->priv->backlight_helper_pending >= 0) { + /* more requests arrived while we were writing - write the + * final value only (coalescing) */ + backlight_helper_flush_pending (manager); + } +} + +static void +backlight_helper_flush_pending (CsdPowerManager *manager) { gboolean ret; - gint exit_status = 0; gchar *command = NULL; + gchar **argv = NULL; + GPid pid; + GError *error = NULL; + gint value = manager->priv->backlight_helper_pending; -#ifndef __linux__ - /* non-Linux platforms won't have /sys/class/backlight */ - g_set_error_literal (error, - CSD_POWER_MANAGER_ERROR, - CSD_POWER_MANAGER_ERROR_FAILED, - "The sysfs backlight helper is only for Linux"); - goto out; -#endif + if (value < 0 || manager->priv->backlight_helper_write_in_flight) + return; - /* get the data */ - command = g_strdup_printf ("pkexec " LIBEXECDIR "/csd-backlight-helper --%s %i %s", - argument, value, - manager->priv->backlight_helper_preference_args); - ret = g_spawn_command_line_sync (command, - NULL, - NULL, - &exit_status, - error); + manager->priv->backlight_helper_pending = -1; + manager->priv->backlight_helper_write_in_flight = TRUE; - g_debug ("executed %s retval: %i", command, exit_status); + command = g_strdup_printf ("pkexec " LIBEXECDIR "/csd-backlight-helper --set-brightness %i %s", + value, + manager->priv->backlight_helper_preference_args); + if (!g_shell_parse_argv (command, NULL, &argv, &error)) { + g_warning ("failed to parse brightness helper command: %s", error->message); + g_error_free (error); + manager->priv->backlight_helper_write_in_flight = FALSE; + goto out; + } - if (!ret || WEXITSTATUS (exit_status) != 0) + ret = g_spawn_async (NULL, argv, NULL, + G_SPAWN_DO_NOT_REAP_CHILD | G_SPAWN_SEARCH_PATH | + G_SPAWN_STDOUT_TO_DEV_NULL | G_SPAWN_STDERR_TO_DEV_NULL, + NULL, NULL, &pid, &error); + if (!ret) { + g_warning ("failed to spawn brightness helper: %s", error->message); + g_error_free (error); + manager->priv->backlight_helper_write_in_flight = FALSE; goto out; + } + g_child_watch_add (pid, backlight_helper_write_done_cb, manager); out: g_free (command); - return ret; + g_strfreev (argv); +} + +/** + * backlight_helper_queue_write: + * + * Asynchronous, coalescing replacement for the old synchronous + * "pkexec csd-backlight-helper --set-brightness" call. + * + * The old code blocked the main loop for the whole pkexec round-trip on + * EVERY StepUp/StepDown call. Keyboard autorepeat (~30 Hz) produces steps + * several times faster than that, so holding the brightness key piled the + * calls up in the daemon's D-Bus queue: brightness kept "running" long + * after the key was released, and an opposite key press had to wait for + * the whole tail of the queue before taking effect. + * + * Now each request just records the desired value and returns; at most one + * helper is in flight, and when it finishes only the LAST requested value + * is written. The queue can no longer outgrow one element. + **/ +static gboolean +backlight_helper_queue_write (CsdPowerManager *manager, gint value) +{ + manager->priv->backlight_helper_target = value; + manager->priv->backlight_helper_pending = value; + backlight_helper_flush_pending (manager); + return TRUE; } static void @@ -2641,14 +2703,20 @@ backlight_get_percentage (CsdPowerManager *manager, GError **error) } /* fall back to the polkit helper */ - max = backlight_helper_get_value ("get-max-brightness", manager, error); + max = backlight_helper_get_max (manager, error); if (max < 0) { goto out; } - now = backlight_helper_get_value ("get-brightness", manager, error); - if (now < 0) { - goto out; - } + /* while writes are being coalesced the hardware lags behind the last + * requested value - report the target so callers see fresh state */ + if (manager->priv->backlight_helper_target >= 0) { + now = manager->priv->backlight_helper_target; + } else { + now = backlight_helper_get_value ("get-brightness", manager, error); + if (now < 0) { + goto out; + } + } value = ABS_TO_PERCENTAGE (min_abs_brightness (manager, min, max), max, now); out: @@ -2686,6 +2754,8 @@ backlight_set_percentage (CsdPowerManager *manager, ret = gnome_rr_output_set_backlight (output, CLAMP (value, 0, 100), &grr_error); + if (ret) + goto out; if (grr_error != NULL) { g_debug ("Could not set backlight using xrandr: %s", grr_error->message); g_error_free (grr_error); @@ -2703,15 +2773,12 @@ backlight_set_percentage (CsdPowerManager *manager, gint new; /* fall back to the polkit helper */ - max = backlight_helper_get_value ("get-max-brightness", manager, error); + max = backlight_helper_get_max (manager, error); if (max < 0) goto out; new = CLAMP (PERCENTAGE_TO_ABS (min_abs_brightness (manager, min, max), max, value), min_abs_brightness (manager, min, max), max); - ret = backlight_helper_set_value ("set-brightness", - new, - manager, - error); + ret = backlight_helper_queue_write (manager, new); out: if (ret && emit_changed) backlight_emit_changed (manager); @@ -2770,18 +2837,22 @@ backlight_step_up (CsdPowerManager *manager, GError **error) gint min = 0; /* fall back to the polkit helper */ - current = backlight_helper_get_value ("get-brightness", manager, error); - if (current < 0) - goto out; - max = backlight_helper_get_value ("get-max-brightness", manager, error); + max = backlight_helper_get_max (manager, error); if (max < 0) goto out; + /* step from the last requested value, not from the hardware: while + * writes are coalesced the hardware lags behind, and stepping from a + * stale reading would make held-down keys lose steps */ + if (manager->priv->backlight_helper_target >= 0) { + current = manager->priv->backlight_helper_target; + } else { + current = backlight_helper_get_value ("get-brightness", manager, error); + if (current < 0) + goto out; + } step = BRIGHTNESS_STEP_AMOUNT (max - min_abs_brightness (manager, min, max)); new = MIN (current + step, max); - ret = backlight_helper_set_value ("set-brightness", - new, - manager, - error); + ret = backlight_helper_queue_write (manager, new); if (ret) percentage_value = ABS_TO_PERCENTAGE (min_abs_brightness (manager, min, max), max, new); out: @@ -2842,18 +2913,21 @@ backlight_step_down (CsdPowerManager *manager, GError **error) gint max = 0; /* fall back to the polkit helper */ - current = backlight_helper_get_value ("get-brightness", manager, error); - if (current < 0) - goto out; - max = backlight_helper_get_value ("get-max-brightness", manager, error); + max = backlight_helper_get_max (manager, error); if (max < 0) goto out; + /* step from the last requested value, not from the hardware (see + * backlight_step_up) */ + if (manager->priv->backlight_helper_target >= 0) { + current = manager->priv->backlight_helper_target; + } else { + current = backlight_helper_get_value ("get-brightness", manager, error); + if (current < 0) + goto out; + } step = BRIGHTNESS_STEP_AMOUNT (max - min_abs_brightness (manager, min, max)); new = MAX (current - step, min_abs_brightness (manager, min, max)); - ret = backlight_helper_set_value ("set-brightness", - new, - manager, - error); + ret = backlight_helper_queue_write (manager, new); if (ret) percentage_value = ABS_TO_PERCENTAGE (min_abs_brightness (manager, min, max), max, new); out: @@ -4886,6 +4960,10 @@ csd_power_manager_init (CsdPowerManager *manager) manager->priv = CSD_POWER_MANAGER_GET_PRIVATE (manager); manager->priv->inhibit_lid_switch_fd = -1; manager->priv->inhibit_suspend_fd = -1; + manager->priv->backlight_helper_target = -1; + manager->priv->backlight_helper_max = -1; + manager->priv->backlight_helper_write_in_flight = FALSE; + manager->priv->backlight_helper_pending = -1; } static void