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. Regards, Hans 1) For now I, intend to extend this with detection of Apple GMUX and NVIDIA_WMI_EC_BACKLIGHT support
