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. > > 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? Alex > > > For instance if we can guarantee that the min level won't turn the screen > > completely off we could make the range start from 1 instead of 0. > > Or allow -1 to mean "minimum value, maybe completely off". > > Right, the problem is we really don't know and when the range is > e.g. 0-65535 then something like 1 will almost always still just > turn the screen completely off. There will be a value of say like > 150 or some such which is then the actual minimum value to still > get the backlight to light up at all. The problem is we have > no clue what the actual minimum is. And if the PWM output does > not directly drive the LEDs but is used as an input for some > LED backlight driver chip, that chip itself may have a lookup > table (which may also take care of doing perceived brightness > mapping) and may guarantee a minimum backlight even when given > a 0% duty cycle PWM signal... > > This prop is sort of orthogonal to the generic change to > drm_connector props, so we could also do this later as a follow up > change. At a minimum when I code this up this should be in its > own commit(s) I believe. > > But I do think having this will be useful for the above > GNOME example. > > >> bl_brightness_control_method: ro, enum, possible values: > >> none: The GPU driver expects brightness control to be provided by another > >> driver and that driver has not loaded yet. > >> unknown: The underlying control mechanism is unknown. > >> pwm: The brightness property directly controls the duty-cycle of a PWM > >> output. > >> firmware: The brightness is controlled through firmware calls. > >> DDC/CI: The brightness is controlled through the DDC/CI protocol. > >> gmux: The brightness is controlled by the GMUX. > >> Note this enum may be extended in the future, so other values may > >> be read, these should be treated as "unknown". > >> > >> When brightness control becomes available after being reported > >> as not available before (bl_brightness_control_method=="none") > >> a uevent with CONNECTOR=<connector-id> and > >> > >> PROPERTY=<bl_brightness_control_method-id> will be generated > >> > >> at this point all the properties must be re-read. > >> > >> When/once brightness control is available then all the read-only > >> properties are fixed and will never change. > >> > >> Besides the "none" value for no driver having loaded yet, > >> the different bl_brightness_control_method values are intended for > >> (userspace) heuristics for such things as the brightness setting > >> linearly controlling electrical power or setting perceived brightness. > > > > Can you elaborate? I don't know enough about brightness control to > > understand all of the implications here. > > So after sending this email I was already thinking myself that this > one might not be the best idea. Another shortcoming of the current > backlight API is that it does not let userspace know if the > number is a linear control of the time the LEDs are on vs off > (assuming a LED backlight) or if some component already uses a > lookup table to make 0-100% be more of a linear scale in the > human perception, which is very much non linear. See e.g.: > > https://www.sciencedirect.com/topics/computer-science/perceived-brightness > > "refers to the perceived amount of light coming from self-luminous sources" > "Perceived brightness is a very nonlinear function of the amount of light emitted by a lamp." > > The problem is that at the kernel level we have no idea if > we are controlling "the amount of light emitted" or > perceived brightness and it would be sorta nice for userspace > to know. So the idea here is/was to allow userspace to make some > educated guess here. E.g. a bl_brightness_control_method of "PWM" > hints strongly at "the amount of light emitted" (but this is > not true 100% of the time). ATM userspace does not do any > "perceived brightness" curve correction so for the first > implementation of moving brightness control to drm properties > I believe it might be better to just park the whole > bl_brightness_control_method propery idea. > > Which would leave the problem of communicating the control_method=="none" > case but we can just use bl_brightness_max == 0 for that. > > Regards, > > Hans > > >
