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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 28, 2022 at 01:57:18PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 28, 2022 at 01:04:01PM +0300, Jani Nikula wrote:
> > 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.)
> 
> Why don't we just normalize the range to something sensible?
> 
> > 
> > > 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?
> 
> Hmm. I guess due to this. I'd prefer the opposite tbh so I could
> just put in my opregion BCLM patch. It's annoying to have to
> carry it locally just to have reasonable backlight behaviour

After second though I guess I'm actually agreeing with Hans here.
The current situation is where small change in the value near one
end of the range does basically nothing, while a small change at
the other of the range causes a massive brightness change. That
is no good.

-- 
Ville Syrjälä
Intel