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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 09 Sep 2022, Hans de Goede <hdegoede@redhat.com> wrote:
> 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-d37d844b02c6@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-652f50343158@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-a2c96c9b1eff@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".

This could use a few words explaining the choice of range and property
type. (I assume it's because you can't change a range property's max at
runtime. Which is also why you need a separate max property.)

> 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.

Do you mean every step should be a visible change?

> 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.

I think this could be a bit more restrictive in stating the allowed
runtime changes. Is it only 0 -> actual max for non-hotpluggable
displays, nothing else, and additionally actual max -> 0 when unplugging
a display?

>
>
> Future extensions
> =================
>
> Some hardware do adaptive brightness in hardware, rather then providing
> an ALS sensor and letting userspace handle this.
>
> One example of this is the Steam deck, which currently uses some custom
> sysfs attributes to allow tweaking (and enable/disable?) the adaptive
> brightness. Adding standardized uAPI for this through new
> "display brightness *" properties seems like a natural extension of this
> proposal.

Another example is adjusting for non-linear luminance curve. It would be
nicer for equal steps in brightness value to have similar luminance
changes. I think sometimes we lose precision with the limited number of
steps in UIs. But this is thinking pretty far ahead. :)

Other than the minor clarifications, the plans sounds good to me.

Thanks again for doing all this, much appreciated!


BR,
Jani.

>
> 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/
>
> 2) 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
> An initial implementation of this proposal is available here:
>  https://cgit.freedesktop.org/~mperes/linux/log/?h=backlight
>
>

-- 
Jani Nikula, Intel Open Source Graphics Center