Re: [RFC] drm/kms: control display brightness through drm_connector properties

[Yusuf Khan <yusisamerican@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 15243 bytes --]

Perhaps the Kconfig modifications could be postponed to stage 2
since for people running distros that suddenly decide to disable
/sys/class/backlight/ it may be impractical for them to recompile
their kernels and such. Also stage 2 should probably take ~2 decades
until it comes into being, for reference fbdev SPECIFIC drivers
were removed from fedora just recently and because of that there
were some issues with some user's systems. I understand it's much
easier to change from the /sys/class/backlight/ interface to the one
you have proposed than to change from fbdev to KMS though.

On Thu, Aug 25, 2022 at 3:27 AM Hans de Goede <hdegoede@redhat.com> wrote:

> Hi Yusuf,
>
> On 8/24/22 04:18, Yusuf Khan wrote:
> > Sorry for the necro-bump, I hadnt seen this go by
>
> No problem.
>
> > My main concern with this proposal is the phasing out of
> /sys/class/backlight/.
> > Currently on the user(user, not userland) level its easier for me to
> just modify
> > the file and be done with it. xbacklight doesnt tell me when its failed,
> > brightnessctl doesnt make assumptions about what device is what, and
> > other brightness setting applications ive seen are much worse than them.
> > Someone needs to create a userland application thats less inconvenient
> > than `echo`ing into /sys/class/backlight with a name that human beings
> can
> > actually remember before I stop using the sysfs, perhaps "setbrightness"
> > could be the binary's name? Also I dont think its wise to disable or
> make it
> > read only though Kconfig as older apps may depend on it, maybe add a
> > kernel param that disables the old interface so bigger distros can
> pressure
> > app makers into changing the interface? As a big draw for DDC/CI is that
> > many displays support it as a way to change brightness(even if you arent
> > doing anything special that would break the old interface) perhaps it
> could
> > be an early adopter to that kernel parameter?
>
> Right, so deprecating the /sys/class/backlight API definitely is the last
> step and probably is years away. As you say hiding / making it read-only
> should probably be a kernel-parameter at first, with maybe a Kconfig
> option to set the default. So the depcration would go like this:
>
> 1. Add:
> A kernel-parameter to allow hiding or read-only-ing the sysfs interface +
> Kconfig to select the default +
> dev_warn_once() when the old API is used
>
> 2. (much later) Drop the Kconfig option and default to hiding/read-only
>
> 3. (even later) Maybe completely remove the sysfs interface?
>
> Note the hiding vs read-only thing is to be decided. ATM I'm rather more
> focused on getting the new API in place then on deprecating the old one :)
>
> Anyways I fully agree that we need to do the deprecation carefully and
> slowly. This is likely going to take multiple years and then some ...
>
> Regards,
>
> Hans
>
>
>
> >
> > On Thu, Apr 7, 2022 at 10:39 AM Hans de Goede <hdegoede@redhat.com
> <mailto:hdegoede@redhat.com>> wrote:
> >
> >     As discussed already several times in the past:
> >      https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/ <
> https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/>
> >
> https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@linux.intel.com/
> <
> https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@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 <
> 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
> brightness
> >       when userspace saves it over suspend/resume or reboots.
> >     - Almost all laptops have brightness up/down hotkeys. E.g GNOME
> decides
> >       on a step size for the hotkeys by doing min(brightness_max/20, 1).
> >       Some of the vendor specific backlight fw APIs (e.g. dell-laptop)
> have
> >       only 8 steps. When giving userspace the actual max_brightness
> value, then
> >       this will all work just fine. When hardcode brightness_max to
> 65535 OTOH
> >       then in this case GNOME will still give the user 20 steps where
> only 1
> >       in every 2-3 steps actually changes the brightness which IMHO is
> >       an unacceptably bad user experience.
> >
> >     0-65535 advantages / 1:1 disadvantages
> >     - Without a fixed scale for the brightness property the
> brightness_max
> >       value may change after an userspace application's initial
> enumeration
> >       of the drm_connector. This can happen when neither the native GPU
> nor
> >       the acpi_video backlight devices are present/usable in this case
> >       acpi_video_get_backlight_type() will _assume_ a vendor specific fw
> API
> >       will be used for backlight control and the driver proving the
> "vendor"
> >       backlight device will show up much later and may even never
> show-up,
> >       so waiting for it is not an option. With a fixed 0-65535 scale
> userspace
> >       can just always assume this and the drm_connector backlight props
> helper
> >       code can even cache writes and send it to the actual backlight
> device
> >       when it shows up. With a 1:1 mapping userspace needs to listen for
> >       a uevent and then update the brightness range on such an event.
> >
> >     I believe that the 1:1 mapping advantages out way the disadvantages
> >     here. Also note that current userspace already blindly assumes that
> >     all relevant drivers are loaded before the graphical-environment
> >     starts and all the desktop environments as such already only do
> >     a single scan of /sys/class/backlight when they start. So when
> >     userspace forgets to add code to listen for the uevent when switching
> >     to the new API nothing changes; and with the uevent userspace
> actually
> >     gets a good mechanism to detect backlight drivers loading after
> >     the graphical-environment has already started.
> >
> >     So based on this here is my proposal for a set of new brightness
> >     properties on the drm_connector object. Note these are all prefixed
> with
> >     bl which stands for backlight, which is technically not correct for
> OLED.
> >     But we need a prefix to avoid a name collision with the "brightness"
> >     attribute which is part of the existing TV specific properties and
> IMHO
> >     it is good to have a common prefix to make it clear that these all
> >     belong together.
> >
> >
> >     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.
> >
> >     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).
> >
> >     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 _not_ guaranteed.
> >     This will e.g. be true when directly driving a PWM and the video-BIOS
> >     has provided a minimum (non 0) duty-cycle below which the driver will
> >     never go.
> >
> >     bl_brightness_control_method: ro, enum, possible values:
> >     none:     The GPU driver expects brightness control to be provided
> by another
> >               driver and that driver has not loaded yet.
> >     unknown:  The underlying control mechanism is unknown.
> >     pwm:      The brightness property directly controls the duty-cycle
> of a PWM
> >               output.
> >     firmware: The brightness is controlled through firmware calls.
> >     DDC/CI:   The brightness is controlled through the DDC/CI protocol.
> >     gmux:     The brightness is controlled by the GMUX.
> >     Note this enum may be extended in the future, so other values may
> >     be read, these should be treated as "unknown".
> >
> >     When brightness control becomes available after being reported
> >     as not available before (bl_brightness_control_method=="none")
> >     a uevent with CONNECTOR=<connector-id> and
> >     PROPERTY=<bl_brightness_control_method-id> will be generated
> >     at this point all the properties must be re-read.
> >
> >     When/once brightness control is available then all the read-only
> >     properties are fixed and will never change.
> >
> >     Besides the "none" value for no driver having loaded yet,
> >     the different bl_brightness_control_method values are intended for
> >     (userspace) heuristics for such things as the brightness setting
> >     linearly controlling electrical power or setting perceived
> brightness.
> >
> >     Regards,
> >
> >     Hans
> >
> >
> >     1) The need to be able to map the backlight device to a specific
> display
> >     has become clear once more with the recent proposal to add DDCDI
> support:
> >
> https://lore.kernel.org/lkml/20220403230850.2986-1-yusisamerican@gmail.com/
> <
> https://lore.kernel.org/lkml/20220403230850.2986-1-yusisamerican@gmail.com/
> >
> >
> >     2)
> https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@linux.intel.com/
> <
> https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@linux.intel.com/
> >
> >     Note this proposal included a method for userspace to be able to
> tell the
> >     kernel if the native/acpi_video/vendor backlight device should be
> used,
> >     but this has been solved in the kernel for years now:
> >      https://www.spinics.net/lists/linux-acpi/msg50526.html <
> https://www.spinics.net/lists/linux-acpi/msg50526.html>
> >     An initial implementation of this proposal is available here:
> >      https://cgit.freedesktop.org/~mperes/linux/log/?h=backlight <
> https://cgit.freedesktop.org/~mperes/linux/log/?h=backlight>
> >
>
>

[-- Attachment #2: Type: text/html, Size: 19399 bytes --]