Re: KMS backlight ABI proposition

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 15372 bytes --]

Cc'ing Daniel, Lee and Jingoo, the backlight subsystem maintainers.

On 17/02/17 14:58, Martin Peres wrote:
> Hey everyone,
> 
> We have been working towards exposing the backlight as a KMS property
> instead of relying on the backlight drivers. We have CC:ed the people we
> have found to be the more likely to be interested in the discussion but
> please add everyone you think would have some experience with this issue.

First off, I welcome this effort, because I think we really need
something like this.

However, I had attempted to tackle this around the time of FOSDEM 2014
and even had a working prototype, but when discussing this on IRC there
were a couple of senior DRM maintainers that objected to the idea and
said it couldn't be done. I don't remember the exact details, but I
think the objection was that my prototype was hooking up backlights
essentially from devicetree links (via panels, in that case) and at the
time the general opinion had been that it couldn't be done on x86 and
therefore we weren't supposed to have it at all.

You might want to check with Dave and Daniel explicitly to make sure
they're on board this time. It seems like the general opinion might have
changed in the meantime, so I'll keep my fingers crossed.

> == Introduction ==
> 
> We are trying to bring the same level of support for the backlight on
> both the xf86-video-intel and -modesetting DDX.
> 
> Looking into the situation of the backlight, we identified these
> problems which are almost show-stoppers for -modesetting and wayland
> compositors:
> 
>  - There is no mapping between the backlight driver and DRM-connectors.
> This means that, in case there are multiple backlight drivers, the
> userspace has to have knowledge of the machine to know which driver
> should be used. See the priority list for the intel driver [0].

This is a fairly trivial issue for ARM (and other device tree based
architectures) because we explicitly link the backlight to the panel
already. The prototype that I had would look at the panel and see if
brightness support was implemented and expose that as a connector
property.

>  - The luminance curve of the backlight drivers is not specified, which
> can lead to a bad user experience: Little changes in the highest levels
> but drastic changes in the low levels.

I think this is something that the backlight framework should specify.
It's fairly unlikely that you could get this right every time in the
display driver. I suppose for something like x86 this might actually
work, but I don't think it'll work on something like ARM where you'd
have to encode this based on what backlight driver was actually being
used.

>  - Writing to the backlight driver still requires root rights. Given
> that the xserver and wayland compositors are now running root-less, this
> means we would need a complex dance involving a setuid helper [1].
> 
> Hans de Goede has already given a presentation about these issues at
> XDC2014. The slides are a good read[2].
> 
> [0]
> https://cgit.freedesktop.org/xorg/driver/xf86-video-intel/tree/src/backlight.c#n259
> 
> 
> [1]
> https://cgit.freedesktop.org/xorg/driver/xf86-video-intel/tree/src/backlight.c#n348
> 
> 
> [2]
> https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/backlight.pdf
> 
> == Proposal ==
> 
> Since David Hermann already worked on this and proposed what I consider
> being greats foundations for building towards a solution addressing the
> issues above, I will just ask you to read his original words:
> 
> https://lists.freedesktop.org/archives/dri-devel/2014-September/067984.html
> 
> == Open issues ==
> 
> Here are the open issues we have identified with the solution proposed
> by David:
> 
>   1) Backlight device interoperability: How far should we support
>      mixing the backlight device and brightness property? Should it be
>      unidirectional or bi-directional? What about the start-up value
>      exposed by the brightness property?
> 
>   2) How many steps should be exposed: fixed or driver-dependent?
> 
>   3) Expected output curve: power? luminance? Simply monotonically
>      increasing?
> 
>   4) Should the userspace be able to turn off the backlight? If so, how
>      should it do it? What can we do to let the userspace distinguish
>      between backlight off or on?
> 
>   5) Should we expose to the userspace what is the current backlight
>      power?
> 
> Here is our current point of view on the matter:
> 
> === 1) Backlight device interoperability ===
> 
> Since we need to keep backward compatibility of the backlight, we have
> to keep the current backlight drivers.
> 
> Here are possible options:
> 
>  - Exclusive access: Unregister a backlight device when the drm
> brightness property is requested/used;
>  - Unidirectional access: When writing to the backlight property, update
> the backlight device;
>  - Bi-directional access: Propagate back changes from the backlight
> device to the property's value.
> 
> Being bi-directional would be of course the best, but this requires that
> both drivers have the same number of steps, otherwise, we may write a
> value to the property, but get another one when reading it right after,
> due to the non-bijective nature of the transformation.

I think this is the only reasonable choice. As has been discussed
elsewhere trying to map different ranges always has drawbacks. Also its
really the backlight driver's job to expose the number of brightness
levels that make sense given the specific hardware.

There had been a fairly extensive discussion back when we devised the
device tree bindings for the pwm-backlight driver that many ARM-based
boards use. At the time pwm-backlight was only supporting a contiguous
range of values, usually from 0 to 255. But it turned out that often
this was suboptimal because of the non-linear relationship brightness
to power. What we ended up doing is allow each board to define a set
of levels, with the idea that some characterization step in production
would yield the appropriate values to make brightness scale linearly
over the range of values.

Of course I've seen a number of device trees that then went and simply
added a contiguous range of brightness levels, from 0 to 255. I have no
idea why that happened. Perhaps people were just too accustomed to echo
255 to the sysfs brightness attribute in order to set full brightness.
Or perhaps there was in those cases existing userspace that assumed the
brightness would always go from 0 to 255.

In any case, I think it makes sense to expose the full flexibility of
the hardware via the backlight driver and then simply proxy that with a
KMS connector property. If we define this properly from the get-go then
userspace has no excuse to do crazy things. The obvious thing to do is
to grab the range and provide a user interface that uses the range. If
backlight hardware can only adjust brightness in 5 steps, then the UI
(slider, spinner, whatever) should expose just that: 5 levels with a
step size of 1.

> But since David already had to add an in-kernel interface to access the
> backlight devices [0], we could add capabilities to the drivers while
> keeping the backward compatibility.
> 
> From the in-kernel interface, it is already possible to turn on and off
> the backlight for sure (when supported, but this is also reported
> properly). However, what is not supported is to know what the value 0
> means (lowest setting possible but not turned off, or no power at all).
> 
> It was brought up that we could simply not allow the backlight to be
> turned off, and just request DPMS to reach this state. However, I do not
> think it is a good idea as some panels (like the one from the OLPC)
> switch to e-paper mode when the backlight is set to 0 and are perfectly
> readable.

I think this is something that the backlight API should be stricter
about. Some drivers seem to equate brightness 0 with backlight off,
while others don't. That's really confusing in my opinion. Since we
already have separate properties for both (and reflected in sysfs),
I think the most sensible thing to do would be to formalize this in
the backlight subsystem.

> I would suggest we design an interface that will allow good drivers to
> expose as many features as possible, but yet gracefully degrade if
> information is not present.

Agreed, this sounds like a good idea.

> Over time, drivers will improve to expose information about the
> platform, and the user experience will improve as a result.
> 
> [0]
> https://lists.freedesktop.org/archives/dri-devel/2014-September/067987.html

Reading some of David's patches it looks like there's some potential to
unify the API for all users. Currently it seems geared mostly towards
x86 (or desktop in general) use cases, but for ARM devices it could be
made to tie into device tree infrastructure for lookup. I think it'd be
worth considering APIs similar to what we have in other subsystems,
i.e.:

	struct backlight_device *backlight_get(struct device *dev,
					       const char *consumer);
	void backlight_put(struct backlight_device *backlight);

It's a fairly proven set of functions and the implementations share many
commonalities. We do the same for GPIOs, regulators, PWMs and many other
resources.

I volunteer to help out with this, so feel free to Cc me on patches.

> ===  5) Exposing the current backlight power?  ===
> 
> The backlight_current interface in the backlight devices is meant to
> expose the currently-used backlight value, regardless of the wanted
> value that should be used when the backlight is not off.
> 
> My current stance on this is that this should not be needed. The
> userspace should describe the intent of the user (wanted backlight
> level) and trust the KMS property to turn off the backlight when
> entering DPMS.

I'm not exactly sure how this last part of the sentence relates to the
backlight_current interface. But I generally agree that the interface
should be designed in a way that doesn't require reading back the
current value to determine what was configured.

If we expose all values that the backlight driver exposes, then there
should be no reason not to trust that when we set a value within the
given range, it will be the value that's actually going to be
programmed.

> == Current KMS ABI proposal ==
> 
> The current ABI proposal has mostly been proposed by Jani Nikula, as a
> result of his experience and our discussions.
> 
> It takes the following approach:
> 
>  - Fixed number of steps (I think we should change it to expose the same
> number of steps)

+1

>  - Uni-directional: KMS -> backlight

I don't think that'll work very well. At the very least we'll need to
read back the initial brightness. And then there's the issue of these
brightness values going out of sync. Both of which are likely going to
result in very annoying user interface behaviour. Bidirectional sounds
like the best to me, and I don't think it would be very difficult to
implement, either.

>  - Do not deal yet with 3) and 4): I have ideas, but I have been
> procrastinating long-enough to send this email and we already have much
> to discuss!

I think we should push 3) into backlight drivers where this knowledge
should exist. I suspect there could be edge cases where the driver does
not expose a sensible range (think 0-65535, with no obvious relation to
brightness at all). In those cases I think we could possible overlay a
backlight driver that makes something useful out of those values,
possibly using device-specific knowledge.

>  - Does not expose the current backlight power as we want to let the
> kernel deal with DPMS on its own

I think DPMS and backlight power are separate things. As was mentioned
elsewhere there are cases where it makes sense to disable the backlight
but keep the display pipeline running. It's probably okay to not support
these right away, but I think we need to be careful to design an
interface that will allow them to be supported later on.

> === ABI proposal ===
> 
> The brightness property MUST have values 0...100 inclusive.
> 
> The display brightness MUST be a monotonically increasing function of
> the brightness property.
> 
> Brightness property value 1 MUST mean the minimum supported visible
> brightness.
> 
> Brightness property value 100 MUST mean the maximum supported
> brightness.
> 
> Brightness property value 0 SHOULD mean backlight off or equivalent for
> non-backlight brightness adjustment, typically completely
> black. Brightness property value 0 MUST NOT switch the display or pipe
> off [1].

Why special case 0? Couldn't we simply add a second property for the
power? We could for example have a boolean "backlight" property that
userspace can turn "On" or "Off" and a "brightness" property which
controls the backlight brightness.

> If the hardware is not capable of supporting zero brightness, and the
> driver knows this, value 0 MUST be equal to value 1.
> 
> If the driver does not know whether the hardware is capable of
> supporting zero brightness, the driver SHOULD err on the side of 0 not
> being off rather than 1 meaning off. In this case, value 0 is likely
> different from value 1, and the minimum brightness can only be reached
> via property value 0 [2].

Yeah, this definition is really complicated. I've been reading it three
times and I'm not sure I fully understand it. =)

For my understanding, with those e-paper displays, what would be the
difference between brightness 0 and backlight off? Would backlight off
actually turn off the display? So that no "ink" would even be visible?
For panels we typically have enabled and disabled states, where the
disabled state, among other things, also turns off the backlight. So
for panels we do have one more level of abstraction. For e-paper-able
displays we could simply have a full range of brightness values, with
0 meaning e-paper mode and panel off meaning, well, panel off.

Alternatively, e-paper mode could be the same as backlight off, since
obviously there is no backlight anymore, in which case there wouldn't be
a need to differentiate the special case for 0.

Thierry

> If the brightness gets changed outside of the property interface,
> reading the property value MAY be out of sync with the actual brightness
> [3].
> 
> [1] Must be able to support displays which are visible even with the
> backlight switched off.
> 
> [2] The main downside corner case with this is that if the driver
> doesn't know whether it can switch off the backlight, 0 might end up
> meaning the minimum visible, and 1 is the second lowest visible, and
> with a userspace that avoids black display, the user can't use the
> lowest brightness setting.
> 
> [3] This is not unlike the "brightness" property in the backlight class
> sysfs interface. The intention is that the drm interface does not have
> an equivalent of "actual_brightness".
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel