Re: [RFC] drm/kms: control display brightness through drm_connector properties
On Mon, 11 Apr 2022, Alex Deucher wrote: > On Mon, Apr 11, 2022 at 6:18 AM Hans de Goede wrote: >> >> Hi, >> >> On 4/8/22 17:11, Alex Deucher wrote: >> > On Fri, Apr 8, 2022 at 10:56 AM Hans de Goede wrote: >> >> >> >> Hi, >> >> >> >> On 4/8/22 16:08, Alex Deucher wrote: >> >>> On Fri, Apr 8, 2022 at 4:07 AM Daniel Vetter wrote: >> >> On Thu, Apr 07, 2022 at 05:05:52PM -0400, Alex Deucher wrote: >> > On Thu, Apr 7, 2022 at 1:43 PM Hans de Goede >> > wrote: >> >> >> >> Hi Simon, >> >> >> >> On 4/7/22 18:51, Simon Ser wrote: >> >>> Very nice plan! Big +1 for the overall approach. >> >> >> >> Thanks. >> >> >> >>> On Thursday, April 7th, 2022 at 17:38, Hans de Goede >> >>> wrote: >> >>> >> The drm_connector brightness properties >> === >> >> bl_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 bl_brightness_max. >> >>> >> >>> Do we need to split this up into two props for sw/hw state? The >> >>> privacy screen >> >>> stuff needed this, but you're pretty familiar with that. :) >> >> >> >> Luckily that won't be necessary, since the privacy-screen is a >> >> security >> >> feature the firmware/embedded-controller may refuse our requests >> >> (may temporarily lock-out changes) and/or may make changes without >> >> us requesting them itself. Neither is really the case with the >> >> brightness setting of displays. >> >> >> bl_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 (yet). >> >>> >> >>> I don't think we actually need that one. Integer KMS props all have a >> >>> range which can be fetched via drmModeGetProperty. The max can be >> >>> exposed via this range. Example with the existing alpha prop: >> >>> >> >>> "alpha": range [0, UINT16_MAX] = 65535 >> >> >> >> Right, I already knew that, which is why I explicitly added a range >> >> to the props already. The problem is that the range must be set >> >> before registering the connector and when the backlight driver >> >> only shows up (much) later during boot then we don't know the >> >> range when registering the connector. I guess we could "patch-up" >> >> the range later. But AFAIK that would be a bit of abuse of the >> >> property API as the range is intended to never change, not >> >> even after hotplug uevents. At least atm there is no infra >> >> in the kernel to change the range later. >> >> >> >> Which is why I added an explicit bl_brightness_max property >> >> of which the value gives the actual effective maximum of the >> >> brightness. >> >> Uh ... I'm not a huge fan tbh. The thing is, if we allow hotplugging >> brightness control later on then we just perpetuate the nonsense we have >> right now, forever. >> >> Imo we should support two kinds of drivers: >> >> - drivers which are non-crap, and make sure their backlight driver is >> loaded before they register the drm_device (or at least the >> drm_connector). For those we want the drm_connector->backlight pointer >> to bit static over the lifetime of the connector, and then we can also >> set up the brightness range correctly. >> >> - funny drivers which implement the glorious fallback dance which >> libbacklight implements currently in userspace. Imo for these drivers >> we >> should have a libbacklight_heuristics_backlight, which normalizes or >> whatever, and is also ways there. And then internally handles the >> fallback mess to the "right" backlight driver. >> >> We might have some gaps on acpi systems to make sure the drm driver can >> wait for the backlight driver to show up, but that's about it. >> >> Hotplugging random pieces later on is really not how drivers work >> nowadays >> with deferred probe and component framework and all that. >> >> >> I did consider using the range for this and updating it >> >> on the fly I think nothing is really preventing us from >> >> doing so, but it very much feels like abusing the generic >> >> properties API. >> >> >> bl_brightness_0_is_min_brightness: ro, boolean >> When this is set to true then it is safe to set brightness to 0 >> without worrying that this completely turns the backlight off >> causing >> the screen to become unreadable. When this is false setting >> brightness >> to 0 may turn the backlight off, but this is
Re: [RFC] drm/kms: control display brightness through drm_connector properties
On Thu, 07 Apr 2022, Hans de Goede wrote: > 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, the biggest 2 being: > > 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. > > 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) 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. > b) 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 > > I already have a pretty detailed plan to tackle this, which I will > post in a separate RFC email. I plan to start working on this right > away, as it will be good to have this fixed regardless. > > > 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. > > An alternative to disabling the sysfs class entirely, would be > to allow setting it to read-only through Kconfig. > > > What scale to use for the drm_connector bl_brightness property? > === > > The tricky part of this plan is phase 2 and then esp. defining what the > new brightness properties will look like and how they will work. > > The biggest challenge here is to decide on a fixed scale for the main > brightness property, say 0-65535, using scaling where the actual hw scale > is different, or if this should simply be a 1:1 mirror of the current > backlight interface, with the actual hw scale / brightness_max value > exposed as a drm_connector property. > > 1:1 advantages / 0-65535 disadvantages > - Userspace will likely move over to the connector-props quite slowly and > we can expect various userspace bits, esp. also custom user scripts, to > keep using the old uAPI for a long time. Using the 2 APIs are intermixed > is fine when using a 1:1 brightness scale mapping. But if we end up doing > a scaling round-trip all the time then eventually the brightness is going > do drift. This can even happen if the user never changes the bri
