Hi, On 9/9/22 15:39, Simon Ser wrote: > On Friday, September 9th, 2022 at 12:12, Hans de Goede <hdegoede@xxxxxxxxxx> 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 >
