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@xxxxxxxxxx> 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@xxxxxxxxxx> 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 >
