dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: Simon Ser <contact@emersion.fr>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>,
	Martin Roukala <martin.roukala@mupuf.org>,
	Christoph Grenz <christophg+lkml@grenz-bonn.de>,
	wayland <wayland-devel@lists.freedesktop.org>,
	Yusuf Khan <yusisamerican@gmail.com>
Subject: Re: [RFC] drm/kms: control display brightness through drm_connector properties
Date: Fri, 08 Apr 2022 08:22:36 +0000	[thread overview]
Message-ID: <XwpYE-RK1zRFJfojeMJV5ddsxHnHg4HRTXd4fZ_7yUMBZRCy3ARRIPC6Y-eCJ5Ag9Fin8FGLz6t-L8Ix4P7ykQlrJ6dH4LYye20kyHKtVaI=@emersion.fr> (raw)
In-Reply-To: <0e1cffc1-e8b6-dc58-56ff-53f911f33e60@redhat.com>

Hi Hans,

Thanks for your details replies!

On Thursday, April 7th, 2022 at 19:43, Hans de Goede <hdegoede@redhat.com> wrote:

> > On Thursday, April 7th, 2022 at 17:38, Hans de Goede <hdegoede@redhat.com> 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.

Cool, makes sense to me!

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

Since this is new uAPI there's no concern about backwards compat here. So it's
pretty much a matter of how we want the uAPI to look like. I was suggesting
using a range because it's self-describing, but maybe it's an abuse.

Daniel Vetter, what do you think? If a property's range is going to be updated
on the fly, should we go for it, or should we use a separate prop to describe
the max value?

> >> 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
> > here.
> >
> > 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.
> > For instance if we can guarantee that the min level won't turn the screen
> > completely off we could make the range start from 1 instead of 0.
> > Or allow -1 to mean "minimum value, maybe completely off".
> Right, the problem is we really don't know and when the range is
> e.g. 0-65535 then something like 1 will almost always still just
> turn the screen completely off. There will be a value of say like
> 150 or some such which is then the actual minimum value to still
> get the backlight to light up at all. The problem is we have
> no clue what the actual minimum is. And if the PWM output does
> not directly drive the LEDs but is used as an input for some
> LED backlight driver chip, that chip itself may have a lookup
> table (which may also take care of doing perceived brightness
> mapping) and may guarantee a minimum backlight even when given
> a 0% duty cycle PWM signal...

Oh, what a fun world we live in.

Would it be completely unreasonable to have a hwdb in the kernel to know the
real minimum value if it hasn't been provided by the VBT? Or would that be too
much of a colossal effort?

What happens in the DDC/CI world? What does 0 mean there? Is it the same messy
situation again?

> This prop is sort of orthogonal to the generic change to
> drm_connector props, so we could also do this later as a follow up
> change. At a minimum when I code this up this should be in its
> own commit(s) I believe.
> But I do think having this will be useful for the above
> GNOME example.
> >> 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.
> >
> > Can you elaborate? I don't know enough about brightness control to
> > understand all of the implications here.
> So after sending this email I was already thinking myself that this
> one might not be the best idea. Another shortcoming of the current
> backlight API is that it does not let userspace know if the
> number is a linear control of the time the LEDs are on vs off
> (assuming a LED backlight) or if some component already uses a
> lookup table to make 0-100% be more of a linear scale in the
> human perception, which is very much non linear. See e.g.:
> https://www.sciencedirect.com/topics/computer-science/perceived-brightness
> "refers to the perceived amount of light coming from self-luminous sources"
> "Perceived brightness is a very nonlinear function of the amount of light emitted by a lamp."
> The problem is that at the kernel level we have no idea if
> we are controlling "the amount of light emitted" or
> perceived brightness and it would be sorta nice for userspace
> to know. So the idea here is/was to allow userspace to make some
> educated guess here. E.g. a bl_brightness_control_method of "PWM"
> hints strongly at "the amount of light emitted" (but this is
> not true 100% of the time).  ATM userspace does not do any
> "perceived brightness" curve correction so for the first
> implementation of moving brightness control to drm properties
> I believe it might be better to just park the whole
> bl_brightness_control_method propery idea.

Hm, I see.

Are there other use-cases for this property besides the perceived brightness?

What other non-PWM methods set the perceived brightness (as opposed to
electrical power)?

As above, is it completely unreasonable to have a hwdb?

  parent reply	other threads:[~2022-04-08  8:22 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
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 [this message]
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

Reply instructions:

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 \
    --in-reply-to='XwpYE-RK1zRFJfojeMJV5ddsxHnHg4HRTXd4fZ_7yUMBZRCy3ARRIPC6Y-eCJ5Ag9Fin8FGLz6t-L8Ix4P7ykQlrJ6dH4LYye20kyHKtVaI=@emersion.fr' \
    --to=contact@emersion.fr \
    --cc=christophg+lkml@grenz-bonn.de \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=hdegoede@redhat.com \
    --cc=martin.roukala@mupuf.org \
    --cc=sebastian.wick@redhat.com \
    --cc=wayland-devel@lists.freedesktop.org \
    --cc=yusisamerican@gmail.com \


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