On Fri, Apr 8, 2022 at 10:56 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > Hi, > > On 4/8/22 16:08, Alex Deucher wrote: > > On Fri, Apr 8, 2022 at 4:07 AM Daniel Vetter <daniel@xxxxxxxx> 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. > >> > >> - 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? > > Right we certainly need to define the behavior of the new API > clearly, so that userspace does not misuse / misinterpret it. > > The intend is for brightness=0 to mean minimum brightness > to still be able to see what is on the screen. But the problem > is that in many cases the GPU driver directly controls a PWM > output, no minimum PWM value is given in the video BIOS tables > and actually setting the PWM to 0% dutycycle turns off the > screen. Sure. So have the GPU driver map 0 to some valid minimum if that is the case or might be the case. If bugs come up, we can add quirks in the GPU drivers. > > So we can only promise a best-effort to make brightness=0 > mean minimum brightness, combined with documenting that it > may turn off the backlight and that userspace _must_ never > depend on it turning off the backlight. > > Also note that setting a direct PWM output to duty-cycle 0% > does not guarantee that the backlight goes off, this may be > an input for a special backlight LED driver IC like the > TI LP855x series which can have an internal lookup > table causing it to actually output a minimum brightness > when its PWM input is at 0% dutycycle. So this is a case > where we just don't get enough info from the fw/hw to be able > to offer the guarantees which we would like to guarantee. So set it to a level we can guarantee can call it 0. If we have the flag we are just punting on the problem in my opinion. The kernel driver would seem to have a better idea what is a valid minimum than some arbitrary userspace application. Plus then if we need a workaround for what is the minimum valid brightness, we can fix it one place rather than letting every application try and fix it. Alex > > Regards, > > Hans > > > > >
