[PATCH 1/3] xf86-video-intel: Fix fd_set_nonblock

[PATCH 1/3] xf86-video-intel: Fix fd_set_nonblock

[Hans de Goede]

O_NONBLOCK is a status flag not a descriptor flag, so F_GETFL / F_SETFL should
be used to modify it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 src/intel_device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/intel_device.c b/src/intel_device.c
index effda24..07b7d2e 100644
--- a/src/intel_device.c
+++ b/src/intel_device.c
@@ -167,10 +167,10 @@ static int fd_set_nonblock(int fd)
 	if (fd == -1)
 		return fd;
 
-	flags = fcntl(fd, F_GETFD);
+	flags = fcntl(fd, F_GETFL);
 	if (flags != -1) {
 		flags |= O_NONBLOCK;
-		fcntl(fd, F_SETFD, flags);
+		fcntl(fd, F_SETFL, flags);
 	}
 
 	return fd;
-- 
1.8.5.3

[PATCH 2/3] xf86-video-intel: export fd_set_cloexec / fd_set_nonblock

[Hans de Goede]

Allow fd_set_cloexec / fd_set_nonblock to be used outside of intel_device.c.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 src/intel_device.c | 8 ++++----
 src/intel_driver.h | 2 ++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/intel_device.c b/src/intel_device.c
index 07b7d2e..fd0b735 100644
--- a/src/intel_device.c
+++ b/src/intel_device.c
@@ -142,7 +142,7 @@ static int __intel_check_device(int fd)
 	return ret;
 }
 
-static int fd_set_cloexec(int fd)
+int intel_fd_set_cloexec(int fd)
 {
 	int flags;
 
@@ -160,7 +160,7 @@ static int fd_set_cloexec(int fd)
 	return fd;
 }
 
-static int fd_set_nonblock(int fd)
+int intel_fd_set_nonblock(int fd)
 {
 	int flags;
 
@@ -209,7 +209,7 @@ static int __intel_open_device(const struct pci_device *pci, char **path)
 				fd = -1;
 			}
 		}
-		fd = fd_set_nonblock(fd);
+		fd = intel_fd_set_nonblock(fd);
 	} else {
 #ifdef O_CLOEXEC
 		fd = open(*path, O_RDWR | O_NONBLOCK | O_CLOEXEC);
@@ -217,7 +217,7 @@ static int __intel_open_device(const struct pci_device *pci, char **path)
 		fd = -1;
 #endif
 		if (fd == -1)
-			fd = fd_set_cloexec(open(*path, O_RDWR | O_NONBLOCK));
+			fd = intel_fd_set_cloexec(open(*path, O_RDWR | O_NONBLOCK));
 	}
 
 	return fd;
diff --git a/src/intel_driver.h b/src/intel_driver.h
index b2cb1b9..52c2768 100644
--- a/src/intel_driver.h
+++ b/src/intel_driver.h
@@ -126,6 +126,8 @@ int intel_open_device(int entity_num,
 		      const struct pci_device *pci,
 		      struct xf86_platform_device *dev);
 int intel_get_device(ScrnInfoPtr scrn);
+int intel_fd_set_cloexec(int fd);
+int intel_fd_set_nonblock(int fd);
 const char *intel_get_client_name(ScrnInfoPtr scrn);
 int intel_get_device_id(ScrnInfoPtr scrn);
 int intel_get_master(ScrnInfoPtr scrn);
-- 
1.8.5.3

[PATCH 3/3] xf86-video-intel: Add a helper for setting backlight without root rights

[Hans de Goede]

Once the xserver stops running as root on kms capabable systems, we will need
some other way to access the backlight.

The approach taken in this patch leaves most of the heavy lifting (wrt
doing everything suid root safe) to pkexec, as is done in ie
gnome-settings-daemon, which controls the backlight directly on ati and
nouveau cards.

This commit adds src/backlight.h and src/backlight.c as a place to share common
backlight code, in the future most of the duplicate backlight code inside
src/sna/sna_display.c and src/uxa/intel_display.c should be moved there.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 Makefile.am                                        |   4 +
 backlight_helper/Makefile.am                       |  37 ++++++
 backlight_helper/backlight_helper.c                |  37 ++++++
 ...g.x.xf86-video-intel.backlight-helper.policy.in |  19 +++
 configure.ac                                       |  11 ++
 src/Makefile.am                                    |   2 +
 src/backlight.c                                    | 142 +++++++++++++++++++++
 src/backlight.h                                    |  22 ++++
 src/sna/sna_display.c                              |  17 +--
 src/uxa/intel_display.c                            |  17 +--
 10 files changed, 292 insertions(+), 16 deletions(-)
 create mode 100644 backlight_helper/Makefile.am
 create mode 100644 backlight_helper/backlight_helper.c
 create mode 100644 backlight_helper/org.x.xf86-video-intel.backlight-helper.policy.in
 create mode 100644 src/backlight.c
 create mode 100644 src/backlight.h

diff --git a/Makefile.am b/Makefile.am
index 71c7698..d6b97b6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -26,6 +26,10 @@ if BUILD_TOOLS
 SUBDIRS += tools
 endif
 
+if BUILD_BACKLIGHT_HELPER
+SUBDIRS += backlight_helper
+endif
+
 MAINTAINERCLEANFILES = ChangeLog INSTALL
 
 if HAVE_X11
diff --git a/backlight_helper/Makefile.am b/backlight_helper/Makefile.am
new file mode 100644
index 0000000..a4a5050
--- /dev/null
+++ b/backlight_helper/Makefile.am
@@ -0,0 +1,37 @@
+#  Copyright 2005 Adam Jackson.
+#
+#  Permission is hereby granted, free of charge, to any person obtaining a
+#  copy of this software and associated documentation files (the "Software"),
+#  to deal in the Software without restriction, including without limitation
+#  on the rights to use, copy, modify, merge, publish, distribute, sub
+#  license, and/or sell copies of the Software, and to permit persons to whom
+#  the Software is furnished to do so, subject to the following conditions:
+#
+#  The above copyright notice and this permission notice (including the next
+#  paragraph) shall be included in all copies or substantial portions of the
+#  Software.
+#
+#  THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+#  IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+#  FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.  IN NO EVENT SHALL
+#  ADAM JACKSON BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER
+#  IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+#  CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+AM_CFLAGS = \
+	@CWARNFLAGS@ \
+	@NOWARNFLAGS@ \
+	$(NULL)
+
+backlight_helperdir = $(prefix)/libexec
+policydir = $(datarootdir)/polkit-1/actions
+
+backlight_helper_PROGRAMS = xf86-video-intel-backlight-helper
+nodist_policy_DATA = org.x.xf86-video-intel.backlight-helper.policy
+
+xf86_video_intel_backlight_helper_SOURCES = \
+	backlight_helper.c \
+	$(NULL)
+
+EXTRA_DIST = org.x.xf86-video-intel.backlight-helper.policy.in
+DISTCLEANFILES = $(nodist_policy_DATA)
diff --git a/backlight_helper/backlight_helper.c b/backlight_helper/backlight_helper.c
new file mode 100644
index 0000000..c681aae
--- /dev/null
+++ b/backlight_helper/backlight_helper.c
@@ -0,0 +1,37 @@
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#define BUF_SIZE 1024
+
+int main(int argc, char *argv[])
+{
+	char buf[BUF_SIZE];
+	FILE *f;
+	int r;
+
+	if (argc != 3) {
+		fprintf(stderr, "Error %s expects exactly 2 parameters\n",
+			argv[0]);
+		fprintf(stderr, "Usage: %s <iface> <level>\n", argv[0]);
+		return 1;
+	}
+
+	snprintf(buf, BUF_SIZE, "/sys/class/backlight/%s/brightness", argv[1]);
+	f = fopen(buf, "r+");
+	if (!f) {
+		fprintf(stderr, "Error opening %s: %s\n", buf, strerror(errno));
+		return 1;
+	}
+	
+	r = fprintf(f, "%s\n", argv[2]);	
+	if (r < 0) {
+		fprintf(stderr, "Error writing %s: %s\n", buf, strerror(errno));
+		return 1;
+	}
+	
+	fclose(f);
+
+	return 0;
+}
diff --git a/backlight_helper/org.x.xf86-video-intel.backlight-helper.policy.in b/backlight_helper/org.x.xf86-video-intel.backlight-helper.policy.in
new file mode 100644
index 0000000..37e9622
--- /dev/null
+++ b/backlight_helper/org.x.xf86-video-intel.backlight-helper.policy.in
@@ -0,0 +1,19 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE policyconfig PUBLIC
+ "-//freedesktop//DTD PolicyKit Policy Configuration 1.0//EN"
+ "http://www.freedesktop.org/standards/PolicyKit/1.0/policyconfig.dtd">
+<policyconfig>
+  <vendor>The X.Org project</vendor>
+  <vendor_url>https://01.org/linuxgraphics/community/xf86-video-intel</vendor_url>
+  <icon_name>brightness</icon_name>
+  <action id="org.x.xf86-video-intel.backlight-helper">
+    <description>Modify lcd panel brightness</description>
+    <message>Authentication is required to modify the lcd panel brightness</message>
+    <defaults>
+      <allow_any>no</allow_any>
+      <allow_inactive>no</allow_inactive>
+      <allow_active>yes</allow_active>
+    </defaults>
+    <annotate key="org.freedesktop.policykit.exec.path">@prefix@/libexec/xf86-video-intel-backlight-helper</annotate>
+  </action>
+</policyconfig>
diff --git a/configure.ac b/configure.ac
index 4f73ba4..6bc80de 100644
--- a/configure.ac
+++ b/configure.ac
@@ -62,6 +62,14 @@ AC_DISABLE_STATIC
 AC_PROG_LIBTOOL
 AC_SYS_LARGEFILE
 
+# Platform specific settings
+case $host_os in
+  *linux*)
+    backlight_helper=yes
+    ;;
+esac
+AM_CONDITIONAL(BUILD_BACKLIGHT_HELPER, [test "x$backlight_helper" = "xyes"])
+
 # Are we in a git checkout?
 dot_git=no
 if test -e .git; then
@@ -681,6 +689,7 @@ fi
 DRIVER_NAME="intel"
 AC_SUBST([DRIVER_NAME])
 AC_SUBST([moduledir])
+AC_DEFINE_DIR([PREFIX_PATH], prefix, [installation prefix])
 
 AC_CONFIG_FILES([
                 Makefile
@@ -700,6 +709,8 @@ AC_CONFIG_FILES([
                 xvmc/shader/vld/Makefile
 		test/Makefile
 		tools/Makefile
+		backlight_helper/Makefile
+		backlight_helper/org.x.xf86-video-intel.backlight-helper.policy
 ])
 AC_OUTPUT
 
diff --git a/src/Makefile.am b/src/Makefile.am
index e87f030..842d0b2 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -59,6 +59,8 @@ intel_drv_la_SOURCES = \
 	intel_driver.h \
 	intel_options.c \
 	intel_module.c \
+	backlight.c \
+	backlight.h \
 	compat-api.h \
 	$(NULL)
 
diff --git a/src/backlight.c b/src/backlight.c
new file mode 100644
index 0000000..a98b6e6
--- /dev/null
+++ b/src/backlight.c
@@ -0,0 +1,142 @@
+/***************************************************************************
+
+ Copyright 2013 Intel Corporation.  All Rights Reserved.
+
+ Permission is hereby granted, free of charge, to any person obtaining a
+ copy of this software and associated documentation files (the
+ "Software"), to deal in the Software without restriction, including
+ without limitation the rights to use, copy, modify, merge, publish,
+ distribute, sub license, and/or sell copies of the Software, and to
+ permit persons to whom the Software is furnished to do so, subject to
+ the following conditions:
+
+ The above copyright notice and this permission notice (including the
+ next paragraph) shall be included in all copies or substantial portions
+ of the Software.
+
+ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+ OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
+ IN NO EVENT SHALL INTEL, AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
+ DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR
+ THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+ **************************************************************************/
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include <errno.h>
+#include <fcntl.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <xorg-server.h>
+#include <xf86.h>
+#include <xf86Crtc.h>
+#include "intel_driver.h"
+#include "backlight.h"
+
+#define READ_PIPE	0
+#define WRITE_PIPE	1
+
+#ifdef __linux__ /* The helper is for linux only */
+
+/*
+ * Unfortunately this is not as simple as I would like it to be. If selinux is
+ * dropping dbus messages pkexec may block *forever*.
+ *
+ * Backgrounding pkexec by doing System("pkexec ...&") does not work because
+ * that detaches pkexec from its parent at which point its security checks
+ * fail and it refuses to execute the helper.
+ *
+ * So we're left with spawning a helper child which gets levels to set written
+ * to it through a pipe. This turns the blocking forever problem from a hung
+ * machine problem into a simple backlight control not working problem.
+ */
+
+static void backlight_helper_main(struct backlight_helper *helper)
+{
+	char cmd[1024];
+	int r, level;
+
+	while (1) {
+		r = read(helper->fds[READ_PIPE], &level, sizeof(level));
+		if (r != sizeof(level))
+			break;
+		snprintf(cmd, sizeof(cmd),
+		  "pkexec %s/libexec/xf86-video-intel-backlight-helper %s %d",
+		  PREFIX_PATH, helper->iface, level);
+		System(cmd);
+	}
+	close(helper->fds[READ_PIPE]);
+	_exit(0);
+}
+
+static int backlight_helper_init(struct backlight_helper *helper,
+				  xf86OutputPtr output, const char *iface)
+{
+	int r;
+
+	if (helper->child_created)
+		return 0;
+
+	helper->iface = iface;
+
+	r = pipe(helper->fds);
+	if (r) {
+		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+			   "backlight pipe error: %s\n", strerror(errno));
+		return r;
+	}
+	intel_fd_set_cloexec(helper->fds[READ_PIPE]);
+	intel_fd_set_cloexec(helper->fds[WRITE_PIPE]);
+	intel_fd_set_nonblock(helper->fds[WRITE_PIPE]);
+
+	helper->pid = fork();
+	switch (helper->pid) {
+	case -1:
+		close(helper->fds[READ_PIPE]);
+		close(helper->fds[WRITE_PIPE]);
+		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+			   "backlight fork error: %s\n", strerror(errno));
+		return -1;
+	case 0:
+		close(helper->fds[WRITE_PIPE]);
+		backlight_helper_main(helper);
+		break;
+	default:
+		close(helper->fds[READ_PIPE]);
+		helper->child_created = 1;
+	}
+	return 0;
+}
+
+void backlight_helper_set(struct backlight_helper *helper,
+			  xf86OutputPtr output, const char *iface, int level)
+{
+	int r;
+
+	r = backlight_helper_init(helper, output, iface);
+	if (r)
+		return;
+
+	r = write(helper->fds[WRITE_PIPE], &level, sizeof(level));
+	if (r != sizeof(level))
+		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+			   "backlight write error: %s\n", strerror(errno));
+}
+
+void backlight_helper_cleanup(struct backlight_helper *helper)
+{
+	if (!helper->child_created)
+		return;
+
+	close(helper->fds[WRITE_PIPE]);
+	waitpid(helper->pid, NULL, 0);
+}
+
+#endif /* The helper is for linux only */
diff --git a/src/backlight.h b/src/backlight.h
new file mode 100644
index 0000000..dd036c4
--- /dev/null
+++ b/src/backlight.h
@@ -0,0 +1,22 @@
+#ifndef BACKLIGHT_H
+#define BACKLIGHT_H
+
+struct backlight_helper {
+	int child_created;
+	int fds[2];
+	int pid;
+	const char *iface;
+};
+
+#ifdef __linux__ 
+
+void backlight_helper_set(struct backlight_helper *helper,
+			 xf86OutputPtr output, const char *iface, int level);
+void backlight_helper_cleanup(struct backlight_helper *helper);
+
+#else
+
+#define backlight_helper_cleanup(helper)
+
+#endif
+#endif
diff --git a/src/sna/sna_display.c b/src/sna/sna_display.c
index 521d4ef..559c8fa 100644
--- a/src/sna/sna_display.c
+++ b/src/sna/sna_display.c
@@ -44,6 +44,7 @@
 #include "sna_reg.h"
 #include "fb/fbpict.h"
 #include "intel_options.h"
+#include "backlight.h"
 
 #include <xf86Crtc.h>
 
@@ -144,6 +145,7 @@ struct sna_output {
 	char *backlight_iface;
 	int backlight_active_level;
 	int backlight_max;
+	struct backlight_helper backlight_helper;
 
 	int num_modes;
 	struct drm_mode_modeinfo *modes;
@@ -391,6 +393,12 @@ sna_output_backlight_set(xf86OutputPtr output, int level)
 		BACKLIGHT_CLASS, sna_output->backlight_iface);
 	fd = open(path, O_RDWR);
 	if (fd == -1) {
+		/* If this is an access error, invoke the helper instead */
+		if (errno == EACCES)  {
+			backlight_helper_set(&sna_output->backlight_helper,
+				output, sna_output->backlight_iface, level);
+			return;
+		}
 		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR, "failed to open %s for backlight "
 			   "control: %s\n", path, strerror(errno));
 		return;
@@ -447,14 +455,6 @@ sna_output_backlight_get_max(xf86OutputPtr output)
 	char path[1024], val[BACKLIGHT_VALUE_LEN];
 	int fd, max = 0;
 
-	/* We are used as an initial check to see if we can
-	 * control the backlight, so first test if we can set values.
-	 */
-	sprintf(path, "%s/%s/brightness",
-		BACKLIGHT_CLASS, sna_output->backlight_iface);
-	if (access(path, R_OK | W_OK))
-		return -1;
-
 	sprintf(path, "%s/%s/max_brightness",
 		BACKLIGHT_CLASS, sna_output->backlight_iface);
 	fd = open(path, O_RDONLY);
@@ -2539,6 +2539,7 @@ sna_output_destroy(xf86OutputPtr output)
 	free(sna_output->prop_ids);
 	free(sna_output->prop_values);
 
+	backlight_helper_cleanup(&sna_output->backlight_helper);
 	free(sna_output->backlight_iface);
 
 	free(sna_output);
diff --git a/src/uxa/intel_display.c b/src/uxa/intel_display.c
index e1b2920..41b391e 100644
--- a/src/uxa/intel_display.c
+++ b/src/uxa/intel_display.c
@@ -43,6 +43,7 @@
 #include "intel.h"
 #include "intel_bufmgr.h"
 #include "intel_options.h"
+#include "backlight.h"
 #include "xf86drm.h"
 #include "xf86drmMode.h"
 #include "X11/Xatom.h"
@@ -123,6 +124,7 @@ struct intel_output {
 	const char *backlight_iface;
 	int backlight_active_level;
 	int backlight_max;
+	struct backlight_helper backlight_helper;
 	xf86OutputPtr output;
 	struct list link;
 };
@@ -245,6 +247,12 @@ intel_output_backlight_set(xf86OutputPtr output, int level)
 		BACKLIGHT_CLASS, intel_output->backlight_iface);
 	fd = open(path, O_RDWR);
 	if (fd == -1) {
+		/* If this is an access error, invoke the helper instead */
+		if (errno == EACCES)  {
+			backlight_helper_set(&intel_output->backlight_helper,
+				output, intel_output->backlight_iface, level);
+			return;
+		}
 		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR, "failed to open %s for backlight "
 			   "control: %s\n", path, strerror(errno));
 		return;
@@ -298,14 +306,6 @@ intel_output_backlight_get_max(xf86OutputPtr output)
 	char path[BACKLIGHT_PATH_LEN], val[BACKLIGHT_VALUE_LEN];
 	int fd, max = 0;
 
-	/* We are used as an initial check to see if we can
-	 * control the backlight, so first test if we can set values.
-	 */
-	sprintf(path, "%s/%s/brightness",
-		BACKLIGHT_CLASS, intel_output->backlight_iface);
-	if (access(path, R_OK | W_OK))
-		return -1;
-
 	sprintf(path, "%s/%s/max_brightness",
 		BACKLIGHT_CLASS, intel_output->backlight_iface);
 	fd = open(path, O_RDONLY);
@@ -1083,6 +1083,7 @@ intel_output_destroy(xf86OutputPtr output)
 	intel_output->mode_output = NULL;
 
 	list_del(&intel_output->link);
+	backlight_helper_cleanup(&intel_output->backlight_helper);
 	free(intel_output);
 
 	output->driver_private = NULL;
-- 
1.8.5.3

Re: [PATCH 1/3] xf86-video-intel: Fix fd_set_nonblock

[Chris Wilson]

On Sat, Feb 15, 2014 at 12:02:35AM +0100, Hans de Goede wrote:
> O_NONBLOCK is a status flag not a descriptor flag, so F_GETFL / F_SETFL should
> be used to modify it.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Oops, that was a silly typo that unfortunately compiled.
Thanks,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] xf86-video-intel: Add a helper for setting backlight without root rights

[Chris Wilson]

On Sat, Feb 15, 2014 at 12:02:37AM +0100, Hans de Goede wrote:
> Once the xserver stops running as root on kms capabable systems, we will need
> some other way to access the backlight.
> 
> The approach taken in this patch leaves most of the heavy lifting (wrt
> doing everything suid root safe) to pkexec, as is done in ie
> gnome-settings-daemon, which controls the backlight directly on ati and
> nouveau cards.
> 
> This commit adds src/backlight.h and src/backlight.c as a place to share common
> backlight code, in the future most of the duplicate backlight code inside
> src/sna/sna_display.c and src/uxa/intel_display.c should be moved there.

Right, I agree and think we can make that transition now. It should not
result in much more code than motion than introducing the helper.
Importantly, I think it will also clarify the tests we require before
declaring the backlight functional.

Is there a reason why the standalone helper is not kept alive? We could
just connect the pipe to the helper's stdin and launch it as a daemon
rather than keeping a child around who's only task is to spawn the
standalone helper everytime?

Otherwise the only other tweak I can see would be to replace the #ifdef
__linux__ with something like #ifdef USE_BACKLIGHT_HELPER
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] xf86-video-intel: Add a helper for setting backlight without root rights

[Hans de Goede]

Hi,

On 02/15/2014 12:54 AM, Chris Wilson wrote:
> On Sat, Feb 15, 2014 at 12:02:37AM +0100, Hans de Goede wrote:
>> Once the xserver stops running as root on kms capabable systems, we will need
>> some other way to access the backlight.
>>
>> The approach taken in this patch leaves most of the heavy lifting (wrt
>> doing everything suid root safe) to pkexec, as is done in ie
>> gnome-settings-daemon, which controls the backlight directly on ati and
>> nouveau cards.
>>
>> This commit adds src/backlight.h and src/backlight.c as a place to share common
>> backlight code, in the future most of the duplicate backlight code inside
>> src/sna/sna_display.c and src/uxa/intel_display.c should be moved there.
>
> Right, I agree and think we can make that transition now. It should not
> result in much more code than motion than introducing the helper.
> Importantly, I think it will also clarify the tests we require before
> declaring the backlight functional.

So you would like me to unify things before adding the helper I assume,
so first a unifying patch introducing backlight.[c,h] and then a patch adding
the helper on top ?

Note I might chicken out wrt the bits for determining which iface to use, at a
first glance those seem significantly different between the sna and uxa code,
but the rest should be easy to unify.

> Is there a reason why the standalone helper is not kept alive? We could
> just connect the pipe to the helper's stdin and launch it as a daemon
> rather than keeping a child around who's only task is to spawn the
> standalone helper everytime?

Hmm, so to be clear what you mean is having the helper take only the iface
on its cmdline, and then read level values from stdin, until it gets EOF
on stdin, yes that should work nicely, I'll change that in the next fd.

> Otherwise the only other tweak I can see would be to replace the #ifdef
> __linux__ with something like #ifdef USE_BACKLIGHT_HELPER

Yes that would be cleaner I'll change that too.

Regards,

Hans

Re: [PATCH 3/3] xf86-video-intel: Add a helper for setting backlight without root rights

[Chris Wilson]

On Sat, Feb 15, 2014 at 09:48:14AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 02/15/2014 12:54 AM, Chris Wilson wrote:
> >On Sat, Feb 15, 2014 at 12:02:37AM +0100, Hans de Goede wrote:
> >>Once the xserver stops running as root on kms capabable systems, we will need
> >>some other way to access the backlight.
> >>
> >>The approach taken in this patch leaves most of the heavy lifting (wrt
> >>doing everything suid root safe) to pkexec, as is done in ie
> >>gnome-settings-daemon, which controls the backlight directly on ati and
> >>nouveau cards.
> >>
> >>This commit adds src/backlight.h and src/backlight.c as a place to share common
> >>backlight code, in the future most of the duplicate backlight code inside
> >>src/sna/sna_display.c and src/uxa/intel_display.c should be moved there.
> >
> >Right, I agree and think we can make that transition now. It should not
> >result in much more code than motion than introducing the helper.
> >Importantly, I think it will also clarify the tests we require before
> >declaring the backlight functional.
> 
> So you would like me to unify things before adding the helper I assume,
> so first a unifying patch introducing backlight.[c,h] and then a patch adding
> the helper on top ?

No, I've just moved some code around in your patch and reduced a little
more duplication. I'm happy with it all now, except for one minor
niggle..

How do you make pkexec work? On my f20, it just hangs in
polkit_authority_check_authorization_sync()

I've verfied that the direct interface works. I've verifed that the
helper works. But pkexec doesn't launch the helper. :(
 
> Note I might chicken out wrt the bits for determining which iface to use, at a
> first glance those seem significantly different between the sna and uxa code,
> but the rest should be easy to unify.
> 
> >Is there a reason why the standalone helper is not kept alive? We could
> >just connect the pipe to the helper's stdin and launch it as a daemon
> >rather than keeping a child around who's only task is to spawn the
> >standalone helper everytime?
> 
> Hmm, so to be clear what you mean is having the helper take only the iface
> on its cmdline, and then read level values from stdin, until it gets EOF
> on stdin, yes that should work nicely, I'll change that in the next fd.

Already done.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] intel: Add a helper for setting backlight without root rights

[Chris Wilson]

From: Hans de Goede <hdegoede@redhat.com>

Once the xserver stops running as root on kms capabable systems, we will need
some other way to access the backlight.

The approach taken in this patch leaves most of the heavy lifting (wrt
doing everything suid root safe) to pkexec, as is done in ie
gnome-settings-daemon, which controls the backlight directly on ati and
nouveau cards.

This commit adds src/backlight.h and src/backlight.c as a place to share common
backlight code, in the future most of the duplicate backlight code inside
src/sna/sna_display.c and src/uxa/intel_display.c should be moved there.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 Makefile.am                                        |   6 +-
 configure.ac                                       |  20 +-
 src/Makefile.am                                    |   2 +
 src/backlight.c                                    | 225 +++++++++++++++++++++
 src/backlight.h                                    |  46 +++++
 src/sna/sna_display.c                              | 194 +++++-------------
 src/uxa/intel_display.c                            | 168 ++++-----------
 tools/Makefile.am                                  |  26 ++-
 tools/backlight_helper.c                           |  56 +++++
 ...g.x.xf86-video-intel.backlight-helper.policy.in |  19 ++
 10 files changed, 467 insertions(+), 295 deletions(-)
 create mode 100644 src/backlight.c
 create mode 100644 src/backlight.h
 create mode 100644 tools/backlight_helper.c
 create mode 100644 tools/org.x.xf86-video-intel.backlight-helper.policy.in

diff --git a/Makefile.am b/Makefile.am
index 71c7698..6bb4854 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -20,11 +20,7 @@
 
 ACLOCAL_AMFLAGS = ${ACLOCAL_FLAGS} -I m4
 
-SUBDIRS = man xvmc src
-
-if BUILD_TOOLS
-SUBDIRS += tools
-endif
+SUBDIRS = man xvmc src tools
 
 MAINTAINERCLEANFILES = ChangeLog INSTALL
 
diff --git a/configure.ac b/configure.ac
index 4f73ba4..c9410c1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -62,6 +62,18 @@ AC_DISABLE_STATIC
 AC_PROG_LIBTOOL
 AC_SYS_LARGEFILE
 
+# Platform specific settings
+case $host_os in
+  *linux*)
+    backlight_helper=yes
+    ;;
+esac
+AM_CONDITIONAL(BUILD_BACKLIGHT_HELPER, [test "x$backlight_helper" = "xyes"])
+if test "x$backlight_helper" = "xyes"; then
+	tools_msg="$tools_msg xf86-video-intel-backlight-helper"
+	AC_DEFINE(USE_BACKLIGHT_HELPER, 1, [Enable use of the backlight helper interfaces])
+fi
+
 # Are we in a git checkout?
 dot_git=no
 if test -e .git; then
@@ -218,6 +230,8 @@ if test "x$tools" = "xyes"; then
       AC_CHECK_HEADERS([X11/extensions/XShm.h X11/extensions/shmproto.h X11/extensions/shmstr.h], [], [tools=no],
 		       [#include <X11/Xlibint.h>
 			#include <X11/Xproto.h>])
+
+      tools_msg="$tools_msg intel-virtual-output"
 fi
 
 AM_CONDITIONAL(BUILD_TOOLS, test "x$tools" = "xyes")
@@ -681,6 +695,7 @@ fi
 DRIVER_NAME="intel"
 AC_SUBST([DRIVER_NAME])
 AC_SUBST([moduledir])
+AC_DEFINE_DIR([PREFIX_PATH], prefix, [installation prefix])
 
 AC_CONFIG_FILES([
                 Makefile
@@ -700,6 +715,7 @@ AC_CONFIG_FILES([
                 xvmc/shader/vld/Makefile
 		test/Makefile
 		tools/Makefile
+		tools/org.x.xf86-video-intel.backlight-helper.policy
 ])
 AC_OUTPUT
 
@@ -730,9 +746,7 @@ if test "x$dri_msg" = "x"; then
 	dri_msg=" none"
 fi
 
-if test "x$tools" = "xyes"; then
-	tools_msg=" intel-virtual-output"
-else
+if test "x$tools_msg" = "x"; then
 	tools_msg=" none"
 fi
 
diff --git a/src/Makefile.am b/src/Makefile.am
index b7ccde6..da75125 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -52,6 +52,8 @@ endif
 NULL:=#
 
 intel_drv_la_SOURCES = \
+	backlight.c \
+	backlight.h \
 	fd.h \
 	fd.c \
 	i915_pciids.h \
diff --git a/src/backlight.c b/src/backlight.c
new file mode 100644
index 0000000..3a30ad1
--- /dev/null
+++ b/src/backlight.c
@@ -0,0 +1,225 @@
+/***************************************************************************
+
+ Copyright 2014 Intel Corporation.  All Rights Reserved.
+
+ Permission is hereby granted, free of charge, to any person obtaining a
+ copy of this software and associated documentation files (the
+ "Software"), to deal in the Software without restriction, including
+ without limitation the rights to use, copy, modify, merge, publish,
+ distribute, sub license, and/or sell copies of the Software, and to
+ permit persons to whom the Software is furnished to do so, subject to
+ the following conditions:
+
+ The above copyright notice and this permission notice (including the
+ next paragraph) shall be included in all copies or substantial portions
+ of the Software.
+
+ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+ OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
+ IN NO EVENT SHALL INTEL, AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
+ DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR
+ THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+ **************************************************************************/
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <sys/stat.h>
+
+#include "backlight.h"
+#include "fd.h"
+
+/* Enough for 10 digits of backlight + '\n' + '\0' */
+#define BACKLIGHT_VALUE_LEN 12
+
+/*
+ * Unfortunately this is not as simple as I would like it to be. If selinux is
+ * dropping dbus messages pkexec may block *forever*.
+ *
+ * Backgrounding pkexec by doing System("pkexec ...&") does not work because
+ * that detaches pkexec from its parent at which point its security checks
+ * fail and it refuses to execute the helper.
+ *
+ * So we're left with spawning a helper child which gets levels to set written
+ * to it through a pipe. This turns the blocking forever problem from a hung
+ * machine problem into a simple backlight control not working problem.
+ */
+
+static int
+__backlight_read(const char *iface, const char *file)
+{
+	char buf[1024];
+	struct stat st;
+	int fd, val;
+
+	snprintf(buf, sizeof(buf), "%s/%s/%s", BACKLIGHT_CLASS, iface, file);
+	fd = open(buf, O_RDONLY);
+	if (fd == -1)
+		return -1;
+
+	if (fstat(fd, &st) == 0 && major(st.st_dev) == 0) {
+		val = read(fd, buf, BACKLIGHT_VALUE_LEN - 1);
+		if (val > 0) {
+			buf[val] = '\0';
+			val = atoi(buf);
+		} else
+			val = -1;
+	} else
+		val = -1;
+	close(fd);
+
+	return val;
+}
+
+int backlight_exists(const char *iface)
+{
+	if (__backlight_read(iface, "brightness") < 0)
+		return 0;
+
+	if (__backlight_read(iface, "max_brightness") <= 0)
+		return 0;
+
+	return 1;
+}
+
+static int __backlight_init(struct backlight *b, char *iface, int fd)
+{
+	b->fd = fd_set_cloexec(fd_set_nonblock(fd));
+	b->iface = iface;
+	return 1;
+}
+
+static int __backlight_direct_init(struct backlight *b, char *iface)
+{
+	struct stat st;
+	char path[1024];
+	int fd;
+
+	snprintf(path, sizeof(path), "%s/%s/brightness", BACKLIGHT_CLASS, iface);
+	fd = open(path, O_RDWR);
+	if (fd < 0 || fstat(fd, &st) || major(st.st_dev))
+		return 0;
+
+	return __backlight_init(b, iface, fd);
+}
+
+static int __backlight_helper_init(struct backlight *b, char *iface)
+{
+#if USE_BACKLIGHT_HELPER
+	int fds[2];
+
+	/* First test to see if pkexec itself is executable */
+	if (system("pkexec --version"))
+		return 0;
+
+	if (pipe(fds))
+		return 0;
+
+	switch ((b->pid = fork())) {
+	case 0:
+		close(fds[1]);
+		dup2(fds[0], 0);
+		close(fds[0]);
+		_exit(execlp("pkexec", "pkexec",
+			     PREFIX_PATH "/libexec/xf86-video-intel-backlight-helper",
+			     iface,
+			     NULL));
+		/* unreachable fallthrough */
+	case -1:
+		close(fds[1]);
+		close(fds[0]);
+		return 0;
+
+	default:
+		close(fds[0]);
+		return __backlight_init(b, iface, fds[1]);
+	}
+#else
+	return 0;
+#endif
+}
+
+int backlight_init(struct backlight *b, char *iface)
+{
+	int level;
+
+	if (iface == NULL)
+		return -1;
+
+	b->max = __backlight_read(iface, "max_brightness");
+	if (b->max <= 0)
+		return -1;
+
+	level = __backlight_read(iface, "brightness");
+	if (level < 0)
+		return -1;
+
+	if (!__backlight_direct_init(b, iface) &&
+	    !__backlight_helper_init(b, iface))
+		return -1;
+
+	return level;
+}
+
+int backlight_set(struct backlight *b, int level)
+{
+	char val[BACKLIGHT_VALUE_LEN];
+	int len, ret = 0;
+
+	if (b->iface == NULL)
+		return 0;
+
+	if ((unsigned)level > b->max)
+		level = b->max;
+
+	len = snprintf(val, BACKLIGHT_VALUE_LEN, "%d\n", level);
+	if (write(b->fd, val, len) != len)
+		ret = -1;
+
+	return ret;
+}
+
+int backlight_get(struct backlight *b)
+{
+	int level;
+
+	if (b->iface == NULL)
+		return -1;
+
+	level = __backlight_read(b->iface, "brightness");
+	if (level > b->max)
+		level = b->max;
+	else if (level < 0)
+		level = -1;
+	return level;
+}
+
+void backlight_disable(struct backlight *b)
+{
+	if (b->iface == NULL)
+		return;
+
+	if (b->fd != -1)
+		close(b->fd);
+
+	free(b->iface);
+	b->iface = NULL;
+}
+
+void backlight_fini(struct backlight *b)
+{
+	backlight_disable(b);
+	if (b->pid)
+		waitpid(b->pid, NULL, 0);
+}
diff --git a/src/backlight.h b/src/backlight.h
new file mode 100644
index 0000000..da61e1b
--- /dev/null
+++ b/src/backlight.h
@@ -0,0 +1,46 @@
+/***************************************************************************
+
+ Copyright 2014 Intel Corporation.  All Rights Reserved.
+
+ Permission is hereby granted, free of charge, to any person obtaining a
+ copy of this software and associated documentation files (the
+ "Software"), to deal in the Software without restriction, including
+ without limitation the rights to use, copy, modify, merge, publish,
+ distribute, sub license, and/or sell copies of the Software, and to
+ permit persons to whom the Software is furnished to do so, subject to
+ the following conditions:
+
+ The above copyright notice and this permission notice (including the
+ next paragraph) shall be included in all copies or substantial portions
+ of the Software.
+
+ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+ OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
+ IN NO EVENT SHALL INTEL, AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
+ DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR
+ THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+ **************************************************************************/
+
+#ifndef BACKLIGHT_H
+#define BACKLIGHT_H
+
+struct backlight {
+	char *iface;
+	int max;
+	int pid, fd;
+};
+
+#define BACKLIGHT_CLASS "/sys/class/backlight"
+
+int backlight_exists(const char *iface);
+
+int backlight_init(struct backlight *backlight, char *iface);
+int backlight_set(struct backlight *backlight, int level);
+int backlight_get(struct backlight *backlight);
+void backlight_disable(struct backlight *backlight);
+void backlight_fini(struct backlight *backlight);
+
+#endif /* BACKLIGHT_H */
diff --git a/src/sna/sna_display.c b/src/sna/sna_display.c
index 636217a..11aa48d 100644
--- a/src/sna/sna_display.c
+++ b/src/sna/sna_display.c
@@ -44,6 +44,7 @@
 #include "sna_reg.h"
 #include "fb/fbpict.h"
 #include "intel_options.h"
+#include "backlight.h"
 
 #include <xf86Crtc.h>
 
@@ -141,9 +142,8 @@ struct sna_output {
 
 	uint32_t dpms_id;
 	int dpms_mode;
-	char *backlight_iface;
+	struct backlight backlight;
 	int backlight_active_level;
-	int backlight_max;
 
 	int num_modes;
 	struct drm_mode_modeinfo *modes;
@@ -186,11 +186,6 @@ static bool sna_mode_wait_for_event(struct sna *sna)
 	return poll(&pfd, 1, -1) == 1;
 }
 
-#define BACKLIGHT_CLASS "/sys/class/backlight"
-
-/* Enough for 10 digits of backlight + '\n' + '\0' */
-#define BACKLIGHT_VALUE_LEN 12
-
 static inline uint32_t fb_id(struct kgem_bo *bo)
 {
 	return bo->delta;
@@ -298,6 +293,10 @@ static void gem_close(int fd, uint32_t handle)
 	(void)drmIoctl(fd, DRM_IOCTL_GEM_CLOSE, &close);
 }
 
+#define BACKLIGHT_NAME             "Backlight"
+#define BACKLIGHT_DEPRECATED_NAME  "BACKLIGHT"
+static Atom backlight_atom, backlight_deprecated_atom;
+
 #ifdef __OpenBSD__
 
 #include <dev/wscons/wsconsio.h>
@@ -310,13 +309,13 @@ sna_output_backlight_set(xf86OutputPtr output, int level)
 	struct wsdisplay_param param;
 
 	DBG(("%s: level=%d, max=%d\n", __FUNCTION__,
-	     level, sna_output->backlight_max));
+	     level, sna_output->backlight.max));
 
-	if (!sna_output->backlight_iface)
+	if (!sna_output->backlight.iface)
 		return;
 
-	if ((unsigned)level > sna_output->backlight_max)
-		level = sna_output->backlight_max;
+	if ((unsigned)level > sna_output->backlight.max)
+		level = sna_output->backlight.max;
 
 	VG_CLEAR(param);
 	param.param = WSDISPLAYIO_PARAM_BRIGHTNESS;
@@ -324,8 +323,12 @@ sna_output_backlight_set(xf86OutputPtr output, int level)
 
 	if (ioctl(xf86Info.consoleFd, WSDISPLAYIO_SETPARAM, &param) == -1) {
 		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
-			   "Failed to set backlight level: %s\n",
+			   "Failed to set backlight level, disabling: %s\n",
 			   strerror(errno));
+		free(sna_output->backlight.iface);
+		sna_output->backlight.iface = NULL;
+		RRDeleteOutputProperty(output->randr_output, backlight_atom);
+		RRDeleteOutputProperty(output->randr_output, backlight_deprecated_atom);
 	}
 }
 
@@ -363,8 +366,9 @@ sna_output_backlight_init(xf86OutputPtr output)
 
 	DBG(("%s: found 'wscons'\n", __FUNCTION__));
 
-	sna_output->backlight_iface = strdup("wscons");
-	sna_output->backlight_max = param.max;
+	sna_output->backlight.iface = strdup("wscons");
+	sna_output->backlight.max = param.max;
+	sna_output->backlight.fd = -1;
 	sna_output->backlight_active_level = param.curval;
 }
 
@@ -374,115 +378,25 @@ static void
 sna_output_backlight_set(xf86OutputPtr output, int level)
 {
 	struct sna_output *sna_output = output->driver_private;
-	char path[1024], val[BACKLIGHT_VALUE_LEN];
-	int fd, len, ret;
-
-	DBG(("%s: level=%d, max=%d\n", __FUNCTION__,
-	     level, sna_output->backlight_max));
-
-	if (!sna_output->backlight_iface)
-		return;
 
-	if ((unsigned)level > sna_output->backlight_max)
-		level = sna_output->backlight_max;
+	DBG(("%s(%s) level=%d, max=%d\n", __FUNCTION__,
+	     output->name, level, sna_output->backlight.max));
 
-	len = snprintf(val, BACKLIGHT_VALUE_LEN, "%d\n", level);
-	sprintf(path, "%s/%s/brightness",
-		BACKLIGHT_CLASS, sna_output->backlight_iface);
-	fd = open(path, O_RDWR);
-	if (fd == -1) {
-		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR, "failed to open %s for backlight "
-			   "control: %s\n", path, strerror(errno));
-		return;
-	}
-
-	ret = write(fd, val, len);
-	if (ret == -1) {
-		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR, "write to %s for backlight "
-			   "control failed: %s\n", path, strerror(errno));
+	if (backlight_set(&sna_output->backlight, level)) {
+		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+			   "failed to set backlight %s for output %s to brightness level %d, disabling\n",
+			   sna_output->backlight.iface, output->name, level);
+		backlight_disable(&sna_output->backlight);
+		RRDeleteOutputProperty(output->randr_output, backlight_atom);
+		RRDeleteOutputProperty(output->randr_output, backlight_deprecated_atom);
 	}
-
-	close(fd);
 }
 
 static int
 sna_output_backlight_get(xf86OutputPtr output)
 {
 	struct sna_output *sna_output = output->driver_private;
-	char path[1024], val[BACKLIGHT_VALUE_LEN];
-	int fd, level;
-
-	sprintf(path, "%s/%s/actual_brightness",
-		BACKLIGHT_CLASS, sna_output->backlight_iface);
-	fd = open(path, O_RDONLY);
-	if (fd == -1) {
-		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR, "failed to open %s "
-			   "for backlight control: %s\n", path, strerror(errno));
-		return -1;
-	}
-
-	memset(val, 0, sizeof(val));
-	if (read(fd, val, BACKLIGHT_VALUE_LEN) == -1) {
-		close(fd);
-		return -1;
-	}
-
-	close(fd);
-
-	level = atoi(val);
-	DBG(("%s: level=%d (max=%d)\n",
-	     __FUNCTION__, level, sna_output->backlight_max));
-
-	if (level > sna_output->backlight_max)
-		level = sna_output->backlight_max;
-	else if (level < 0)
-		level = -1;
-	return level;
-}
-
-static int
-sna_output_backlight_get_max(xf86OutputPtr output)
-{
-	struct sna_output *sna_output = output->driver_private;
-	char path[1024], val[BACKLIGHT_VALUE_LEN];
-	struct stat st;
-	int fd, max = 0;
-
-	/* We are used as an initial check to see if we can
-	 * control the backlight, so first test if we can set values.
-	 */
-	sprintf(path, "%s/%s/brightness",
-		BACKLIGHT_CLASS, sna_output->backlight_iface);
-	if (access(path, R_OK | W_OK))
-		return -1;
-
-	if (stat(path, &st))
-		return -1;
-
-	if (major(st.st_dev)) /* is this a kernel psuedo filesystem? */
-		return -1;
-
-	sprintf(path, "%s/%s/max_brightness",
-		BACKLIGHT_CLASS, sna_output->backlight_iface);
-	fd = open(path, O_RDONLY);
-	if (fd == -1) {
-		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR, "failed to open %s "
-			   "for backlight control: %s\n", path, strerror(errno));
-		return -1;
-	}
-
-	memset(val, 0, sizeof(val));
-	if (read(fd, val, BACKLIGHT_VALUE_LEN) == -1) {
-		close(fd);
-		return -1;
-	}
-
-	close(fd);
-
-	max = atoi(val);
-	if (max <= 0)
-		max = -1;
-	return max;
+	return backlight_get(&sna_output->backlight);
 }
 
 enum {
@@ -495,19 +409,14 @@ enum {
 static char *
 has_user_backlight_override(xf86OutputPtr output)
 {
-	struct sna_output *sna_output = output->driver_private;
 	struct sna *sna = to_sna(output->scrn);
 	const char *str;
-	int max;
 
 	str = xf86GetOptValString(sna->Options, OPTION_BACKLIGHT);
 	if (str == NULL)
 		return NULL;
 
-	sna_output->backlight_iface = (char *)str;
-	max = sna_output_backlight_get_max(output);
-	sna_output->backlight_iface = NULL;
-	if (max <= 0) {
+	if (!backlight_exists(str)) {
 		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
 			   "unrecognised backlight control interface '%s'\n",
 			   str);
@@ -520,7 +429,6 @@ has_user_backlight_override(xf86OutputPtr output)
 static char *
 has_device_backlight(xf86OutputPtr output, int *best_type)
 {
-	struct sna_output *sna_output = output->driver_private;
 	struct sna *sna = to_sna(output->scrn);
 	struct pci_device *pci;
 	char path[1024];
@@ -576,12 +484,8 @@ has_device_backlight(xf86OutputPtr output, int *best_type)
 
 		if (v < *best_type) {
 			char *copy;
-			int max;
 
-			sna_output->backlight_iface = de->d_name;
-			max = sna_output_backlight_get_max(output);
-			sna_output->backlight_iface = NULL;
-			if (max <= 0)
+			if (!backlight_exists(de->d_name))
 				continue;
 
 			copy = strdup(de->d_name);
@@ -615,7 +519,6 @@ has_backlight(xf86OutputPtr output, int *best_type)
 		"acpi_video0",
 		"intel_backlight",
 	};
-	struct sna_output *sna_output = output->driver_private;
 	char *best_iface = NULL;
 	DIR *dir;
 	struct dirent *de;
@@ -669,14 +572,10 @@ has_backlight(xf86OutputPtr output, int *best_type)
 
 		if (v < *best_type) {
 			char *copy;
-			int max;
 
 			/* XXX detect right backlight for multi-GPU/panels */
 
-			sna_output->backlight_iface = de->d_name;
-			max = sna_output_backlight_get_max(output);
-			sna_output->backlight_iface = NULL;
-			if (max <= 0)
+			if (!backlight_exists(de->d_name))
 				continue;
 
 			copy = strdup(de->d_name);
@@ -716,9 +615,11 @@ sna_output_backlight_init(xf86OutputPtr output)
 	return;
 
 done:
-	sna_output->backlight_iface = best_iface;
-	sna_output->backlight_max = sna_output_backlight_get_max(output);
-	sna_output->backlight_active_level = sna_output_backlight_get(output);
+	sna_output->backlight_active_level =
+		backlight_init(&sna_output->backlight, best_iface);
+	if (sna_output->backlight_active_level < 0)
+		return;
+
 	switch (best_type) {
 	case INT_MAX: best_iface = (char *)"user"; from = X_CONFIG; break;
 	case FIRMWARE: best_iface = (char *)"firmware"; break;
@@ -728,7 +629,7 @@ done:
 	}
 	xf86DrvMsg(output->scrn->scrnIndex, from,
 		   "found backlight control interface %s (type '%s')\n",
-		   sna_output->backlight_iface, best_iface);
+		   sna_output->backlight.iface, best_iface);
 }
 #endif
 
@@ -2585,7 +2486,7 @@ sna_output_destroy(xf86OutputPtr output)
 	free(sna_output->prop_ids);
 	free(sna_output->prop_values);
 
-	free(sna_output->backlight_iface);
+	backlight_fini(&sna_output->backlight);
 
 	free(sna_output);
 	output->driver_private = NULL;
@@ -2596,7 +2497,7 @@ sna_output_dpms_backlight(xf86OutputPtr output, int oldmode, int mode)
 {
 	struct sna_output *sna_output = output->driver_private;
 
-	if (!sna_output->backlight_iface)
+	if (!sna_output->backlight.iface)
 		return;
 
 	DBG(("%s(%s) -- %d -> %d\n", __FUNCTION__, output->name, oldmode, mode));
@@ -2700,10 +2601,6 @@ sna_output_create_ranged_atom(xf86OutputPtr output, Atom *atom,
 			   "RRChangeOutputProperty error, %d\n", err);
 }
 
-#define BACKLIGHT_NAME             "Backlight"
-#define BACKLIGHT_DEPRECATED_NAME  "BACKLIGHT"
-static Atom backlight_atom, backlight_deprecated_atom;
-
 static void
 sna_output_create_resources(xf86OutputPtr output)
 {
@@ -2775,20 +2672,20 @@ sna_output_create_resources(xf86OutputPtr output)
 		}
 	}
 
-	if (sna_output->backlight_iface) {
+	if (sna_output->backlight.iface) {
 		/* Set up the backlight property, which takes effect
 		 * immediately and accepts values only within the
 		 * backlight_range.
 		 */
 		sna_output_create_ranged_atom(output, &backlight_atom,
 					      BACKLIGHT_NAME, 0,
-					      sna_output->backlight_max,
+					      sna_output->backlight.max,
 					      sna_output->backlight_active_level,
 					      FALSE);
 		sna_output_create_ranged_atom(output,
 					      &backlight_deprecated_atom,
 					      BACKLIGHT_DEPRECATED_NAME, 0,
-					      sna_output->backlight_max,
+					      sna_output->backlight.max,
 					      sna_output->backlight_active_level,
 					      FALSE);
 	}
@@ -2813,8 +2710,8 @@ sna_output_set_property(xf86OutputPtr output, Atom property,
 
 		val = *(INT32 *)value->data;
 		DBG(("%s: setting backlight to %d (max=%d)\n",
-		     __FUNCTION__, (int)val, sna_output->backlight_max));
-		if (val < 0 || val > sna_output->backlight_max)
+		     __FUNCTION__, (int)val, sna_output->backlight.max));
+		if (val < 0 || val > sna_output->backlight.max)
 			return FALSE;
 
 		if (sna_output->dpms_mode == DPMSModeOn)
@@ -2880,7 +2777,7 @@ sna_output_get_property(xf86OutputPtr output, Atom property)
 	if (property == backlight_atom || property == backlight_deprecated_atom) {
 		INT32 val;
 
-		if (!sna_output->backlight_iface)
+		if (!sna_output->backlight.iface)
 			return FALSE;
 
 		val = sna_output_backlight_get(output);
@@ -2890,7 +2787,6 @@ sna_output_get_property(xf86OutputPtr output, Atom property)
 		err = RRChangeOutputProperty(output->randr_output, property,
 					     XA_INTEGER, 32, PropModeReplace, 1, &val,
 					     FALSE, FALSE);
-
 		if (err != 0) {
 			xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
 				   "RRChangeOutputProperty error, %d\n", err);
diff --git a/src/uxa/intel_display.c b/src/uxa/intel_display.c
index e1b2920..98e877d 100644
--- a/src/uxa/intel_display.c
+++ b/src/uxa/intel_display.c
@@ -43,6 +43,7 @@
 #include "intel.h"
 #include "intel_bufmgr.h"
 #include "intel_options.h"
+#include "backlight.h"
 #include "xf86drm.h"
 #include "xf86drmMode.h"
 #include "X11/Xatom.h"
@@ -120,9 +121,8 @@ struct intel_output {
 	int panel_vdisplay;
 
 	int dpms_mode;
-	const char *backlight_iface;
+	struct backlight backlight;
 	int backlight_active_level;
-	int backlight_max;
 	xf86OutputPtr output;
 	struct list link;
 };
@@ -150,9 +150,9 @@ intel_output_backlight_set(xf86OutputPtr output, int level)
 	struct intel_output *intel_output = output->driver_private;
 	struct wsdisplay_param param;
 
-	if (level > intel_output->backlight_max)
-		level = intel_output->backlight_max;
-	if (! intel_output->backlight_iface || level < 0)
+	if (level > intel_output->backlight.max)
+		level = intel_output->backlight.max;
+	if (! intel_output->backlight.iface || level < 0)
 		return;
 
 	param.param = WSDISPLAYIO_PARAM_BRIGHTNESS;
@@ -188,19 +188,18 @@ intel_output_backlight_init(xf86OutputPtr output)
 
 	param.param = WSDISPLAYIO_PARAM_BRIGHTNESS;
 	if (ioctl(xf86Info.consoleFd, WSDISPLAYIO_GETPARAM, &param) == -1) {
-		intel_output->backlight_iface = NULL;
+		intel_output->backlight.iface = NULL;
 		return;
 	}
 
-	intel_output->backlight_iface = "wscons";
-	intel_output->backlight_max = param.max;
+	intel_output->backlight.iface = strdup("wscons");
+	intel_output->backlight.max = param.max;
+	intel_output->backlight.fd = -1;
 	intel_output->backlight_active_level = param.curval;
 }
 
 #else
 
-#define BACKLIGHT_CLASS "/sys/class/backlight"
-
 /*
  * List of available kernel interfaces in priority order
  */
@@ -220,113 +219,24 @@ static const char *backlight_interfaces[] = {
 	"intel_backlight",
 	NULL,
 };
-/*
- * Must be long enough for BACKLIGHT_CLASS + '/' + longest in above table +
- * '/' + "max_backlight"
- */
-#define BACKLIGHT_PATH_LEN 80
-/* Enough for 10 digits of backlight + '\n' + '\0' */
-#define BACKLIGHT_VALUE_LEN 12
 
 static void
 intel_output_backlight_set(xf86OutputPtr output, int level)
 {
 	struct intel_output *intel_output = output->driver_private;
-	char path[BACKLIGHT_PATH_LEN], val[BACKLIGHT_VALUE_LEN];
-	int fd, len, ret;
-
-	if (level > intel_output->backlight_max)
-		level = intel_output->backlight_max;
-	if (! intel_output->backlight_iface || level < 0)
-		return;
-
-	len = snprintf(val, BACKLIGHT_VALUE_LEN, "%d\n", level);
-	sprintf(path, "%s/%s/brightness",
-		BACKLIGHT_CLASS, intel_output->backlight_iface);
-	fd = open(path, O_RDWR);
-	if (fd == -1) {
-		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR, "failed to open %s for backlight "
-			   "control: %s\n", path, strerror(errno));
-		return;
-	}
-
-	ret = write(fd, val, len);
-	if (ret == -1) {
-		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR, "write to %s for backlight "
-			   "control failed: %s\n", path, strerror(errno));
+	if (backlight_set(&intel_output->backlight, level) < 0) {
+		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
+			   "failed to set backlight %s to brightness level %d, disabling\n",
+			   intel_output->backlight.iface, level);
+		backlight_disable(&intel_output->backlight);
 	}
-
-	close(fd);
 }
 
 static int
 intel_output_backlight_get(xf86OutputPtr output)
 {
 	struct intel_output *intel_output = output->driver_private;
-	char path[BACKLIGHT_PATH_LEN], val[BACKLIGHT_VALUE_LEN];
-	int fd, level;
-
-	sprintf(path, "%s/%s/actual_brightness",
-		BACKLIGHT_CLASS, intel_output->backlight_iface);
-	fd = open(path, O_RDONLY);
-	if (fd == -1) {
-		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR, "failed to open %s "
-			   "for backlight control: %s\n", path, strerror(errno));
-		return -1;
-	}
-
-	memset(val, 0, sizeof(val));
-	if (read(fd, val, BACKLIGHT_VALUE_LEN) == -1) {
-		close(fd);
-		return -1;
-	}
-
-	close(fd);
-
-	level = atoi(val);
-	if (level > intel_output->backlight_max)
-		level = intel_output->backlight_max;
-	if (level < 0)
-		level = -1;
-	return level;
-}
-
-static int
-intel_output_backlight_get_max(xf86OutputPtr output)
-{
-	struct intel_output *intel_output = output->driver_private;
-	char path[BACKLIGHT_PATH_LEN], val[BACKLIGHT_VALUE_LEN];
-	int fd, max = 0;
-
-	/* We are used as an initial check to see if we can
-	 * control the backlight, so first test if we can set values.
-	 */
-	sprintf(path, "%s/%s/brightness",
-		BACKLIGHT_CLASS, intel_output->backlight_iface);
-	if (access(path, R_OK | W_OK))
-		return -1;
-
-	sprintf(path, "%s/%s/max_brightness",
-		BACKLIGHT_CLASS, intel_output->backlight_iface);
-	fd = open(path, O_RDONLY);
-	if (fd == -1) {
-		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR, "failed to open %s "
-			   "for backlight control: %s\n", path, strerror(errno));
-		return -1;
-	}
-
-	memset(val, 0, sizeof(val));
-	if (read(fd, val, BACKLIGHT_VALUE_LEN) == -1) {
-		close(fd);
-		return -1;
-	}
-
-	close(fd);
-
-	max = atoi(val);
-	if (max <= 0)
-		max = -1;
-	return max;
+	return backlight_get(&intel_output->backlight);
 }
 
 static void
@@ -334,42 +244,31 @@ intel_output_backlight_init(xf86OutputPtr output)
 {
 	struct intel_output *intel_output = output->driver_private;
 	intel_screen_private *intel = intel_get_screen_private(output->scrn);
-	char path[BACKLIGHT_PATH_LEN];
-	struct stat buf;
 	char *str;
 	int i;
 
 	str = xf86GetOptValString(intel->Options, OPTION_BACKLIGHT);
 	if (str != NULL) {
-		sprintf(path, "%s/%s", BACKLIGHT_CLASS, str);
-		if (!stat(path, &buf)) {
-			intel_output->backlight_iface = str;
-			intel_output->backlight_max = intel_output_backlight_get_max(output);
-			if (intel_output->backlight_max > 0) {
-				intel_output->backlight_active_level = intel_output_backlight_get(output);
-				xf86DrvMsg(output->scrn->scrnIndex, X_CONFIG,
-					   "found backlight control interface %s\n", path);
-				return;
-			}
+		if (backlight_exists(str)) {
+			xf86DrvMsg(output->scrn->scrnIndex, X_CONFIG,
+				   "found backlight control interface %s\n", str);
+			intel_output->backlight_active_level =
+				backlight_init(&intel_output->backlight, strdup(str));
+			return;
 		}
 		xf86DrvMsg(output->scrn->scrnIndex, X_ERROR,
 			   "unrecognised backlight control interface %s\n", str);
 	}
 
 	for (i = 0; backlight_interfaces[i] != NULL; i++) {
-		sprintf(path, "%s/%s", BACKLIGHT_CLASS, backlight_interfaces[i]);
-		if (!stat(path, &buf)) {
-			intel_output->backlight_iface = backlight_interfaces[i];
-			intel_output->backlight_max = intel_output_backlight_get_max(output);
-			if (intel_output->backlight_max > 0) {
-				intel_output->backlight_active_level = intel_output_backlight_get(output);
-				xf86DrvMsg(output->scrn->scrnIndex, X_PROBED,
-					   "found backlight control interface %s\n", path);
-				return;
-			}
+		if (backlight_exists(backlight_interfaces[i])) {
+			xf86DrvMsg(output->scrn->scrnIndex, X_PROBED,
+				   "found backlight control interface %s\n", backlight_interfaces[i]);
+			intel_output->backlight_active_level =
+				backlight_init(&intel_output->backlight, strdup(backlight_interfaces[i]));
+			return;
 		}
 	}
-	intel_output->backlight_iface = NULL;
 }
 
 #endif
@@ -1083,6 +982,7 @@ intel_output_destroy(xf86OutputPtr output)
 	intel_output->mode_output = NULL;
 
 	list_del(&intel_output->link);
+	backlight_fini(&intel_output->backlight);
 	free(intel_output);
 
 	output->driver_private = NULL;
@@ -1093,7 +993,7 @@ intel_output_dpms_backlight(xf86OutputPtr output, int oldmode, int mode)
 {
 	struct intel_output *intel_output = output->driver_private;
 
-	if (!intel_output->backlight_iface)
+	if (!intel_output->backlight.iface)
 		return;
 
 	if (mode == DPMSModeOn) {
@@ -1286,20 +1186,20 @@ intel_output_create_resources(xf86OutputPtr output)
 		}
 	}
 
-	if (intel_output->backlight_iface) {
+	if (intel_output->backlight.iface) {
 		/* Set up the backlight property, which takes effect
 		 * immediately and accepts values only within the
 		 * backlight_range.
 		 */
 		intel_output_create_ranged_atom(output, &backlight_atom,
 					BACKLIGHT_NAME, 0,
-					intel_output->backlight_max,
+					intel_output->backlight.max,
 					intel_output->backlight_active_level,
 					FALSE);
 		intel_output_create_ranged_atom(output,
 					&backlight_deprecated_atom,
 					BACKLIGHT_DEPRECATED_NAME, 0,
-					intel_output->backlight_max,
+					intel_output->backlight.max,
 					intel_output->backlight_active_level,
 					FALSE);
 	}
@@ -1323,7 +1223,7 @@ intel_output_set_property(xf86OutputPtr output, Atom property,
 		}
 
 		val = *(INT32 *)value->data;
-		if (val < 0 || val > intel_output->backlight_max)
+		if (val < 0 || val > intel_output->backlight.max)
 			return FALSE;
 
 		if (intel_output->dpms_mode == DPMSModeOn)
@@ -1389,7 +1289,7 @@ intel_output_get_property(xf86OutputPtr output, Atom property)
 	if (property == backlight_atom || property == backlight_deprecated_atom) {
 		INT32 val;
 
-		if (! intel_output->backlight_iface)
+		if (!intel_output->backlight.iface)
 			return FALSE;
 
 		val = intel_output_backlight_get(output);
diff --git a/tools/Makefile.am b/tools/Makefile.am
index d294e2a..c168e27 100644
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -20,23 +20,41 @@
 
 AM_CFLAGS = \
 	@CWARNFLAGS@ \
-	$(TOOL_CFLAGS) \
 	@NOWARNFLAGS@ \
 	$(NULL)
 
 drivermandir = $(DRIVER_MAN_DIR)
+backlight_helperdir = $(prefix)/libexec
+policydir = $(datarootdir)/polkit-1/actions
 
+if BUILD_TOOLS
 bin_PROGRAMS = intel-virtual-output
 driverman_DATA = intel-virtual-output.$(DRIVER_MAN_SUFFIX)
+endif
+
+if BUILD_BACKLIGHT_HELPER
+backlight_helper_PROGRAMS = xf86-video-intel-backlight-helper
+nodist_policy_DATA = org.x.xf86-video-intel.backlight-helper.policy
+endif
 
+intel_virtual_output_CFLAGS = \
+	@CWARNFLAGS@ \
+	$(TOOL_CFLAGS) \
+	@NOWARNFLAGS@ \
+	$(NULL)
 intel_virtual_output_SOURCES = \
 	virtual.c \
 	$(NULL)
+intel_virtual_output_LDADD = \
+	$(TOOL_LIBS) \
+	$(NULL)
 
-intel_virtual_output_LDADD = $(TOOL_LIBS)
+xf86_video_intel_backlight_helper_SOURCES = \
+	backlight_helper.c \
+	$(NULL)
 
-EXTRA_DIST = intel-virtual-output.man
-CLEANFILES = $(driverman_DATA)
+EXTRA_DIST = intel-virtual-output.man org.x.xf86-video-intel.backlight-helper.policy.in
+CLEANFILES = $(driverman_DATA) $(nodist_policy_DATA)
 
 # String replacements in MAN_SUBSTS now come from xorg-macros.m4 via configure
 SUFFIXES = .$(DRIVER_MAN_SUFFIX) .man
diff --git a/tools/backlight_helper.c b/tools/backlight_helper.c
new file mode 100644
index 0000000..e5312f9
--- /dev/null
+++ b/tools/backlight_helper.c
@@ -0,0 +1,56 @@
+#include <stdio.h>
+#include <string.h>
+#include <fcntl.h>
+#include <unistd.h>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#define BUF_SIZE 1024
+
+int main(int argc, char *argv[])
+{
+	struct stat st;
+	char buf[BUF_SIZE], *b = buf;
+	int len, fd;
+
+	if (argc == 1) /* called with no arguments to probe existence */
+		return 0;
+
+	if (argc != 2) {
+		fprintf(stderr, "Usage: %s <iface>\n", argv[0]);
+		return 1;
+	}
+
+	snprintf(buf, BUF_SIZE, "/sys/class/backlight/%s/brightness", argv[1]);
+	fd = open(buf, O_RDWR);
+	if (fd < 0 || fstat(fd, &st) || major(st.st_dev)) {
+		fprintf(stderr, "Cannot access backlight interface '%s'\n", argv[1]);
+		return 1;
+	}
+
+	while ((len = read(0, b, sizeof(buf) - (b - buf) - 1)) > 0) {
+		len += b - buf;
+		buf[len] = '\0';
+
+		b = buf;
+		do {
+			char *end = strchr(b, '\n');
+			if (end == NULL)
+				break;
+
+			++end;
+			if (write(fd, b, end - b) != end - b) {
+				fprintf(stderr, "Failed to update backlight interface '%s'\n", argv[1]);
+				return 2;
+			}
+
+			b = end;
+		} while (1);
+
+		memmove(buf, b, len = buf + len - b);
+		b = buf + len;
+	}
+
+	return 0;
+}
diff --git a/tools/org.x.xf86-video-intel.backlight-helper.policy.in b/tools/org.x.xf86-video-intel.backlight-helper.policy.in
new file mode 100644
index 0000000..37e9622
--- /dev/null
+++ b/tools/org.x.xf86-video-intel.backlight-helper.policy.in
@@ -0,0 +1,19 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE policyconfig PUBLIC
+ "-//freedesktop//DTD PolicyKit Policy Configuration 1.0//EN"
+ "http://www.freedesktop.org/standards/PolicyKit/1.0/policyconfig.dtd">
+<policyconfig>
+  <vendor>The X.Org project</vendor>
+  <vendor_url>https://01.org/linuxgraphics/community/xf86-video-intel</vendor_url>
+  <icon_name>brightness</icon_name>
+  <action id="org.x.xf86-video-intel.backlight-helper">
+    <description>Modify lcd panel brightness</description>
+    <message>Authentication is required to modify the lcd panel brightness</message>
+    <defaults>
+      <allow_any>no</allow_any>
+      <allow_inactive>no</allow_inactive>
+      <allow_active>yes</allow_active>
+    </defaults>
+    <annotate key="org.freedesktop.policykit.exec.path">@prefix@/libexec/xf86-video-intel-backlight-helper</annotate>
+  </action>
+</policyconfig>
-- 
1.9.0.rc3

Re: [PATCH] intel: Add a helper for setting backlight without root rights

[Chris Wilson]

On Sat, Feb 15, 2014 at 11:53:30AM +0000, Chris Wilson wrote:
> From: Hans de Goede <hdegoede@redhat.com>
> 
> Once the xserver stops running as root on kms capabable systems, we will need
> some other way to access the backlight.
> 
> The approach taken in this patch leaves most of the heavy lifting (wrt
> doing everything suid root safe) to pkexec, as is done in ie
> gnome-settings-daemon, which controls the backlight directly on ati and
> nouveau cards.
> 
> This commit adds src/backlight.h and src/backlight.c as a place to share common
> backlight code, in the future most of the duplicate backlight code inside
> src/sna/sna_display.c and src/uxa/intel_display.c should be moved there.

Spent some more time bikeshedding, satisfying myself that it all should
be working properly, e.g. (echo 15 ; sleep 2; echo 4; sleep 1; echo -e
"20\n10"; sleep 1; echo -n 1; sleep 1; echo 5) | pkexec /usr/libexec/xf86-video-intel-backlight-helper
works and so decided that it was just pkexec not behaving as expected
during Xorg launch.

Overall is was nice improvement, thanks a lot.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] xf86-video-intel: Add a helper for setting backlight without root rights

[Hans de Goede]

Hi,

On 02/15/2014 12:52 PM, Chris Wilson wrote:
> On Sat, Feb 15, 2014 at 09:48:14AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 02/15/2014 12:54 AM, Chris Wilson wrote:
>>> On Sat, Feb 15, 2014 at 12:02:37AM +0100, Hans de Goede wrote:
>>>> Once the xserver stops running as root on kms capabable systems, we will need
>>>> some other way to access the backlight.
>>>>
>>>> The approach taken in this patch leaves most of the heavy lifting (wrt
>>>> doing everything suid root safe) to pkexec, as is done in ie
>>>> gnome-settings-daemon, which controls the backlight directly on ati and
>>>> nouveau cards.
>>>>
>>>> This commit adds src/backlight.h and src/backlight.c as a place to share common
>>>> backlight code, in the future most of the duplicate backlight code inside
>>>> src/sna/sna_display.c and src/uxa/intel_display.c should be moved there.
>>>
>>> Right, I agree and think we can make that transition now. It should not
>>> result in much more code than motion than introducing the helper.
>>> Importantly, I think it will also clarify the tests we require before
>>> declaring the backlight functional.
>>
>> So you would like me to unify things before adding the helper I assume,
>> so first a unifying patch introducing backlight.[c,h] and then a patch adding
>> the helper on top ?
> 
> No, I've just moved some code around in your patch and reduced a little
> more duplication. I'm happy with it all now, except for one minor
> niggle..

Nice, thanks for working on this! I'll run some tests with your modified code
coming Monday to ensure that everything works as it should with pkexec and
the xserver running as a regular user.

> How do you make pkexec work? On my f20, it just hangs in
> polkit_authority_check_authorization_sync()

Do you have selinux in enforcing mode ? That is know to cause this issue.
I need to file selinux policy bugs for both this as well as for Xorg's inability
to talk to systemd-logind when selinux is in enforcing mode. Doing:
setenforce 0 as root may help.

Regards,

Hans