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

[Alex Deucher <alexdeucher@gmail.com>]

On Fri, Apr 8, 2022 at 4:07 AM Daniel Vetter <daniel@ffwll.ch> 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 <hdegoede@redhat.com> wrote:
> > >
> > > Hi Simon,
> > >
> > > On 4/7/22 18:51, Simon Ser wrote:
> > > > Very nice plan! Big +1 for the overall approach.
> > >
> > > Thanks.
> > >
> > > > 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.
> > >
> > > >> 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.
>
> 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
> > > > 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.
> >
> > 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?

Alex

> -Daniel
>
> >
> > Alex
> >
> > >
> > > > 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...
> > >
> > > 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.
> > >
> > > Which would leave the problem of communicating the control_method=="none"
> > > case but we can just use bl_brightness_max == 0 for that.
> > >
> > > Regards,
> > >
> > > Hans
> > >
> > >
> > >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch