GNOME Bugzilla – Bug 758413
Turn backlight helper into a D-Bus system service
Last modified: 2019-03-20 11:31:24 UTC
Created attachment 315983 [details] [review] dbus backlight helper The attached patch turns the backlight helper into a D-Bus system service. As a result, the brightness slider in GNOME Shell seems to be a lot less "laggy" (it occasionally seemed to have quite a lot of delay while dragging), and the journal isn't getting spammed with pkexec and PAM messages. There's still lots of room for improvement - the GSD-side code creates a new GDBusProxy for every change of brightness, while the helper-side code only checks for the best device at startup and doesn't track the device's brightness for changes. Maybe a proper backlight control service should be part of a lower component like systemd, instead? The latter already has a helper that provides brightness persistence across reboots.
As mentioned on IRC, I'd rather this service was available in a lower level package, such as systemd. It will probably also need changes to "future proof" it, such as being able to handle multiple outputs, and being able to match an output/screen to a particular level of brightness.
Review of attachment 315983 [details] [review]: ::: configure.ac @@ +232,3 @@ + xtst + gio-unix-2.0 + polkit-gobject-1 This looks spurious (no style changes combined with functional ones, please) ::: plugins/power/Makefile.am @@ +117,3 @@ $(BACKLIGHT_HELPER_CFLAGS) +org.gnome.SettingsDaemon.BacklightHelper.service: org.gnome.SettingsDaemon.BacklightHelper.service.in Makefile Needs to go in CLEANFILES. @@ +126,3 @@ +dbus_systemconf_DATA = org.gnome.SettingsDaemon.BacklightHelper.conf + +gsd-backlight-helper.service: gsd-backlight-helper.service.in Makefile Ditto ::: plugins/power/backlight-interface.xml @@ +2,3 @@ +"http://www.freedesktop.org/standards/dbus/1.0/introspect.dtd"> +<node> + <interface name="org.gnome.SettingsDaemon.BacklightHelper"> Well, you might as well have it replace the org.gnome.SettingsDaemon.Power.Screen API, but on the system instead of the session, no? I have no idea how that's going to play out with systems which poke the X11 xrandr property directly (through mutter), or whether we should make mutter implement this API in the backend. ::: plugins/power/org.gnome.SettingsDaemon.BacklightHelper.conf @@ +11,3 @@ + + <policy context="default"> + <allow send_destination="org.gnome.SettingsDaemon.BacklightHelper"/> This is likely wrong, it should be limited to users on the console.
(In reply to Bastien Nocera from comment #2) > ::: plugins/power/org.gnome.SettingsDaemon.BacklightHelper.conf > @@ +11,3 @@ > + > + <policy context="default"> > + <allow send_destination="org.gnome.SettingsDaemon.BacklightHelper"/> > > This is likely wrong, it should be limited to users on the console. AFAIK, D-Bus' at_console is being phased out in favor of polkit (https://lintian.debian.org/tags/dbus-policy-at-console.html), isn't it?
(In reply to Mantas Mikulėnas from comment #3) <snip> > AFAIK, D-Bus' at_console is being phased out in favor of polkit > (https://lintian.debian.org/tags/dbus-policy-at-console.html), isn't it? Sure, although stopping the D-Bus calls at their root, while we still have the daemon, is probably a good idea.
(In reply to Bastien Nocera from comment #4) > (In reply to Mantas Mikulėnas from comment #3) > <snip> > > AFAIK, D-Bus' at_console is being phased out in favor of polkit > > (https://lintian.debian.org/tags/dbus-policy-at-console.html), isn't it? > > Sure, although stopping the D-Bus calls at their root, while we still have > the daemon, is probably a good idea. With D-Bus maintainer hat on: we would prefer you to use polkit.
Review of attachment 315983 [details] [review]: Doing a D-Bus call to a service is much better than spawning a pkexec subprocess, so I would like to suggest not making the perfect the enemy of the good here. Even if this service isn't very future-proof and only lasts for a GNOME release or two before being superseded by something better, it's still better than spawning subprocesses to do the same things. ::: plugins/power/gpm-common.c @@ +423,3 @@ + helper = backlight_helper_proxy_new_for_bus_sync (G_BUS_TYPE_SYSTEM, + G_DBUS_PROXY_FLAGS_NONE, "org.gnome.SettingsDaemon.BacklightHelper", + "/org/gnome/SettingsDaemon/BacklightHelper", NULL, error); Caching this proxy would make it "cheaper". Failing that, use DO_NOT_CONNECT_SIGNALS if you aren't going to keep it around for long.
I'd really rather this lived in mutter where screens can be accessed. I don't see the point in having gnome-settings-daemon handle brightness when mutter already knows all there is to know about screens, crtcs and outputs.
This is more important to solve now that we have more and more super slow PWM and GPIO hooked brightness controls. Hans saw this on a Purism laptop where the brightness slider would laaaaag ages behind the mouse because the calls to the helper are sync, which means that we cannot handle D-Bus calls that happen in the meanwhile, so can't discard "outdated" requests. I'd recommend moving this functionality to the compositor though so that we can get per-screen brightness if it's ever possible technically.
(In reply to Bastien Nocera from comment #8) > Hans saw this on a Purism laptop where the brightness slider would laaaaag > ages behind the mouse because the calls to the helper are sync, which means > that we cannot handle D-Bus calls that happen in the meanwhile, so can't > discard "outdated" requests. This was not on a Purism laptop but on my XPS15, but it can be reproduced on almost any laptop, making the helper a dbus service is not really going to help here though. The real problem is that clutter generates way too many events when dragging the slider rather then starting a timer and say firing one event with the latest slider position every 20 ms. This means that if your mouse samples at 1000 Hz, we try to do 1000 brightness changes per second, which is just silly really.
As I see it we can do two things: * Put this into a dbus service to save overhead (which this patch does). * Call the helper asynchronously and compress multiple brightness setting calls into one. Both of these are improvements could be done independently and the second does not require the first. I would be happy to implement look into making the pkexec calls asynchronously to allow doing the correct magic in g-s-d. And yes, ideally we wouldn't be handling it in g-s-d, but it seems that there are currently no plans on how to do it in mutter.
(In reply to Hans de Goede from comment #9) > (In reply to Bastien Nocera from comment #8) > > Hans saw this on a Purism laptop where the brightness slider would laaaaag > > ages behind the mouse because the calls to the helper are sync, which means > > that we cannot handle D-Bus calls that happen in the meanwhile, so can't > > discard "outdated" requests. > > This was not on a Purism laptop but on my XPS15, but it can be reproduced on > almost any laptop, making the helper a dbus service is not really going to > help here though. The real problem is that clutter generates way too many > events when dragging the slider rather then starting a timer and say firing > one event with the latest slider position every 20 ms. This means that if > your mouse samples at 1000 Hz, we try to do 1000 brightness changes per > second, which is just silly really. Yes. And the service is such that it can't discard all those extra events. Whether you send 500 events per second or 5, if setting the brightness takes a second, it's going to feel laggy ;)
Created attachment 366613 [details] [review] Alright, this escalated a bit, but with these patches the whole thing is still inside gsd-power but completely asynchronous. I agree that handling this in mutter would be nice from an architectual point of view. Unfortunately, that doesn't solve the whole issue of needing the setuid helper and all rest of the weird logic. And to be honest, pushing all that into mutter also feels odd to me. So, here a solution that works without changing the whole infrastructure. power: Refactor backlight handling to be asynchronous Writing the backlight value can take quite long, in particular when we also get the overhead of starting up the helper process for writing. Add code to handle the writing asynchronously and compress multiple set actions into one write. The notification of the change only happens once all of the queued up set operations have finished. A trivial bugfix included is that the OSD display is again shown only on the affected (internal) screen.
Created attachment 366615 [details] [review] power: Simplify backlight helper to only support writing As it makes sense to handle udev events and everything in gsd-power, the only feature left that we actually need from the backlight helper is writing to the sysfs file. So strip it down to just check we are writing a sensible file and write out the value.
Review of attachment 366615 [details] [review]: FWIW, makes sense to me. Just had a quick look only though. ::: plugins/power/gsd-backlight-helper.c @@ +43,3 @@ + fprintf (stderr, "Usage: %s device brightness\n", argv[0]); + fprintf (stderr, " device: The backlight directory starting with \"/sys/class/backlight/\"\n"); + fprintf (stderr, " brightness: The new brightness to write\n"); g_printerr ? @@ +84,3 @@ + dp = opendir ("/sys/class/backlight"); + if (dp == NULL) { + fprintf (stderr, "Error: Could not open /sys/class/backlight\n"); Include underlying error via g_strerror? @@ -258,8 +84,14 @@ - ret = gsd_backlight_helper_write (filename, set_brightness, &error); - if (!ret) { - g_print ("%s: %s\n", ... 5 more ... + dp = opendir ("/sys/class/backlight"); + if (dp == NULL) { + fprintf (stderr, "Error: Could not open /sys/class/backlight\n"); ... 11 more ... free path? g_autofree char *path? @@ +96,3 @@ + snprintf (tmp, sizeof(tmp) - 11, "/sys/class/backlight/%s", ep->d_name); + path = realpath (tmp, NULL); + if (strcmp (path, device) == 0) { Personally, I'd also invert the flow, i.e. continue if no match, less indent, but that is just personal taste. @@ -258,8 +84,15 @@ - ret = gsd_backlight_helper_write (filename, set_brightness, &error); - if (!ret) { - g_print ("%s: %s\n", ... 5 more ... + dp = opendir ("/sys/class/backlight"); + if (dp == NULL) { + fprintf (stderr, "Error: Could not open /sys/class/backlight\n"); ... 12 more ... Since realpath can in theory return NULL (because you pass NULL as resolved_path) I felt more safe if you were to use g_strcmp0 ;) @@ +101,3 @@ + fd = open (tmp, O_WRONLY); + if (fd < 0) { + fprintf (stderr, "Error: Could not open brightness sysfs file (errno: %d)\n", errno); g_strerror instead of errno? @@ -258,8 +84,33 @@ - ret = gsd_backlight_helper_write (filename, set_brightness, &error); - if (!ret) { - g_print ("%s: %s\n", ... 5 more ... + dp = opendir ("/sys/class/backlight"); + if (dp == NULL) { + fprintf (stderr, "Error: Could not open /sys/class/backlight\n"); ... 30 more ... You want to close the dp here too to properly clean up
(In reply to Christian Kellner from comment #14) > Review of attachment 366615 [details] [review] [review]: > ::: plugins/power/gsd-backlight-helper.c > @@ +43,3 @@ > + fprintf (stderr, "Usage: %s device brightness\n", argv[0]); > + fprintf (stderr, " device: The backlight directory starting with > \"/sys/class/backlight/\"\n"); > + fprintf (stderr, " brightness: The new brightness to write\n"); > > g_printerr ? Sorry, totally missed you are not linking against glib anymore. Ignore that ;)
Hi Benjamin, Great that you are working on this, but TBH your approach seems overly complicated and still not very efficient. Ugh, I wrote a long comment on a better approach a while back, but that is not in this bug (or bugzilla ate it / user error), so I guess you did not see it, so now you've done all this work while IMHO there is a better method, sorry about this. Anyways I will describe (again) what I believe is the better way to deal with this and then you can decided to go that route or keep your current workaround. So the better approach IMHO would be to: 1) Not use a helper at all for reads, that is not necessary (not sure what the current code is actually doing for reads). 2) Change the helper binary to read values to write to the brightness attribute from stdin (simply send binary gint32 values down the pipe), popen (or glib equiv.) the binary once and keep it running. This will avoid the overhead of a doing a double exec (one for pkexec + one for the actual binary) as well as the likely huge overhead for doing the actually polkit check *each time*. 3) When setting the brightness simply write a gint32 to the stdin pipe and done, the kernel will buffer this, so no more delays here at all. 4) In the helper do a select() on stdin and when woken up do non-blocking reads of gint32-s until the non-blocking read fails with -EAGAIN, consuming all values in the pipe in one go and then write the last value read to the actual brightness sysfs attribute. To me this seems much more KISS (and more efficient) then what you've now, a downside of this is that the polkit check gets done only once, so if a user has access to the brightness when executing the helper, but later shouldn't (think fast-user switching) then instead of flooding the journal with pkexec errors if we've code still trying to change the brightness in the inactive session we now actually have the inactive session changing the brightness. I'm not sure if we can live with this downside and you already have working code for your approach so I guess it is probably better to just go with your approach, still I wanted to share this alternative. And thank you for working on this, it will be great to see this fixed. Regards, Hans
About what you said: 1. With the patches the helper is only used for writing. I have also added the logic to correctly re-read the brightness after udev events (and making sure we don't get jumpy property changed notifications on dbus). 2-4. Hrm, yeah, having a binary that just keeps running and feeding values into the kernel sounds like a neat idea. Though I think one may need to add some logic so that one knows the value has been written or we get race conditions if we try to re-read the brightness after udev events. Anyway, probably still sane to get this in, in my view. Removing all the async stuff (if possible) should be rather easy when we want to do it.
(In reply to Benjamin Berg from comment #17) > Anyway, probably still sane to get this in, in my view. Removing all the > async stuff (if possible) should be rather easy when we want to do it. Ack for getting your patches in more-or-less as is for now and maybe revisit this later.
Created attachment 369788 [details] [review] power: Refactor backlight handling to be asynchronous Writing the backlight value can take quite long, in particular when we also get the overhead of starting up the helper process for writing. Add code to handle the writing asynchronously and compress multiple set actions into one write. The notification of the change only happens once all of the queued up set operations have finished. A trivial bugfix included is that the OSD display is again shown only on the affected (internal) screen. Note that a possible further improvement would be to just run the backlight helper once and communicate with it through stdin or some other methods, avoiding the overhead of starting it up all the time. There are a few minor disadvantages with this approach though, see comment #16 and following for more information.
Created attachment 369789 [details] [review] power: Simplify backlight helper to only support writing As it makes sense to handle udev events and everything in gsd-power, the only feature left that we actually need from the backlight helper is writing to the sysfs file. So strip it down to just check we are writing a sensible file and write out the value.
Review of attachment 369788 [details] [review]: A couple of things: - I would make the code movement from gpm-common to gsd-backlight a preparatory patch - This really needs a separate patch that adds a test suite for this part of the code, using umockdev to create a backlight ::: plugins/power/gsd-power-manager.c @@ +1603,3 @@ + manager->priv->pre_dim_brightness, + NULL, NULL, NULL); + /* XXX: Ideally we would do this from the async callback. */ This probably needs fixing before merging. ::: plugins/power/meson.build @@ -1,3 @@ sources = files( 'gpm-common.c', - 'gsd-backlight-linux.c', I don't understand why this file needs to be renamed, or its code moved. That doesn't seem relevant to this patch. Can you split this up into a preparatory patch?
(In reply to Benjamin Berg from comment #20) > Created attachment 369789 [details] [review] [review] > power: Simplify backlight helper to only support writing > > As it makes sense to handle udev events and everything in gsd-power, udev events are usually only sent when the hardware modifies the backlight itself, and sends us back the results, rather than when the backlight is completely software controlled. So either that wouldn't work on a majority of machines (there are very few laptops which control the backlight themselves), or the commit message doesn't explain the change well enough for me to understand what the change is about.
> - I would make the code movement from gpm-common to gsd-backlight a > preparatory patch There is very little of the code still alive after the move. More likely, I would split the removal from the addition, simply removing the obsolete code in a second patch. (In reply to Bastien Nocera from comment #22) > (In reply to Benjamin Berg from comment #20) > > Created attachment 369789 [details] [review] [review] [review] > > power: Simplify backlight helper to only support writing > > > > As it makes sense to handle udev events and everything in gsd-power, > > udev events are usually only sent when the hardware modifies the backlight > itself, and sends us back the results, rather than when the backlight is > completely software controlled. So either that wouldn't work on a majority > of machines (there are very few laptops which control the backlight > themselves), or the commit message doesn't explain the change well enough > for me to understand what the change is about. Ah, yes the commit message is misleading. The old code did not handle uevents at all. The relevant change is that gsd-power now reads the data from sysfs itself rather than relying on the helper. So the helper only needs to check be able to read.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/issues/283.