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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 4/8/22 11:58, Hans de Goede wrote:
> Hi Daniel,
> 
> On 4/8/22 10:07, Daniel Vetter 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.
> 
> The only problem is that outside of device-tree platforms where
> we can have a backlight link in a devicetree display-connector node,
> there are no non crap devices and thus no non crap drivers.
> 
>> - 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.
> 
> So this will be pretty much all of them including i915 and nouveau.
> 
> My first thoughts where the same as yours and we can mostly guarantee
> that the drm_connector->backlight pointer is static over lifetime of
> the connector. But the problem is with the backlight device-s provided
> by things like the dell-laptop, thinkpad_acpi, etc. drivers which are
> still necessary / used for backlight control on core2duo era laptops
> which are still being active used by people.
> 
> Basically atm the kernel code to determine which backlight-device
> to use (which assumes a single internal LCD panel) goes like this (1):
> 
> 1. Check cmdline-override, DMI quirks (and return their value if set)
> 2. If ACPI video extensions are not supported then expect a backlight
>    device of the dell-laptop, thinkpad_acpi, etc. type, and use that.
> 3. If the ACPI tables have been written for Windows8 or later and
>    the GPU driver offers a GPU native backlight device use that.
> 4. Use the ACPI video extensions backlight device
> 
>> 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.
> 
> The problem here is 2. or IOW devices which don't support the
> ACPI video extensions, these typically (always?) also don't offer
> a GPU native backlight device, instead relying on
> the embedded-controller for backlight control using some vendor
> specific firmware API to talk to the EC.
> 
> For the other cases there are indeed some gaps which I plan to close
> so that we can make sure that the backlight device will be in place
> when we register the connector.
> 
> But the old devices without ACPI video extensions case is a big
> problem and more then just some gaps" and that is a path which all
> major x86 drivers may hit.
> 
> In some cases I even expect the backlight_device to simply never
> show up when hitting 2. Either because the necessary driver is
> not enabled in the kernel or because no-one ever added support for
> the specific fw interface used on the laptop in question. But I
> do expect this to be quite rare.
> 
> For the privacy-screen case where we had a similar issue this
> was solved by in essence duplicating the detection part of the
> privacy-screen drivers inside the drm_privacy code and use
> -EPROBE_DEFER to wait for the privacy-screen driver to load.
> 
> But in this case that is not really feasible IMHO because:
> 
> [hans@shalem linux]$ ack -l backlight_device_register drivers/platform/x86
> drivers/platform/x86/toshiba_acpi.c
> drivers/platform/x86/intel/oaktrail.c
> drivers/platform/x86/dell/dell-laptop.c
> drivers/platform/x86/msi-laptop.c
> drivers/platform/x86/panasonic-laptop.c
> drivers/platform/x86/ideapad-laptop.c
> drivers/platform/x86/sony-laptop.c
> drivers/platform/x86/thinkpad_acpi.c
> drivers/platform/x86/acer-wmi.c
> drivers/platform/x86/samsung-q10.c
> drivers/platform/x86/asus-wmi.c
> drivers/platform/x86/apple-gmux.c
> drivers/platform/x86/nvidia-wmi-ec-backlight.c
> drivers/platform/x86/msi-wmi.c
> drivers/platform/x86/asus-laptop.c
> drivers/platform/x86/classmate-laptop.c
> drivers/platform/x86/eeepc-laptop.c
> drivers/platform/x86/fujitsu-laptop.c
> drivers/platform/x86/samsung-laptop.c
> drivers/platform/x86/compal-laptop.c
> [hans@shalem linux]$ ack -l backlight_device_register drivers/platform/x86 | wc -l
> 20
> 
> Duplicating 20 wildly different ACPI/WMI backlight detection
> routines is a bit much; and also something which I cannot test
> easily and doing EPROBE_DEFER like behavior will require all
> of these to also be available in the initrd.
> 
> So IMHO at least for devices relying on these it is best to allow
> having the bl_brightness* properties be presend on the internal
> LCD connector at registration time with a hint that they are
> not functional and then send an uevent when they become functional.
> 
> I really see no other way to deal with these (old) devices.

Oh and one important thing which I forgot to add, it is these
old vendor specific firmware APIs for setting the backlight which
have the issue of having only say 8 levels, so scaling those
to 0-65535 leads to the:

"E.g GNOME decides
  on a step size for the hotkeys by doing min(brightness_max/20, 1).
  Some of the vendor specific backlight fw APIs (e.g. dell-laptop) have
  only 8 steps. When giving userspace the actual max_brightness value, then
  this will all work just fine. When hardcode brightness_max to 65535 OTOH
  then in this case GNOME will still give the user 20 steps where only 1
  in every 2-3 steps actually changes the brightness which IMHO is
  an unacceptably bad user experience."

problem from my original email starting the thread. One thing which
I did consider is to always scale to 0-65535 and then add a
"bl_brightness_step_size" property which would then be set to
65535/8 = 8192 in this case. But there are 2 disadvantages to this:

1. We still need a uevent for when the step-size changes once
the backlight-device finally shows up on impacted old devices
2. Scaling between the backlight device and the property value
sooner or later may lead to drift due to rounding issues.

So I don't really see this as better, TBH the whole scaling
+ reporting step-size thing feels significantly worse then
just updating brightness_max.

And then we would need to report step-size = 0 to report no
backlight device is available yet, which also feels worse then
using brightness_max=0 to indicate lack of brightness control.

Regards,

Hans


> 1) For now I, intend to extend this with detection of Apple GMUX and
>    NVIDIA_WMI_EC_BACKLIGHT support
>