[RFC v2] drm/kms: control display brightness through drm_connector properties
Hi all, Here is v2 of my "drm/kms: control display brightness through drm_connector properties" RFC: Changes from version 1: - Drop bl_brightness_0_is_min_brightness from list of new connector properties. - Clearly define that 0 is always min-brightness when setting the brightness through the connector properties. - Drop bl_brightness_control_method from list of new connector properties. - Phase 1 of the plan has been completed As discussed already several times in the past: https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/ https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b0...@linux.intel.com/ The current userspace API for brightness control offered by /sys/class/backlight devices has various issues: 1. There is no way to map the backlight device to a specific display-output / panel (1) 2. Controlling the brightness requires root-rights requiring desktop-environments to use suid-root helpers for this. 3. The meaning of 0 is not clearly defined, it can be either off, or minimum brightness at which the display is still readable (in a low light environment) 4. It's not possible to change both the gamma and the brightness in the same KMS atomic commit. You'd want to be able to reduce brightness to conserve power, and counter the effects of that by changing gamma to reach a visually similar image. And you'd want to have the changes take effect at the same time instead of reducing brightness at some frame and change gamma at some other frame. This is pretty much impossible to do via the sysfs interface. As already discussed on various conference's hallway tracks and as has been proposed on the dri-devel list once before (2), it seems that there is consensus that the best way to to solve these 2 issues is to add support for controlling a video-output's brightness through properties on the drm_connector. This RFC outlines my plan to try and actually implement this, which has 3 phases: Phase 1: 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. A plan to fix this was posted here: https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343...@redhat.com/ And a pull-req actually implementing this plan has been send out this week: https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1...@redhat.com/ Phase 2: Add drm_connector properties mirroring the matching backlight device = The plan is to add a drm_connector helper function, which optionally takes a pointer to the backlight device for the GPU's native backlight device, which will then mirror the backlight settings from the backlight device in a set of read/write brightness* properties on the connector. This function can then be called by GPU drivers for the drm_connector for the internal panel and it will then take care of everything. When there is no native GPU backlight device, or when it should not be used then (on x86) the helper will use the acpi_video_get_backlight_type() to determine which backlight-device should be used instead and it will find + mirror that one. Phase 3: Deprecate /sys/class/backlight uAPI Once most userspace has moved over to using the new drm_connector brightness props, a Kconfig option can be added to stop exporting the backlight-devices under /sys/class/backlight. The plan is to just disable the sysfs interface and keep the existing backlight-device internal kernel abstraction as is, since some abstraction for (non GPU native) backlight devices will be necessary regardless. It is unsure if we will ever be able to do this. For example people using non fully integrated desktop environments like e.g. sway often use custom scripts binded to hotkeys to get functionality like the brightness up/down keyboard hotkeys changing the brightness. This typically involves e.g. the xbacklight utility. Even if the xbacklight utility is ported to use kms with the new connector object brightness properties then this still will not work because changing the properties will require drm-master rights and e.g. sway will already hold those. The drm_connector brightness properties === The new uAPI for this consists of 2 properties: 1. "display brightness": rw 0-int32_max property controlling the brightness setting of the connected display. The actual maximum of this will be less then int32_max and is given in "display brightness max". Unlike the /sys/class/backlight/foo/brightness this brightness property has a clear definition for the value 0. The kernel must ensure that 0 means minimum brightness
Meeting (BOF) at Plumbers Dublin to discuss backlight brightness as connector object property RFC?
Hi All, I will be at Plumbers Dublin next week and I was wondering if anyone interested in this wants to get together for a quick discussion / birds of a feather session about this? I have just posted version 2 of the RFC: https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86...@redhat.com/T/#u If you are interested in meeting up please send me an email off-list (no need to spam the list further with this) also please let me know if there are any times which do not work for you, and/or times which have your preference. I don't have a specific room/time for this yet, but if people are interested I will try to get a room from the organization and if that does not work out I'm sure we will figure something out. One of the things which I would like to discuss is using the backlight brightness as connector object property vs external (not part of the compositor) tools to control the brightness like e.g. xbacklight, quoting from the RFC: "people using non fully integrated desktop environments like e.g. sway often use custom scripts binded to hotkeys to get functionality like the brightness up/down keyboard hotkeys changing the brightness. This typically involves e.g. the xbacklight utility. Even if the xbacklight utility is ported to use kms with the new connector object brightness properties then this still will not work because changing the properties will require drm-master rights and e.g. sway will already hold those." Note one obvious solution here would be for these use-cases to keep using the old /sys/class/backlight interface for this, with the downside that we will then be stuck to that interface for ever... Regards, Hans
Re: Meeting (BOF) at Plumbers Dublin to discuss backlight brightness as connector object property RFC?
On Friday, September 9th, 2022 at 12:23, Hans de Goede wrote: > "people using > non fully integrated desktop environments like e.g. sway often use custom > scripts binded to hotkeys to get functionality like the brightness > up/down keyboard hotkeys changing the brightness. This typically involves > e.g. the xbacklight utility. > > Even if the xbacklight utility is ported to use kms with the new connector > object brightness properties then this still will not work because > changing the properties will require drm-master rights and e.g. sway will > already hold those." I don't think this is a good argument. Sway (which I'm a maintainer of) can add a command to change the backlight, and then users can bind their keybinding to that command. This is not very different from e.g. a keybind to switch on/off a monitor. We can also standardize a protocol to change the backlight across all non-fully-integrated desktop environments (would be a simple addition to output-power-management [1]), so that a single tool can work for multiple compositors. Simon [1]: https://gitlab.freedesktop.org/wlroots/wlr-protocols/-/issues/114
Re: Meeting (BOF) at Plumbers Dublin to discuss backlight brightness as connector object property RFC?
On Fri, Sep 09, 2022 at 12:23:53PM +0200, Hans de Goede wrote: > Hi All, > > I will be at Plumbers Dublin next week and I was wondering if > anyone interested in this wants to get together for a quick > discussion / birds of a feather session about this? > > I have just posted version 2 of the RFC: > https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86...@redhat.com/T/#u > > If you are interested in meeting up please send me > an email off-list (no need to spam the list further with this) > also please let me know if there are any times which do not > work for you, and/or times which have your preference. > > I don't have a specific room/time for this yet, but if people > are interested I will try to get a room from the organization > and if that does not work out I'm sure we will figure something > out. > > One of the things which I would like to discuss is using > the backlight brightness as connector object property vs > external (not part of the compositor) tools to control the > brightness like e.g. xbacklight, quoting from the RFC: > > "people using > non fully integrated desktop environments like e.g. sway often use custom > scripts binded to hotkeys to get functionality like the brightness > up/down keyboard hotkeys changing the brightness. This typically involves > e.g. the xbacklight utility. > > Even if the xbacklight utility is ported to use kms with the new connector > object brightness properties then this still will not work because > changing the properties will require drm-master rights and e.g. sway will > already hold those." > > Note one obvious solution here would be for these use-cases to keep > using the old /sys/class/backlight interface for this, with the downside > that we will then be stuck to that interface for ever... Isn't xbacklight the thing that only works when you *have* the backlight property? Ie. currently only works on intel ddx which adds a custom property but doesn't work on modesetting ddx for example. -- Ville Syrjälä Intel
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
On Friday, September 9th, 2022 at 12:12, Hans de Goede wrote: > Phase 3: Deprecate /sys/class/backlight uAPI > > > Once most userspace has moved over to using the new drm_connector > brightness props, a Kconfig option can be added to stop exporting > the backlight-devices under /sys/class/backlight. The plan is to > just disable the sysfs interface and keep the existing backlight-device > internal kernel abstraction as is, since some abstraction for (non GPU > native) backlight devices will be necessary regardless. > > It is unsure if we will ever be able to do this. For example people using > non fully integrated desktop environments like e.g. sway often use custom > scripts binded to hotkeys to get functionality like the brightness > up/down keyboard hotkeys changing the brightness. This typically involves > e.g. the xbacklight utility. > > Even if the xbacklight utility is ported to use kms with the new connector > object brightness properties then this still will not work because > changing the properties will require drm-master rights and e.g. sway will > already hold those. I replied to this here in another thread [1]. tl;dr I think it would be fine even for Sway-like compositors. (Also note the utilities used right now are not xbacklight, but brightnessctl/light/brillo/etc [2]) [1]: https://lore.kernel.org/dri-devel/bZJU9OkYWFyaLHVa4XUE4d5iBTPFXBRyPe1wMd_ztKh5VBMu-EDNGoUDpvwtFn_u9-JMvN8QmIZVS3pzMZM_hZTkTCA9gOBnCGXc5HFmsnc=@emersion.fr/ [2]: https://github.com/swaywm/sway/wiki#xbacklight > The drm_connector brightness properties > === > > The new uAPI for this consists of 2 properties: > > 1. "display brightness": rw 0-int32_max property controlling the brightness > setting > of the connected display. The actual maximum of this will be less then > int32_max and is given in "display brightness max". > > Unlike the /sys/class/backlight/foo/brightness this brightness property > has a clear definition for the value 0. The kernel must ensure that 0 > means minimum brightness (so 0 should never turn the backlight off). > If necessary the kernel must enforce a minimum value by adding > an offset to the value seen in the property to ensure this behavior. > > For example if necessary the driver must clamp 0-255 to 10-255, which then > becomes 0-245 on the brightness property, adding 10 internally to writes > done to the brightness property. This adding of an extra offset when > necessary must only be done on the brightness property, > the /sys/class/backlight interface should be left unchanged to not break > userspace which may rely on 0 = off on some systems. > > Note amdgpu already does something like this even for /sys/class/backlight, > see the use of AMDGPU_DM_DEFAULT_MIN_BACKLIGHT in amdgpu. > > Also whenever possible the kernel must ensure that the brightness range > is in perceived brightness, but this cannot always be guaranteed. > > > 2. "display brightness max": ro 0-int32_max property giving the actual maximum > of the display's brightness setting. This will report 0 when brightness > control is not available. > > The value of "display brightness max" may change at runtime, either by > a legacy drivers/platform/x86 backlight driver loading after the drm > driver has loaded; or when plugging in a monitor which allows brightness > control over DDC/CI. In both these cases the max value will change from 0 > to the actual max value, indicating backlight control has become available > on this connector. The kernel will need to ensure that a hotplug uevent is sent to user-space at this point. Otherwise user-space has no way to figure out that the prop has changed. Overall this all looks very solid to me! Simon
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
On Fri, 09 Sep 2022 13:39:37 + Simon Ser wrote: > On Friday, September 9th, 2022 at 12:12, Hans de Goede > wrote: > > > Phase 3: Deprecate /sys/class/backlight uAPI > > > > > > Once most userspace has moved over to using the new drm_connector > > brightness props, a Kconfig option can be added to stop exporting > > the backlight-devices under /sys/class/backlight. The plan is to > > just disable the sysfs interface and keep the existing backlight-device > > internal kernel abstraction as is, since some abstraction for (non GPU > > native) backlight devices will be necessary regardless. > > > > It is unsure if we will ever be able to do this. For example people using > > non fully integrated desktop environments like e.g. sway often use custom > > scripts binded to hotkeys to get functionality like the brightness > > up/down keyboard hotkeys changing the brightness. This typically involves > > e.g. the xbacklight utility. > > > > Even if the xbacklight utility is ported to use kms with the new connector > > object brightness properties then this still will not work because > > changing the properties will require drm-master rights and e.g. sway will > > already hold those. > > I replied to this here in another thread [1]. > > tl;dr I think it would be fine even for Sway-like compositors. Furthermore, if other compositors are like Weston in their KMS state handling, and do not track which property has already been programmed to KMS and which hasn't, and instead just smash all KMS properties every update anyway (it's also great for debugging, you always have the full state in flight), anything changed via sysfs will be immediately reverted. Therefore I think there is a high probability that the external or sysfs controls just naturally stop working anyway, even if the kernel does not remove them first. Thanks, pq > (Also note the utilities used right now are not xbacklight, but > brightnessctl/light/brillo/etc [2]) > > [1]: > https://lore.kernel.org/dri-devel/bZJU9OkYWFyaLHVa4XUE4d5iBTPFXBRyPe1wMd_ztKh5VBMu-EDNGoUDpvwtFn_u9-JMvN8QmIZVS3pzMZM_hZTkTCA9gOBnCGXc5HFmsnc=@emersion.fr/ > [2]: https://github.com/swaywm/sway/wiki#xbacklight > pgp26VluWI55J.pgp Description: OpenPGP digital signature
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
On Friday, September 9th, 2022 at 15:53, Pekka Paalanen wrote: > On Fri, 09 Sep 2022 13:39:37 + > Simon Ser cont...@emersion.fr wrote: > > > On Friday, September 9th, 2022 at 12:12, Hans de Goede hdego...@redhat.com > > wrote: > > > > > Phase 3: Deprecate /sys/class/backlight uAPI > > > > > > > > > Once most userspace has moved over to using the new drm_connector > > > brightness props, a Kconfig option can be added to stop exporting > > > the backlight-devices under /sys/class/backlight. The plan is to > > > just disable the sysfs interface and keep the existing backlight-device > > > internal kernel abstraction as is, since some abstraction for (non GPU > > > native) backlight devices will be necessary regardless. > > > > > > It is unsure if we will ever be able to do this. For example people using > > > non fully integrated desktop environments like e.g. sway often use custom > > > scripts binded to hotkeys to get functionality like the brightness > > > up/down keyboard hotkeys changing the brightness. This typically involves > > > e.g. the xbacklight utility. > > > > > > Even if the xbacklight utility is ported to use kms with the new connector > > > object brightness properties then this still will not work because > > > changing the properties will require drm-master rights and e.g. sway will > > > already hold those. > > > > I replied to this here in another thread 1. > > > > tl;dr I think it would be fine even for Sway-like compositors. > > Furthermore, if other compositors are like Weston in their KMS state > handling, and do not track which property has already been programmed > to KMS and which hasn't, and instead just smash all KMS properties > every update anyway (it's also great for debugging, you always have the > full state in flight), anything changed via sysfs will be immediately > reverted. > > Therefore I think there is a high probability that the external or > sysfs controls just naturally stop working anyway, even if the kernel > does not remove them first. Ah yes, that's a good point. And this wouldn't be a kernel regression, it would be user-space like Sway or Weston taking the decision to use the new uAPI in a way which breaks the sysfs interface. (User-space could also take the decision to _not_ break the sysfs interface, by implementing a simple "dirty" flag.)
Re: Meeting (BOF) at Plumbers Dublin to discuss backlight brightness as connector object property RFC?
On Fri, 9 Sept 2022 at 12:50, Simon Ser wrote: > On Friday, September 9th, 2022 at 12:23, Hans de Goede < > hdego...@redhat.com> wrote: > > "people using > > non fully integrated desktop environments like e.g. sway often use custom > > scripts binded to hotkeys to get functionality like the brightness > > up/down keyboard hotkeys changing the brightness. This typically involves > > e.g. the xbacklight utility. > > > > Even if the xbacklight utility is ported to use kms with the new > connector > > object brightness properties then this still will not work because > > changing the properties will require drm-master rights and e.g. sway will > > already hold those." > > I don't think this is a good argument. Sway (which I'm a maintainer of) > can add a command to change the backlight, and then users can bind their > keybinding to that command. This is not very different from e.g. a > keybind to switch on/off a monitor. > > We can also standardize a protocol to change the backlight across all > non-fully-integrated desktop environments (would be a simple addition > to output-power-management [1]), so that a single tool can work for > multiple compositors. Yeah, I mean, as one of the main people arguing that non-fully-integrated desktops are not the design we want, I agree with Simon. Cheers, Daniel
Re: [RFC v2] drm/kms: control display brightness through drm_connector properties
Hi, On 9/9/22 15:39, Simon Ser wrote: > On Friday, September 9th, 2022 at 12:12, Hans de Goede > wrote: > >> Phase 3: Deprecate /sys/class/backlight uAPI >> >> >> Once most userspace has moved over to using the new drm_connector >> brightness props, a Kconfig option can be added to stop exporting >> the backlight-devices under /sys/class/backlight. The plan is to >> just disable the sysfs interface and keep the existing backlight-device >> internal kernel abstraction as is, since some abstraction for (non GPU >> native) backlight devices will be necessary regardless. >> >> It is unsure if we will ever be able to do this. For example people using >> non fully integrated desktop environments like e.g. sway often use custom >> scripts binded to hotkeys to get functionality like the brightness >> up/down keyboard hotkeys changing the brightness. This typically involves >> e.g. the xbacklight utility. >> >> Even if the xbacklight utility is ported to use kms with the new connector >> object brightness properties then this still will not work because >> changing the properties will require drm-master rights and e.g. sway will >> already hold those. > > I replied to this here in another thread [1]. > > tl;dr I think it would be fine even for Sway-like compositors. Ok, that is good to know. > (Also note the utilities used right now are not xbacklight, but > brightnessctl/light/brillo/etc [2]) Ah I thought that xbacklight got patched at one point to support the sysfs API, but I see now that instead alternative utilities have popped up. Regards, Hans > > [1]: > https://lore.kernel.org/dri-devel/bZJU9OkYWFyaLHVa4XUE4d5iBTPFXBRyPe1wMd_ztKh5VBMu-EDNGoUDpvwtFn_u9-JMvN8QmIZVS3pzMZM_hZTkTCA9gOBnCGXc5HFmsnc=@emersion.fr/ > [2]: https://github.com/swaywm/sway/wiki#xbacklight > >> The drm_connector brightness properties >> === >> >> The new uAPI for this consists of 2 properties: >> >> 1. "display brightness": rw 0-int32_max property controlling the brightness >> setting >> of the connected display. The actual maximum of this will be less then >> int32_max and is given in "display brightness max". >> >> Unlike the /sys/class/backlight/foo/brightness this brightness property >> has a clear definition for the value 0. The kernel must ensure that 0 >> means minimum brightness (so 0 should never turn the backlight off). >> If necessary the kernel must enforce a minimum value by adding >> an offset to the value seen in the property to ensure this behavior. >> >> For example if necessary the driver must clamp 0-255 to 10-255, which then >> becomes 0-245 on the brightness property, adding 10 internally to writes >> done to the brightness property. This adding of an extra offset when >> necessary must only be done on the brightness property, >> the /sys/class/backlight interface should be left unchanged to not break >> userspace which may rely on 0 = off on some systems. >> >> Note amdgpu already does something like this even for /sys/class/backlight, >> see the use of AMDGPU_DM_DEFAULT_MIN_BACKLIGHT in amdgpu. >> >> Also whenever possible the kernel must ensure that the brightness range >> is in perceived brightness, but this cannot always be guaranteed. >> >> >> 2. "display brightness max": ro 0-int32_max property giving the actual >> maximum >> of the display's brightness setting. This will report 0 when brightness >> control is not available. >> >> The value of "display brightness max" may change at runtime, either by >> a legacy drivers/platform/x86 backlight driver loading after the drm >> driver has loaded; or when plugging in a monitor which allows brightness >> control over DDC/CI. In both these cases the max value will change from 0 >> to the actual max value, indicating backlight control has become available >> on this connector. > > The kernel will need to ensure that a hotplug uevent is sent to > user-space at this point. Otherwise user-space has no way to figure out > that the prop has changed. > > Overall this all looks very solid to me! > > Simon >
