Hi, On 4/13/22 10:32, Daniel Vetter wrote: > On Fri, Apr 08, 2022 at 12:26:24PM +0200, Hans de Goede wrote: >> Hi, >> >> On 4/8/22 12:16, Simon Ser wrote: >>> Would it be an option to only support the KMS prop for Good devices, >>> and continue using the suboptimal existing sysfs API for Bad devices? >>> >>> (I'm just throwing ideas around to see what sticks, feel free to ignore.) >> >> Currently suid-root or pkexec helpers are used to deal with the >> /sys/class/backlight requires root rights issue. I really want to >> be able to disable these helpers at build time in e.g. GNOME once >> the new properties are supported in GNOME. So that distros with >> a new enough kernel can reduce their attack surface this way. > > Yeah but otoh perpetuating a bad interface forever isn't a great idea > either. I think the pragmatic plan here is > - Implement this properly on good devices, i.e. anything new. > - Do some runtime disabling in the pkexec helpers if they detect a modern > system (we should be able to put a proper symlink into the drm sysfs > connector directories, to make this easy to detect). It's not as great > as doing this at compile time, but it's a step. > - Figure out a solution for the old crap. We can't really change anything > with the load ordering for existing systems, so if the hacked-up compat > libbacklight-backlight isn't an option then I guess we need some quirk > list/extracted code which makes i915/nouveau/radeon driver load fail > until the right backlight shows up. And that needs to be behind a > Kconfig to avoid breaking existing systems. > > Inflicting hotplug complications on all userspace (including uevent > handling for this hotpluggable backlight and everything) just because > fixing the old crap systems is work is imo really not a good idea. Much > better if we get to the correct future step-by-step. This assumes that we only use the new brightness properties for laptop internal LCD panels. But what about controlling the brightness of external monitors through DDC/CI? If we do that we need hotplug support for this regardless since external monitors can be hotplugged. As I mentioned in other parts of the thread one of the reasons why I've started looking into this again is because of people being interested in this (1). And even just taking internal LCD panels into account, there are hybrid GPU laptops where the backlight is directly controlled by the GPU (native type backlight driver) connected to the panel(2), if we runtime switch the GPU attached to the panel there, then we will get an actual hotplug of the LCD connector and I'm not sure if we can always detect the maximum value of the brightness on the GPU which is not connected to the panel at boot. So in this case we need userspace to support re-reading the brightness max at a hotplug event regardless. So in all in all I feel that supporting hotplug events is unavoidable here. Regards, Hans 1) Including attempting to do this through the old /sys/class/backlight interface which IMHO would be quite bad to do: https://lore.kernel.org/lkml/20220403230850.2986-1-yusisamerican@xxxxxxxxx/ 2) E.g. gnome-settings-daemon has special code to detect which native backlight interface to use if there are 2 native backlight devices on a single laptop, see: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/blob/master/plugins/power/gsd-backlight.c#L95
