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

[Hans de Goede <hdegoede@redhat.com>]

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