Skip to content

power: coalesce backlight helper writes, stop blocking on every step #469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rumours86 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

power: coalesce backlight helper writes, stop blocking on every step #469

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
192 changes: 135 additions & 57 deletions plugins/power/csd-power-manager.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down