Re: [RFC] drm/kms: control display brightness through drm_connector properties
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. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [RFC] drm/kms: control display brightness through drm_connector properties
On Wednesday, April 13th, 2022 at 10:32, Daniel Vetter wrote: > 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. Yup, I fully agree. As a user-space dev I'm perfectly fine with an API only available on some systems as a first step.
Re: [RFC] drm/kms: control display brightness through drm_connector properties
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-yusisameri...@gmail.com/ 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
[RFC] drm/kms: Stop registering multiple /sys/class/backlight devs for a single display
Hi All, As discussed in the "[RFC] drm/kms: control display brightness through drm_connector properties" thread, step 1 of the plan is to stop registering multiple /sys/class/backlight devs for a single display. On x86 there can be multiple firmware + direct-hw-access methods for controlling the backlight and in some cases the kernel registers multiple backlight-devices for a single internal laptop LCD panel, 2 common scenarios where this happens are: 1) i915 and nouveau unconditionally register their "native" backlight dev even on devices where /sys/class/backlight/acpi_video0 must be used to control the backlight, relying on userspace to prefer the "firmware" acpi_video0 device over "native" devices. 2) amdgpu and nouveau rely on the acpi_video driver initializing before them, which currently causes /sys/class/backlight/acpi_video0 to usually show up and then they register their own native backlight driver after which the drivers/acpi/video_detect.c code unregisters the acpi_video0 device. This means that userspace briefly sees 2 devices and the disappearing of acpi_video0 after a brief time confuses the systemd backlight level save/restore code, see e.g.: https://bbs.archlinux.org/viewtopic.php?id=269920 Fixing kms driver unconditionally register their "native" backlight dev === The plan for fixing 1) is to add a "bool native_backlight_available" parameter to acpi_video_get_backlight_type(), which will be set to true when called by e.g. the i915 code to check if it should register its native backlight-device. This way acpi_video_get_backlight_type() will know that a native backlight-device is (will be) available even though it has not been registered yet and then it can return acpi_backlight_native if that is the best option. And then the i915 code will only actually register its native backlight when acpi_backlight_native gets returned, thus hiding it in scenario 1. Fixing acpi_video0 getting registered for a brief time == ATM the acpi_video code will delay parsing the ACPI video extensions when an i915 opregion is present and will immediately parse these + register an acpi_video backlight device on laptops without Intel integrated graphics. On laptops with i915 gfx the i915 driver calls acpi_video_register() near the end of its probe() function when things are ready for the acpi_video code to run, avoiding scenario 2. Where as on systems without i915 gfx acpi_video's module_init() immediately calls acpi_video_register() and then later the ACPI video_detect code calls acpi_video_unregister_backlight() to hide the acpi_video# backlight-device on systems where the native backlight-device should be used. The plan to fix this is to add an acpi_video_register_backlight() and to make acpi_video_register() do all the usual ACPI video extension probing, but have it skip the actual registering of the backlight devices and have drivers explicitly call acpi_video_register() after they have setup their own native backlight-device. This way acpi_video_get_backlight_type() already will know that a native backlight-device is available (and preferred) when acpi_video_register_backlight() runs and the registering of the acpi_video# device will then be skipped, removing it briefly showing up and disappearing again. One problem with this approach is that this relies on the GPU driver to call acpi_video_register_backlight() when it is done. One could argue that this is actually a feature, we have had issues with some desktops where acpi_video falsely registers its backlight (even though there is no internal LCD panel), but this will likely cause issues on some systems (1). So the plan is to queue a delayed work with an 8 second (1) delay from acpi_video_register() and have that register the backlight-device if not done already. Other issues The above only takes native vs acpi_video backlight issues into account, there are also a couple of other scenarios which may lead to multiple backlight-devices getting registered: 1) Apple laptops using the apple_gmux driver 2) Nvidia laptops using the (new) nvidia-wmi-ec-backlight driver 3) drivers/platform/x86 drivers calling acpi_video_set_dmi_backlight_type() to override the normal acpi_video_get_backlight_type() heuristics after the native/acpi_video drivers have already loaded The plan for 1) + 2) is to extend the acpi_backlight_type enum with acpi_backlight_gmux and acpi_backlight_nvidia_wmi_ec values and to add detection of these 2 to acpi_video_get_backlight_type(). The plan for 3) is to move the DMI quirks from drivers/platform/x86 drivers which call acpi_video_set_dmi_backlight_type() in to the existing DMI quirk table in drivers/acpi/video_detect.c, so that they will be available during the first/every call of acpi_video_get_backlight_type() and then remove acpi_video_set_dmi_backlight_t
