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 I just realized another important reason why we really
need to the support for this to be dynamic.

The reason why I've started looking into this (again) is because
Sebastian Wick has been looking into HDR support and he inquired
about support the brightness of external monitors through DDC/CI
and while we were discussing that a series was posted to add
DDC/CI support to /sys/class/backlight, which I nacked because
that would make the backlight-dev <-> connector mapping problem
a lot bigger:
https://lore.kernel.org/lkml/20220403230850.2986-1-yusisamerican@gmail.com/

But there clearly is demand for offering brightness control over
DDC/CI and the intend of this proposal is to also cover that.

But external devices can be plugged/unplugged and then DDC/CI
support may come and go and/or if a different monitor gets
plugged in the range may change. So we need support for brightness
control going away (brightness_max becomes 0) and for the range
changing on the fly regardless of the whole internal panel
discussion.

At least we must support this to support DDC/CI which at least
for me is an explicit goal here.

Regards,

Hans


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