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

Thu Sep 29 18:06:50 UTC 2022

If it is supposed to be a non-linear luminance curve, which one is it?
It would be much clearer if user space can control linear luminance
and use whatever definition of perceived brightness it wants. The
obvious downside of it is that it requires bits to encode changes that
users can't perceive. What about backlights which only have a few
predefined luminance levels? How would user space differentiate
between the continuous and discrete backlight? What about
self-emitting displays? They usually increase the dynamic range when
they increase in brightness because the black level doesn't rise. They
also probably employ some tonemapping to adjust for that. What about
the range of the backlight? What about the absolute luminance of the
backlight? I want to know about that all in user space.

I understand that most of the time the kernel doesn't have answers to
those questions right now but the API should account for all of that.

On Wed, Sep 28, 2022 at 1:14 PM Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
>
> 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 at 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
>