From: Hans de Goede <firstname.lastname@example.org>
To: Alex Deucher <email@example.com>, Daniel Vetter <firstname.lastname@example.org>
Cc: Sebastian Wick <email@example.com>,
Martin Roukala <firstname.lastname@example.org>,
Christoph Grenz <email@example.com>,
Yusuf Khan <firstname.lastname@example.org>
Subject: Re: [RFC] drm/kms: control display brightness through drm_connector properties
Date: Fri, 8 Apr 2022 16:55:51 +0200 [thread overview]
Message-ID: <email@example.com> (raw)
On 4/8/22 16:08, Alex Deucher wrote:
> On Fri, Apr 8, 2022 at 4:07 AM Daniel Vetter <firstname.lastname@example.org> 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 <email@example.com> wrote:
>>>> Hi Simon,
>>>> On 4/7/22 18:51, Simon Ser wrote:
>>>>> Very nice plan! Big +1 for the overall approach.
>>>>> On Thursday, April 7th, 2022 at 17:38, Hans de Goede <firstname.lastname@example.org> 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
>> 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 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.
>>>>> Hm. It's quite unfortunate that it's impossible to have strong guarantees
>>>>> Is there any way we can avoid this prop?
>>>> Not really, the problem is that we really don't know if 0 is off
>>>> or min-brightness. In the given example where we actually never go
>>>> down to a duty-cycle of 0% because the video BIOS tables tell us
>>>> not to, we can be certain that setting the brightness prop to 0
>>>> will not turn of the backlight, since we then set the duty-cycle
>>>> to the VBT provided minimum. Note the intend here is to only set
>>>> the boolean to true if the VBT provided minimum is _not_ 0, 0
>>>> just means the vendor did not bother to provide a minimum.
>>>> Currently e.g. GNOME never goes lower then something like 5%
>>>> of brightness_max to avoid accidentally turning the screen off.
>>>> Turning the screen off is quite bad to do on e.g. tablets where
>>>> the GUI is the only way to undo the brightness change and now
>>>> the user can no longer see the GUI.
>>>> The idea behind this boolean is to give e.g. GNOME a way to
>>>> know that it is safe to go down to 0% and for it to use
>>>> the entire range.
>>> Why not just make it policy that 0 is defined as minimum brightness,
>>> not off, and have all drivers conform to that?
>> Because the backlight subsystem isn't as consistent on this, and it's been
>> an epic source of confusion since forever.
>> What's worse, there's both userspace out there which assumes brightness =
>> 0 is a really fast dpms off _and_ userspace that assumes that brightness =
>> 0 is the lowest setting. Of course on different sets of machines.
>> So yeah we're screwed. I have no idea how to get out of this.
> Yes, but this is a new API. So can't we do better? Sure the old
> backlight interface is broken, but why carry around clunky workarounds
> for new interfaces?
Right we certainly need to define the behavior of the new API
clearly, so that userspace does not misuse / misinterpret it.
The intend is for brightness=0 to mean minimum brightness
to still be able to see what is on the screen. But the problem
is that in many cases the GPU driver directly controls a PWM
output, no minimum PWM value is given in the video BIOS tables
and actually setting the PWM to 0% dutycycle turns off the
So we can only promise a best-effort to make brightness=0
mean minimum brightness, combined with documenting that it
may turn off the backlight and that userspace _must_ never
depend on it turning off the backlight.
Also note that setting a direct PWM output to duty-cycle 0%
does not guarantee that the backlight goes off, this may be
an input for a special backlight LED driver IC like the
TI LP855x series which can have an internal lookup
table causing it to actually output a minimum brightness
when its PWM input is at 0% dutycycle. So this is a case
where we just don't get enough info from the fw/hw to be able
to offer the guarantees which we would like to guarantee.
next prev parent reply other threads:[~2022-04-08 14:56 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-07 15:38 [RFC] drm/kms: control display brightness through drm_connector properties Hans de Goede
2022-04-07 16:51 ` Simon Ser
2022-04-07 17:43 ` Hans de Goede
2022-04-07 21:05 ` Alex Deucher
2022-04-08 8:07 ` Daniel Vetter
2022-04-08 9:58 ` Hans de Goede
2022-04-08 10:09 ` Hans de Goede
2022-04-08 10:16 ` Simon Ser
2022-04-08 10:26 ` Hans de Goede
2022-04-13 8:32 ` Daniel Vetter
2022-04-13 8:38 ` Simon Ser
2022-04-13 9:44 ` Hans de Goede
2022-04-08 10:23 ` Hans de Goede
2022-04-08 14:08 ` Alex Deucher
2022-04-08 14:55 ` Hans de Goede [this message]
2022-04-08 15:11 ` Alex Deucher
2022-04-11 10:18 ` Hans de Goede
2022-04-11 11:34 ` Pekka Paalanen
2022-04-11 11:50 ` Hans de Goede
2022-04-11 13:11 ` Mikhail Gusarov
2022-04-11 14:11 ` Alex Deucher
2022-04-14 10:24 ` Jani Nikula
2022-04-27 14:03 ` Daniel Vetter
2022-04-27 14:23 ` Jani Nikula
2022-04-27 14:26 ` Daniel Vetter
2022-04-29 8:55 ` Hans de Goede
2022-04-29 8:59 ` Simon Ser
2022-04-29 9:06 ` Pekka Paalanen
2022-04-29 9:49 ` Lattannavar, Sameer
2022-04-08 8:22 ` Simon Ser
2022-04-08 15:00 ` Hans de Goede
2022-04-11 10:35 ` Hans de Goede
2022-04-07 18:58 ` Carsten Haitzler
2022-04-11 10:27 ` Hans de Goede
2022-04-11 11:14 ` Carsten Haitzler
2022-04-14 13:10 ` Jani Nikula
2022-05-18 12:59 ` Hans de Goede
2022-05-18 14:23 ` Jani Nikula
2022-05-31 10:40 ` Hans de Goede
2022-05-18 14:40 ` Ville Syrjälä
2022-08-24 2:18 ` Yusuf Khan
2022-08-25 8:27 ` Hans de Goede
2022-08-25 21:40 ` Yusuf Khan
2022-08-28 8:08 ` Hans de Goede
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).