[Intel-gfx] [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