KMS backlight ABI proposition

KMS backlight ABI proposition

[Martin Peres]

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.

== 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].

  - 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.

  - 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.

Uni-directional would work in all cases, with the caveat that mixing 
calls to the KMS property and the backlight device will not be supported 
(changes mades through the sysfs interface of the backlight driver will 
not be reflected in the KMS property). At boot time, we should however 
initialize the value of the backlight property with a value close to 
what is currently set in the backlight driver.

Giving exclusive access does not sound very good to me, as it would be 
hard for the userspace to deal with disappearing drivers...

=== 2) How many steps should be exposed ===

If the KMS property exposes the same number of steps as the backlight 
driver, it allows us to get a bijective function between the two 
interfaces, and allow a bi-directional communication. The downside of 
this is that it forces the userspace to deal with a variable number of 
steps which can range from 4 to 1k+. Also, the userspace would be able 
to handle the case where there are less steps than it would like to expose.

If the KMS property exposes a fixed number of steps (say 100), it 
becomes easy for the userspace to express the wanted brightness. 
However, on drivers exposing less than these 100 steps, we cannot 
guarantee that any change in the value will produce any change. If there 
is only one possible value (on or off), the user may be trying the 
change the brightness, a GUI would show what is the expected backlight 
state, but no change in the luminance would be seen, which is pretty bad.

=== 3) and 4) ===

These issues are not handled at all by the backlight device sysfs interface.

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 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.

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

===  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.

== 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)
  - Uni-directional: KMS -> backlight
  - 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!
  - Does not expose the current backlight power as we want to let the 
kernel deal with DPMS on its own

=== 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].

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].

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

Re: KMS backlight ABI proposition

[Martin Peres]

+plasma-devel, as suggested by Martin Gräßlin.


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.
>
> == 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].
>
>  - 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.
>
>  - 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.
>
> Uni-directional would work in all cases, with the caveat that mixing
> calls to the KMS property and the backlight device will not be supported
> (changes mades through the sysfs interface of the backlight driver will
> not be reflected in the KMS property). At boot time, we should however
> initialize the value of the backlight property with a value close to
> what is currently set in the backlight driver.
>
> Giving exclusive access does not sound very good to me, as it would be
> hard for the userspace to deal with disappearing drivers...
>
> === 2) How many steps should be exposed ===
>
> If the KMS property exposes the same number of steps as the backlight
> driver, it allows us to get a bijective function between the two
> interfaces, and allow a bi-directional communication. The downside of
> this is that it forces the userspace to deal with a variable number of
> steps which can range from 4 to 1k+. Also, the userspace would be able
> to handle the case where there are less steps than it would like to expose.
>
> If the KMS property exposes a fixed number of steps (say 100), it
> becomes easy for the userspace to express the wanted brightness.
> However, on drivers exposing less than these 100 steps, we cannot
> guarantee that any change in the value will produce any change. If there
> is only one possible value (on or off), the user may be trying the
> change the brightness, a GUI would show what is the expected backlight
> state, but no change in the luminance would be seen, which is pretty bad.
>
> === 3) and 4) ===
>
> These issues are not handled at all by the backlight device sysfs
> interface.
>
> 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 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.
>
> 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
>
> ===  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.
>
> == 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)
>  - Uni-directional: KMS -> backlight
>  - 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!
>  - Does not expose the current backlight power as we want to let the
> kernel deal with DPMS on its own
>
> === 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].
>
> 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].
>
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS backlight ABI proposition

[Daniel Thompson]

On 20/02/17 12:46, Martin Peres wrote:
> +plasma-devel, as suggested by Martin Gräßlin.

This reply also adds the current drivers/video/backlight maintainers (I 
forwarded the original mail to them separately, so I've been pretty 
brutal with the delete key when quoting the original mail).


> On 17/02/17 14:58, Martin Peres wrote:
>> === 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 don't accept that bi-directional transfer requires the step range to 
be the same. Isn't all that is required is acceptance that both sides 
maintain a copy of the current value in their own number range and that 
if X is written to then Y may change value (i.e. when mapping between 
0..100 and 0..10 then if 0..100 is at 11 and 0..10 gets 1 written then 
0..100 is allowed to change to 10).

I'd note also that the mechanisms inside backlight to support 
sysfs_notify would mean *implementing* bi-directional comms isn't too 
bloated even if the two sides used different number ranges.


>> Uni-directional would work in all cases, with the caveat that mixing
>> calls to the KMS property and the backlight device will not be supported
>> (changes mades through the sysfs interface of the backlight driver will
>> not be reflected in the KMS property). At boot time, we should however
>> initialize the value of the backlight property with a value close to
>> what is currently set in the backlight driver.
>>
>> Giving exclusive access does not sound very good to me, as it would be
>> hard for the userspace to deal with disappearing drivers...

+1  :-)


>> == 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)

Fixing a large number of steps over an inflexible (lets say 8 level) 
backlight device creates a new problem. User actions to 
increase/decrease the backlight don't work unless the userspace knows 
the hardware step size...

The 0..100 proposal below will encourage the userspace to implement 
hotkeys that jump by 9 (because 0 is reserved with a special meaning). 
and thus there will be deadspots where the hot key has no effect.


>>  - Uni-directional: KMS -> backlight

See above.


>>  - 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!

Do any of those ideas involve adding *new* API to provide information to 
userspace to help it correct the curves (e.g. somewhat like ALSA)?

It's not that I object to such an approach but I consider it pointless 
to present fixed range brightness levels if the userspace were to end up 
responsible for curve correction.


>>  - Does not expose the current backlight power as we want to let the
>> kernel deal with DPMS on its own
 >>
>> === ABI proposal ===
>>
>> The brightness property MUST have values 0...100 inclusive.

I'm somewhat unconvinced by re-ranging the hardware capability but if 
this is the way we want to go perhaps consider -1..100 as the range. 
There's a risk of bikeshedding here but -1 is a more obvious "special" 
value and it offers more flexibility for natural hotkey strides.


>> 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].
>>
>> 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].
>>
>> If the brightness gets changed outside of the property interface,
>> reading the property value MAY be out of sync with the actual brightness
>> [3].

Already discussed above.


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

Re: KMS backlight ABI proposition

[Jani Nikula]

On Mon, 20 Feb 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>>> === 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 don't accept that bi-directional transfer requires the step range to 
> be the same. Isn't all that is required is acceptance that both sides 
> maintain a copy of the current value in their own number range and that 
> if X is written to then Y may change value (i.e. when mapping between 
> 0..100 and 0..10 then if 0..100 is at 11 and 0..10 gets 1 written then 
> 0..100 is allowed to change to 10).
>
> I'd note also that the mechanisms inside backlight to support 
> sysfs_notify would mean *implementing* bi-directional comms isn't too 
> bloated even if the two sides used different number ranges.

I question the need and usefulness of bi-directional access, and I
question it being "the best". The end goal is to use the connector
property exclusively, and deprecate the sysfs API. If you choose to use
the connector property, you should steer clear from the sysfs. That's
part of the deal here.

The sysfs will still work as ever, it won't break or regress or go away
anytime soon, but the ABI and contract for the connector property will
be, "if you touch the sysfs while using the connector property, you
might get unexpected results reading back the property".

There *are* going to be subtle bugs with the simultaneous operation, and
I know I don't want to be in the receiving end of those bugs.

Raise your hands, who wants to deal with them? Who thinks it's worth it?

>>> 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)
>
> Fixing a large number of steps over an inflexible (lets say 8 level)
> backlight device creates a new problem. User actions to
> increase/decrease the backlight don't work unless the userspace knows
> the hardware step size...

Many of the ACPI backlight interfaces have a limited number of steps,
such as 8 or 16.

However, at least for i915 native backlight, we might *theoretically*
have, say, 5000 steps. But we might have no clue how many user
perceivable distinct steps there are.

> The 0..100 proposal below will encourage the userspace to implement 
> hotkeys that jump by 9 (because 0 is reserved with a special meaning). 
> and thus there will be deadspots where the hot key has no effect.

One brainstormed idea was to provide a way to increase/decrease the
brightness by a user perceivable margin or N%, whichever is the bigger
change. I don't think we explored that in depth, or how feasible that is
with the properties. It might not solve everything, but it could solve
one class of problems with expanding a limited hardware range to 0..100.

>>>  - Uni-directional: KMS -> backlight
>
> See above.
>
>
>>>  - 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!
>
> Do any of those ideas involve adding *new* API to provide information to 
> userspace to help it correct the curves (e.g. somewhat like ALSA)?
>
> It's not that I object to such an approach but I consider it pointless 
> to present fixed range brightness levels if the userspace were to end up 
> responsible for curve correction.

One of the ideas we've discussed is having a property to adjust the
curve in kernel. If the driver knows parameters of the backlight, it
could populate the curve with the information it has, but it would allow
the userspace to adjust or replace it. The idea is that the userspace
could then treat the brightness property as linear wrt perceived
brightness. ("Perceived brightness" is kind of vague too, but let's not
go there just yet.)

>>>  - Does not expose the current backlight power as we want to let the
>>> kernel deal with DPMS on its own
>  >>
>>> === ABI proposal ===
>>>
>>> The brightness property MUST have values 0...100 inclusive.
>
> I'm somewhat unconvinced by re-ranging the hardware capability but if 
> this is the way we want to go perhaps consider -1..100 as the range. 
> There's a risk of bikeshedding here but -1 is a more obvious "special" 
> value and it offers more flexibility for natural hotkey strides.

There some benefits for "re-ranging" the hardware range:

* It makes sense for hardware ranges that have far more steps than can
  be perceived. Why expose 5000 steps when you can perceive, say, a
  couple of hundred levels, if that. And the userspace will only use
  maybe ten steps.

* Some PWM based backlight allow adjusting the PWM modulation
  frequency. It could be done on the fly. It would be awkward to change
  the max on the fly; not sure if it's even possible for properties.

* There's the idea of letting userspace re-associate the brightness
  properties with the underlying hardware. The max might change. See
  previous point. Any solution must address this.

We can bikeshed the meaning of 0 or -1, I don't mind. The point is, we
need to define what the drivers should aim for, with the potentially
limited information they have available, to provide as smooth and
unified an experience as possible.

One benefit of -1 is that we might get away with adding that as a
special case later on, if we define 0 properly. And if the drivers know
they don't support off, they could have range 0..100 instead.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS backlight ABI proposition

[Martin Peres]

On 22/02/17 17:05, Jani Nikula wrote:
> On Mon, 20 Feb 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>>>> === 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 don't accept that bi-directional transfer requires the step range to
>> be the same. Isn't all that is required is acceptance that both sides
>> maintain a copy of the current value in their own number range and that
>> if X is written to then Y may change value (i.e. when mapping between
>> 0..100 and 0..10 then if 0..100 is at 11 and 0..10 gets 1 written then
>> 0..100 is allowed to change to 10).
>>
>> I'd note also that the mechanisms inside backlight to support
>> sysfs_notify would mean *implementing* bi-directional comms isn't too
>> bloated even if the two sides used different number ranges.
> I question the need and usefulness of bi-directional access, and I
> question it being "the best". The end goal is to use the connector
> property exclusively, and deprecate the sysfs API. If you choose to use
> the connector property, you should steer clear from the sysfs. That's
> part of the deal here.
>
> The sysfs will still work as ever, it won't break or regress or go away
> anytime soon, but the ABI and contract for the connector property will
> be, "if you touch the sysfs while using the connector property, you
> might get unexpected results reading back the property".
>
> There *are* going to be subtle bugs with the simultaneous operation, and
> I know I don't want to be in the receiving end of those bugs.
>
> Raise your hands, who wants to deal with them? Who thinks it's worth it?
>
>>>> 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)
>> Fixing a large number of steps over an inflexible (lets say 8 level)
>> backlight device creates a new problem. User actions to
>> increase/decrease the backlight don't work unless the userspace knows
>> the hardware step size...
> Many of the ACPI backlight interfaces have a limited number of steps,
> such as 8 or 16.
>
> However, at least for i915 native backlight, we might *theoretically*
> have, say, 5000 steps. But we might have no clue how many user
> perceivable distinct steps there are.
>
>> The 0..100 proposal below will encourage the userspace to implement
>> hotkeys that jump by 9 (because 0 is reserved with a special meaning).
>> and thus there will be deadspots where the hot key has no effect.
> One brainstormed idea was to provide a way to increase/decrease the
> brightness by a user perceivable margin or N%, whichever is the bigger
> change. I don't think we explored that in depth, or how feasible that is
> with the properties. It might not solve everything, but it could solve
> one class of problems with expanding a limited hardware range to 0..100.
>
>>>>   - Uni-directional: KMS -> backlight
>> See above.
>>
>>
>>>>   - 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!
>> Do any of those ideas involve adding *new* API to provide information to
>> userspace to help it correct the curves (e.g. somewhat like ALSA)?
>>
>> It's not that I object to such an approach but I consider it pointless
>> to present fixed range brightness levels if the userspace were to end up
>> responsible for curve correction.
> One of the ideas we've discussed is having a property to adjust the
> curve in kernel. If the driver knows parameters of the backlight, it
> could populate the curve with the information it has, but it would allow
> the userspace to adjust or replace it. The idea is that the userspace
> could then treat the brightness property as linear wrt perceived
> brightness. ("Perceived brightness" is kind of vague too, but let's not
> go there just yet.)

One way would be to just expose a table containing trip points with
known (raw_value, luminance %) points. The userspace may just
allow changing between them or may want to interpolate if it needs
more trip points. This way, we don't have to deal with any curves in
the kernel space.

If this table were user-writable, this would allow people to calibrate
their monitor, as wanted (using a colourhug, or whatever).

>
>>>>   - Does not expose the current backlight power as we want to let the
>>>> kernel deal with DPMS on its own
>>   >>
>>>> === ABI proposal ===
>>>>
>>>> The brightness property MUST have values 0...100 inclusive.
>> I'm somewhat unconvinced by re-ranging the hardware capability but if
>> this is the way we want to go perhaps consider -1..100 as the range.
>> There's a risk of bikeshedding here but -1 is a more obvious "special"
>> value and it offers more flexibility for natural hotkey strides.
> There some benefits for "re-ranging" the hardware range:
>
> * It makes sense for hardware ranges that have far more steps than can
>    be perceived. Why expose 5000 steps when you can perceive, say, a
>    couple of hundred levels, if that. And the userspace will only use
>    maybe ten steps.
>
> * Some PWM based backlight allow adjusting the PWM modulation
>    frequency. It could be done on the fly. It would be awkward to change
>    the max on the fly; not sure if it's even possible for properties.
>
> * There's the idea of letting userspace re-associate the brightness
>    properties with the underlying hardware. The max might change. See
>    previous point. Any solution must address this.

Ah, right! I had forgotten about this!

What I said was that since it is a rare event, we could issue
a hotplug event to work around this issue. The userspace would
be forced to re-probe.

>
> We can bikeshed the meaning of 0 or -1, I don't mind. The point is, we
> need to define what the drivers should aim for, with the potentially
> limited information they have available, to provide as smooth and
> unified an experience as possible.
>
> One benefit of -1 is that we might get away with adding that as a
> special case later on, if we define 0 properly. And if the drivers know
> they don't support off, they could have range 0..100 instead.

Yeah, -1 is a good idea.

>
>
> BR,
> Jani.
>
>

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

Re: KMS backlight ABI proposition

[Hans de Goede]

Hi,

On 22-02-17 16:05, Jani Nikula wrote:
> On Mon, 20 Feb 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>>>> === 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 don't accept that bi-directional transfer requires the step range to
>> be the same. Isn't all that is required is acceptance that both sides
>> maintain a copy of the current value in their own number range and that
>> if X is written to then Y may change value (i.e. when mapping between
>> 0..100 and 0..10 then if 0..100 is at 11 and 0..10 gets 1 written then
>> 0..100 is allowed to change to 10).
>>
>> I'd note also that the mechanisms inside backlight to support
>> sysfs_notify would mean *implementing* bi-directional comms isn't too
>> bloated even if the two sides used different number ranges.
>
> I question the need and usefulness of bi-directional access, and I
> question it being "the best". The end goal is to use the connector
> property exclusively, and deprecate the sysfs API. If you choose to use
> the connector property, you should steer clear from the sysfs. That's
> part of the deal here.

My first thought was that your proposal is reasonable, but on second
thought I foresee trouble here with e.g. the backlight level save / restore
code in systemd still using the sysfs interface, while the desktop
environment has moved on to the property, I believe we really need to
do some sort of bi-directional syncing here ...

> The sysfs will still work as ever, it won't break or regress or go away
> anytime soon, but the ABI and contract for the connector property will
> be, "if you touch the sysfs while using the connector property, you
> might get unexpected results reading back the property".
>
> There *are* going to be subtle bugs with the simultaneous operation, and
> I know I don't want to be in the receiving end of those bugs.
>
> Raise your hands, who wants to deal with them? Who thinks it's worth it?
>
>>>> 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)
>>
>> Fixing a large number of steps over an inflexible (lets say 8 level)
>> backlight device creates a new problem. User actions to
>> increase/decrease the backlight don't work unless the userspace knows
>> the hardware step size...
>
> Many of the ACPI backlight interfaces have a limited number of steps,
> such as 8 or 16.
>
> However, at least for i915 native backlight, we might *theoretically*
> have, say, 5000 steps. But we might have no clue how many user
> perceivable distinct steps there are.
>
>> The 0..100 proposal below will encourage the userspace to implement
>> hotkeys that jump by 9 (because 0 is reserved with a special meaning).
>> and thus there will be deadspots where the hot key has no effect.
>
> One brainstormed idea was to provide a way to increase/decrease the
> brightness by a user perceivable margin or N%, whichever is the bigger
> change. I don't think we explored that in depth, or how feasible that is
> with the properties. It might not solve everything, but it could solve
> one class of problems with expanding a limited hardware range to 0..100.
>
>>>>  - Uni-directional: KMS -> backlight
>>
>> See above.
>>
>>
>>>>  - 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!
>>
>> Do any of those ideas involve adding *new* API to provide information to
>> userspace to help it correct the curves (e.g. somewhat like ALSA)?
>>
>> It's not that I object to such an approach but I consider it pointless
>> to present fixed range brightness levels if the userspace were to end up
>> responsible for curve correction.
>
> One of the ideas we've discussed is having a property to adjust the
> curve in kernel. If the driver knows parameters of the backlight, it
> could populate the curve with the information it has, but it would allow
> the userspace to adjust or replace it. The idea is that the userspace
> could then treat the brightness property as linear wrt perceived
> brightness. ("Perceived brightness" is kind of vague too, but let's not
> go there just yet.)
>
>>>>  - Does not expose the current backlight power as we want to let the
>>>> kernel deal with DPMS on its own
>>  >>
>>>> === ABI proposal ===
>>>>
>>>> The brightness property MUST have values 0...100 inclusive.
>>
>> I'm somewhat unconvinced by re-ranging the hardware capability but if
>> this is the way we want to go perhaps consider -1..100 as the range.
>> There's a risk of bikeshedding here but -1 is a more obvious "special"
>> value and it offers more flexibility for natural hotkey strides.
>
> There some benefits for "re-ranging" the hardware range:
>
> * It makes sense for hardware ranges that have far more steps than can
>   be perceived. Why expose 5000 steps when you can perceive, say, a
>   couple of hundred levels, if that. And the userspace will only use
>   maybe ten steps.
>
> * Some PWM based backlight allow adjusting the PWM modulation
>   frequency. It could be done on the fly. It would be awkward to change
>   the max on the fly; not sure if it's even possible for properties.
>
> * There's the idea of letting userspace re-associate the brightness
>   properties with the underlying hardware. The max might change. See
>   previous point. Any solution must address this.
>
> We can bikeshed the meaning of 0 or -1, I don't mind. The point is, we
> need to define what the drivers should aim for, with the potentially
> limited information they have available, to provide as smooth and
> unified an experience as possible.
>
> One benefit of -1 is that we might get away with adding that as a
> special case later on, if we define 0 properly. And if the drivers know
> they don't support off, they could have range 0..100 instead.

I really believe that we need to define the ABI as 0 meaning minimal
brightness which keeps the screen readable (which for the epaper
example would be no brightness, but normally would be some minimal
level).

Yes we do not get this right in some cases, but let at least define
it properly in the ABI. Add a fat disclaimer for all I care that
in some cases the driver is unable to guarantee this, but lets
clearly define what 0 means and then try to get as much drivers
to adhere to that as possible.

As for -1 meaning turning stuff off, I'm against that, on/off
vs brightness setting really are 2 almost orthogonal controls,
in many cases in hardware they are truely orthogona, with both
an enable pin as well as a pwm input for the brightness level,
and driving the enable low will get the pwm input to effectively
be ignored.

Regards,

Hans


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

Re: KMS backlight ABI proposition

[Jani Nikula]

On Wed, 22 Feb 2017, Hans de Goede <hdegoede@redhat.com> wrote:
> My first thought was that your proposal is reasonable, but on second
> thought I foresee trouble here with e.g. the backlight level save / restore
> code in systemd still using the sysfs interface, while the desktop
> environment has moved on to the property, I believe we really need to
> do some sort of bi-directional syncing here ...

FWIW I think systemd should have no business changing the backlight. The
save/restore should belong in the desktop environments... But I think we
could dodge this one by having the initial sysfs value synced back to
the property on initialization, but not otherwise. The systemd stuff,
IIRC, happens before/after X.

>> We can bikeshed the meaning of 0 or -1, I don't mind. The point is, we
>> need to define what the drivers should aim for, with the potentially
>> limited information they have available, to provide as smooth and
>> unified an experience as possible.
>>
>> One benefit of -1 is that we might get away with adding that as a
>> special case later on, if we define 0 properly. And if the drivers know
>> they don't support off, they could have range 0..100 instead.
>
> I really believe that we need to define the ABI as 0 meaning minimal
> brightness which keeps the screen readable (which for the epaper
> example would be no brightness, but normally would be some minimal
> level).

We can define anything, but it doesn't mean it makes sense for the
drivers or that the drivers can implement it. By your above definition,
the right thing for the driver to do in the epaper case is to switch off
the backlight at 0, because switching it off completely can save more
power than just setting duty cycle to 0. That's the whole point for
epaper. And this contradicts with what you say below about backlight
on/off being orthogonal.

> Yes we do not get this right in some cases, but let at least define
> it properly in the ABI. Add a fat disclaimer for all I care that
> in some cases the driver is unable to guarantee this, but lets
> clearly define what 0 means and then try to get as much drivers
> to adhere to that as possible.
>
> As for -1 meaning turning stuff off, I'm against that, on/off
> vs brightness setting really are 2 almost orthogonal controls,
> in many cases in hardware they are truely orthogona, with both
> an enable pin as well as a pwm input for the brightness level,
> and driving the enable low will get the pwm input to effectively
> be ignored.

I think the crux with the on/off/minimum etc. is to have an ABI that
works sensibly also for drivers/hardware that is not able to switch
backlight off, or doesn't know whether the minimum is off or not. And we
need to have recommendations for drivers on what to do in the imperfect
reality.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS backlight ABI proposition

[Hans de Goede]

Hi,

On 23-02-17 09:55, Jani Nikula wrote:
> On Wed, 22 Feb 2017, Hans de Goede <hdegoede@redhat.com> wrote:
>> My first thought was that your proposal is reasonable, but on second
>> thought I foresee trouble here with e.g. the backlight level save / restore
>> code in systemd still using the sysfs interface, while the desktop
>> environment has moved on to the property, I believe we really need to
>> do some sort of bi-directional syncing here ...
>
> FWIW I think systemd should have no business changing the backlight. The
> save/restore should belong in the desktop environments... But I think we
> could dodge this one by having the initial sysfs value synced back to
> the property on initialization, but not otherwise. The systemd stuff,
> IIRC, happens before/after X.

We would also need to sync from the property back to sysfs when the drm-node
gets closed.

>>> We can bikeshed the meaning of 0 or -1, I don't mind. The point is, we
>>> need to define what the drivers should aim for, with the potentially
>>> limited information they have available, to provide as smooth and
>>> unified an experience as possible.
>>>
>>> One benefit of -1 is that we might get away with adding that as a
>>> special case later on, if we define 0 properly. And if the drivers know
>>> they don't support off, they could have range 0..100 instead.
>>
>> I really believe that we need to define the ABI as 0 meaning minimal
>> brightness which keeps the screen readable (which for the epaper
>> example would be no brightness, but normally would be some minimal
>> level).
>
> We can define anything, but it doesn't mean it makes sense for the
> drivers or that the drivers can implement it. By your above definition,
> the right thing for the driver to do in the epaper case is to switch off
> the backlight at 0, because switching it off completely can save more
> power than just setting duty cycle to 0. That's the whole point for
> epaper.

Correct.

> And this contradicts with what you say below about backlight
> on/off being orthogonal.

Not really this would just mean that for epaper as a special
case dpms on and backlight level 0 would result in the backlight
getting turned off as that still keeps the screen readable, this
can all be dealt with in the kernel without userspace needing to
know.

>> Yes we do not get this right in some cases, but let at least define
>> it properly in the ABI. Add a fat disclaimer for all I care that
>> in some cases the driver is unable to guarantee this, but lets
>> clearly define what 0 means and then try to get as much drivers
>> to adhere to that as possible.
>>
>> As for -1 meaning turning stuff off, I'm against that, on/off
>> vs brightness setting really are 2 almost orthogonal controls,
>> in many cases in hardware they are truely orthogona, with both
>> an enable pin as well as a pwm input for the brightness level,
>> and driving the enable low will get the pwm input to effectively
>> be ignored.
>
> I think the crux with the on/off/minimum etc. is to have an ABI that
> works sensibly also for drivers/hardware that is not able to switch
> backlight off,

Not being able to turn backlight off is covered by my proposal,
since 0 would keep the backlight on for a lcd panel.

> or doesn't know whether the minimum is off or not.

As I said in this case, I'm fine with adding a disclaimer for this
case to the ABI specification for the new brightness property I just
want the main text to clearly define how the driver should behave if
it does know. This also means we may add quirks for (some) models
where the VBT get things wrong and actually override the minimum
pwm from the VBT with a sensible value which actually works on the model
in question. Maybe even allow the interface to bind the backlight to
also provide a (hidden from userspace) minimum value to use when
the property is set to 0.

> And we need to have recommendations for drivers on what to do in the
 > imperfect reality.

I would say they need to do a best effort to make 0 be minimum brightness
and not off, like the i915 driver is doing now, where it tries to make 0
be minimum brightness by reading info from the VBT and if that info is
borked 0 often becomes off.

Regards,

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

Re: KMS backlight ABI proposition

[Thierry Reding]

[-- 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

Re: KMS backlight ABI proposition

[Jani Nikula]

On Mon, 20 Feb 2017, Thierry Reding <thierry.reding@gmail.com> wrote:
>>  - 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.

I outlined one idea elsewhere in the thread [1].

[1] http://lkml.kernel.org/r/87mvdei7ug.fsf@intel.com

>> 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.

I outlined some of the problems with bi-directional access elsewhere in
the thread [1]. This is also related to scaling (or re-ranging) between
API and hardware.

>> 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.

Yes, we should be clear in the specification, but also provide
recommendation for cases where 1) the driver does not know whether the
lowest value is off or not, 2) the driver can't switch the backlight
off.

(Reasons for the latter might be that you just can't switch the PWM
generator off alone, or it must be done in sequence with other hardware
that really needs a full DPMS, and the PWM or board design does not
allow 0 duty cycle with the PWM on. This is not theoretical.)

> 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.

See [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.

See [1]. (Sorry for these types of annoying references, but I already
replied there and don't want to split the discussion about the same
thing further.)

>>  - 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.

Should we encourage a backlight on/off interface when we can't be sure
whether the backlight can be switched off? The OLPC case is a valid one,
but I don't want to encourage backlight off as a "light DPMS".

(Side note, we should really remember that all brightness is not
backlight either.)

>> === 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.

Very few of the backlight class implementations support the bl_power
attribute to switch off the backlight. Even if they can be switched off
using 0 brightness. Unless we're looking at changing all backlight
drivers, the property implementation can only try to use value 0 for
backlight off. But we don't know if it's off or minimum or what. Also
see below.

>> 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.

The main goal was to allow for cases where you can't switch off the
backlight, or you don't know whether the lowest value is pitch black. As
I wrote in [1], we can change the meaning of 0, but we need to take into
account it really might mean "off" for some, and "minimum visible" for
others.

The old sysfs ABI handled this by deferring it all to drivers, and we
know the result... this really is an unholy mess.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS backlight ABI proposition

[Dave Airlie]

On 17 February 2017 at 22:58, Martin Peres <martin.peres@linux.intel.com> 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.
>
> == 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].
>
>  - 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.
>
>  - 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

You might want to read the rest of that thread, the response posted by Matthew

This is really messy and we made a decision to put the policy to pick
which backlight
driver into userspace because it's not something the kernel can figure
out properly.

Things might have changed now a bit with Win10 not using ACPI backlight controls
if memory serves, but it's still an unholy mess.

I think MBPs can expose 3 backlights (ACPI, gmux and driver) and only
the gmux one
does anything.

How do you tackle that end of the problem, how does the i915/drm core
know when the
driver for the one backlight it needs has appeared, and is working, by
deferring this to
userspace we let the system load all the drivers and the policy of
picking the correct one
is left there.

I'm not saying this is pretty,and we have libbacklight to "solve" the
problems for generic
userspace, but any solution is going to a be a lot uglier than you think.

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

Re: KMS backlight ABI proposition

[Hans de Goede]

Hi,

On 20-02-17 20:27, Dave Airlie wrote:
> On 17 February 2017 at 22:58, Martin Peres <martin.peres@linux.intel.com> 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.
>>
>> == 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].
>>
>>  - 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.
>>
>>  - 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

Jumping in in the middle of the thread here, I'll reply to the top-level post
later.

> You might want to read the rest of that thread, the response posted by Matthew
>
> This is really messy and we made a decision to put the policy to pick
> which backlight
> driver into userspace because it's not something the kernel can figure
> out properly.

That is actually not really true, the only policy which in practice lives
in userspace is picking firmware type backlight interfaces over platform /
raw. Everything, really EVERYTHING else is already handled in the kernel,
using quirk tables in various places. David Herrmann's proposal actually
might improve upon this by having a default binding behavior in the kernel
and then allow fixing up the binding between backlight interface and
drm connector through udev rules triggered by hwdb entries, which means
(some (*)) new quirks could be handled in userspace.

> Things might have changed now a bit with Win10 not using ACPI backlight controls
> if memory serves, but it's still an unholy mess.
>
> I think MBPs can expose 3 backlights (ACPI, gmux and driver) and only
> the gmux one does anything.

Correct which is why we have this in the kernel:

drivers/gpu/drm/nouveau/nouveau_backlight.c :


         if (apple_gmux_present()) {
                 NV_INFO(drm, "Apple GMUX detected: not registering Nouveau backlight interface\n");
                 return 0;
         }

> How do you tackle that end of the problem, how does the i915/drm core
> know when the
> driver for the one backlight it needs has appeared, and is working, by
> deferring this to
> userspace we let the system load all the drivers and the policy of
> picking the correct one
> is left there.

Except in practice it is not, each desktop-environment and then some
other bits and pieces all have their own code to pick which backlight
interface to use. Which luckily all are as simple as prefer firmware
over platform/raw, since doing something more smart would be a big
mess as everything has its own code for handling this.

And to work around this we end up with things like hiding the
acpi_video interface on win10 devices since it usually is broken,
but as it has a type of firmware it will be preferred when visible.

Really we are already dealing with this in the kernel, it is one of
the things I've been spending my time on the last 3 years, since
I've blogged and presented about this people tend to know to find
me when it comes to bugs related to this, so I'm quite familiar
with this.

All this proposal does is make the userspace interface for backlight
suck less, we already have all the pain in the kernel anyways.

> I'm not saying this is pretty,and we have libbacklight to "solve" the
> problems for generic
> userspace,

libbacklight ? Never heard of it, ah google gives me:

https://cgit.freedesktop.org/libbacklight/

Last commit 2011, number of users (AFAIK) 0, certainly not used by
gnome, upower, xf86-video-intel, etc.

 > but any solution is going to a be a lot uglier than you think.

Oh I know it is ugly, but as said we are already doing it, so we
might as well make it suck less.

Regards,

Hans




*) I say some because in some cases even loading a driver can put the
firmware in such a shape that backlight control will not work until a
hard reset is done.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS backlight ABI proposition

[Martin Peres]

On 20/02/17 21:57, Hans de Goede wrote:
> Hi,
>
> On 20-02-17 20:27, Dave Airlie wrote:
>> On 17 February 2017 at 22:58, Martin Peres 
>> <martin.peres@linux.intel.com> 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.
>>>
>>> == 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].
>>>
>>>  - 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.
>>>
>>>  - 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 
>>>
>
> Jumping in in the middle of the thread here, I'll reply to the 
> top-level post
> later.
>
>> You might want to read the rest of that thread, the response posted 
>> by Matthew
>>
>> This is really messy and we made a decision to put the policy to pick
>> which backlight
>> driver into userspace because it's not something the kernel can figure
>> out properly.
>
> That is actually not really true, the only policy which in practice lives
> in userspace is picking firmware type backlight interfaces over 
> platform /
> raw. Everything, really EVERYTHING else is already handled in the kernel,
> using quirk tables in various places. David Herrmann's proposal actually
> might improve upon this by having a default binding behavior in the 
> kernel
> and then allow fixing up the binding between backlight interface and
> drm connector through udev rules triggered by hwdb entries, which means
> (some (*)) new quirks could be handled in userspace.

I fully agree with this. Pushing everything to the userspace sounds like 
quite a
risk and adds complexity. This is not something the -modesetting driver and
wayland compositors will not deal in a uniform way...

Also, this means we need to expose a lot of information to the 
userspace, that
means new interfaces (and new ABI).

By trying to handle all the common cases in the kernel space and letting the
userspace override the decision through udev or by hand, it should 
result in a
much-better experience for everyone (no need to recompile your ddx driver
anymore).

Another added benefit of exposing backlight through KMS is that we do not
have to deal with two access control mechanisms, which should help the
userspace.

>
>> Things might have changed now a bit with Win10 not using ACPI 
>> backlight controls
>> if memory serves, but it's still an unholy mess.
>>
>> I think MBPs can expose 3 backlights (ACPI, gmux and driver) and only
>> the gmux one does anything.
>
> Correct which is why we have this in the kernel:
>
> drivers/gpu/drm/nouveau/nouveau_backlight.c :
>
>
>         if (apple_gmux_present()) {
>                 NV_INFO(drm, "Apple GMUX detected: not registering 
> Nouveau backlight interface\n");
>                 return 0;
>         }
>
>> How do you tackle that end of the problem, how does the i915/drm core
>> know when the
>> driver for the one backlight it needs has appeared, and is working, by
>> deferring this to
>> userspace we let the system load all the drivers and the policy of
>> picking the correct one
>> is left there.
>
> Except in practice it is not, each desktop-environment and then some
> other bits and pieces all have their own code to pick which backlight
> interface to use. Which luckily all are as simple as prefer firmware
> over platform/raw, since doing something more smart would be a big
> mess as everything has its own code for handling this.
>
> And to work around this we end up with things like hiding the
> acpi_video interface on win10 devices since it usually is broken,
> but as it has a type of firmware it will be preferred when visible.
>
> Really we are already dealing with this in the kernel, it is one of
> the things I've been spending my time on the last 3 years, since
> I've blogged and presented about this people tend to know to find
> me when it comes to bugs related to this, so I'm quite familiar
> with this.
>
> All this proposal does is make the userspace interface for backlight
> suck less, we already have all the pain in the kernel anyways.
>
>> I'm not saying this is pretty,and we have libbacklight to "solve" the
>> problems for generic
>> userspace,
>
> libbacklight ? Never heard of it, ah google gives me:
>
> https://cgit.freedesktop.org/libbacklight/
>
> Last commit 2011, number of users (AFAIK) 0, certainly not used by
> gnome, upower, xf86-video-intel, etc.

And it only does what you described: Prefer firmware drivers over raw ones.

>
> > but any solution is going to a be a lot uglier than you think.
>
> Oh I know it is ugly, but as said we are already doing it, so we
> might as well make it suck less.
>
> Regards,
>
> Hans
>
>
>
>
> *) I say some because in some cases even loading a driver can put the
> firmware in such a shape that backlight control will not work until a
> hard reset is done.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

Re: KMS backlight ABI proposition

[Alex Deucher]

On Mon, Feb 20, 2017 at 2:27 PM, Dave Airlie <airlied@gmail.com> wrote:
> On 17 February 2017 at 22:58, Martin Peres <martin.peres@linux.intel.com> 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.
>>
>> == 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].
>>
>>  - 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.
>>
>>  - 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
>
> You might want to read the rest of that thread, the response posted by Matthew
>
> This is really messy and we made a decision to put the policy to pick
> which backlight
> driver into userspace because it's not something the kernel can figure
> out properly.
>
> Things might have changed now a bit with Win10 not using ACPI backlight controls
> if memory serves, but it's still an unholy mess.
>
> I think MBPs can expose 3 backlights (ACPI, gmux and driver) and only
> the gmux one
> does anything.

Even on non-Macs, hybrid laptops can be problematic.  In some, the
backlight control was muxed, in some it was not.   Fortunately, muxed
laptops are a thing of the past, at least for non-Macs.

Alex

>
> How do you tackle that end of the problem, how does the i915/drm core
> know when the
> driver for the one backlight it needs has appeared, and is working, by
> deferring this to
> userspace we let the system load all the drivers and the policy of
> picking the correct one
> is left there.
>
> I'm not saying this is pretty,and we have libbacklight to "solve" the
> problems for generic
> userspace, but any solution is going to a be a lot uglier than you think.
>
> Dave.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS backlight ABI proposition

[Jasper St. Pierre]

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

I don't work on this anymore, but, even disregarding the mess that is ACPI
backlight:

I think when people start interfacing with the Backlight API, they wonder
why it's not normalized. And then you start implementing it, and you
realize that some writes don't do anything. And that when you read back,
it's not what you just read. In any real implementation, I would want to
know the step number that causes a level of brightness. And once I have
that, I'm effectively doing 100/step to get a real hardware max level
anyway, so I'm effectively working around the normalized behavior.

These have all caused strange bugs for me in the past. Caching what's read
and what's written sounds like a solution until you realize that userspace
has plenty of DBus services and wrapper libraries to abstract out
implementation details and it's hard to figure out where you should put
that cache, and having it twice can also cause strange bugs.

Normalizing to 100 would make simple cases subtly wrong and make complex
cases impossible.

Instead of normalizing to 100, I think we should expose a max level, and
specify that's max brightness. Every single step in the range should have a
visible difference in output lumens. 0 is minimum brightness -- this means
the backlight is still on. Backlight being off should be -1 or we should
recommend people use DPMS.

This allows userspace to know everything it should and still provide for a
10-step "Brightness Up" button.

On Mon, Feb 20, 2017 at 2:40 PM, Alex Deucher <alexdeucher@gmail.com> wrote:

> On Mon, Feb 20, 2017 at 2:27 PM, Dave Airlie <airlied@gmail.com> wrote:
> > On 17 February 2017 at 22:58, Martin Peres <martin.peres@linux.intel.com>
> 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.
> >>
> >> == 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].
> >>
> >>  - 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.
> >>
> >>  - 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
> >
> > You might want to read the rest of that thread, the response posted by
> Matthew
> >
> > This is really messy and we made a decision to put the policy to pick
> > which backlight
> > driver into userspace because it's not something the kernel can figure
> > out properly.
> >
> > Things might have changed now a bit with Win10 not using ACPI backlight
> controls
> > if memory serves, but it's still an unholy mess.
> >
> > I think MBPs can expose 3 backlights (ACPI, gmux and driver) and only
> > the gmux one
> > does anything.
>
> Even on non-Macs, hybrid laptops can be problematic.  In some, the
> backlight control was muxed, in some it was not.   Fortunately, muxed
> laptops are a thing of the past, at least for non-Macs.
>
> Alex
>
> >
> > How do you tackle that end of the problem, how does the i915/drm core
> > know when the
> > driver for the one backlight it needs has appeared, and is working, by
> > deferring this to
> > userspace we let the system load all the drivers and the policy of
> > picking the correct one
> > is left there.
> >
> > I'm not saying this is pretty,and we have libbacklight to "solve" the
> > problems for generic
> > userspace, but any solution is going to a be a lot uglier than you think.
> >
> > Dave.
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>



-- 
  Jasper

[-- Attachment #1.2: Type: text/html, Size: 7813 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

Re: KMS backlight ABI proposition

[Jani Nikula]

On Tue, 21 Feb 2017, "Jasper St. Pierre" <jstpierre@mecheye.net> wrote:
> Instead of normalizing to 100, I think we should expose a max level, and
> specify that's max brightness. Every single step in the range should have a
> visible difference in output lumens.

The drivers can't guarantee that.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS backlight ABI proposition

[Jasper St. Pierre]

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

Can we at least suggest they try, again with a similar disclaimer that
"this might not work in practice"? The more I think about it, a normalized
0-100 API doesn't really simplify implementations for anybody, especially
if we don't have a natural step level.

On Wed, Feb 22, 2017 at 6:00 AM, Jani Nikula <jani.nikula@linux.intel.com>
wrote:

> On Tue, 21 Feb 2017, "Jasper St. Pierre" <jstpierre@mecheye.net> wrote:
> > Instead of normalizing to 100, I think we should expose a max level, and
> > specify that's max brightness. Every single step in the range should
> have a
> > visible difference in output lumens.
>
> The drivers can't guarantee that.
>
> BR,
> Jani.
>
>
> --
> Jani Nikula, Intel Open Source Technology Center
>



-- 
  Jasper

[-- Attachment #1.2: Type: text/html, Size: 1361 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

Re: KMS backlight ABI proposition

[Jani Nikula]

On Mon, 20 Feb 2017, Dave Airlie <airlied@gmail.com> wrote:
> How do you tackle that end of the problem, how does the i915/drm core
> know when the driver for the one backlight it needs has appeared, and
> is working, by deferring this to userspace we let the system load all
> the drivers and the policy of picking the correct one is left there.

One of David's original ideas was to allow userspace to re-associate the
properties with the underlying backlight class implementations. The
kernel could make a best guess, and udev could handle the exceptions. I
wouldn't mind having the quirks managed there. I think udev rules could
handle the same backlight type priority ordering as we have now.

> I'm not saying this is pretty,and we have libbacklight to "solve" the
> problems for generic userspace, but any solution is going to a be a
> lot uglier than you think.

I have no illusions about this, but I'm really hoping we could hide a
good chunk of the ugliness under a fresh layer of paint in drm,
especially for the nowadays most common case of native backlights.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS backlight ABI proposition

[Hans de Goede]

Hi All,

On 17-02-17 13: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.
>
> == 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].
>
>  - 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.
>
>  - 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.
>
> Uni-directional would work in all cases, with the caveat that mixing calls to the KMS property and the backlight device will not be supported (changes mades through the sysfs interface of the backlight driver will not be reflected in the KMS property). At boot time, we should however initialize the value of the backlight property with a value close to what is currently set in the backlight driver.
>
> Giving exclusive access does not sound very good to me, as it would be hard for the userspace to deal with disappearing drivers...
>
> === 2) How many steps should be exposed ===
>
> If the KMS property exposes the same number of steps as the backlight driver, it allows us to get a bijective function between the two interfaces, and allow a bi-directional communication. The downside of this is that it forces the userspace to deal with a variable number of steps which can range from 4 to 1k+. Also, the userspace would be able to handle the case where there are less steps than it would like to expose.
>
> If the KMS property exposes a fixed number of steps (say 100), it becomes easy for the userspace to express the wanted brightness. However, on drivers exposing less than these 100 steps, we cannot guarantee that any change in the value will produce any change. If there is only one possible value (on or off), the user may be trying the change the brightness, a GUI would show what is the expected backlight state, but no change in the luminance would be seen, which is pretty bad.

So 1 and 2 are closely related the problem is that if we expose a fixed number of steps
we need to convert in both directions, and if userspace tries to increment by doing read,
add 1, write and we expose 1-100 but the hardware has only 4 levels then the read will
keep returning the same value. Note I've fixed bugs like this already so this is a real
problem. As already mentioned in the thread this can be fixed by caching the last written
value on both sides and invalidating both caches on a new write to one side.

> === 3) and 4) ===
>
> These issues are not handled at all by the backlight device sysfs interface.
>
> 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 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.
>
> 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
>
> ===  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.
>
> == 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)
>  - Uni-directional: KMS -> backlight
>  - 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!
>  - Does not expose the current backlight power as we want to let the kernel deal with DPMS on its own
>
> === ABI proposal ===
>
> The brightness property MUST have values 0...100 inclusive.
>
> The display brightness MUST be a monotonically increasing function of
> the brightness property.

What does "monotonically increasing" actually mean ? I would prefer
to clearly define that we are talking about perceived brightness here,
for 2 reasons:

1) This is what the user actually wants
2) Some of the x86 firmware interfaces only give us perceived brightness
and no way to get back to any other unit of measire.

> 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].
>
> If the hardware is not capable of supporting zero brightness, and the
> driver knows this, value 0 MUST be equal to value 1.

Ok.

> 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].

I agree, but this needs rewording (I had to read it 3 times).

> If the brightness gets changed outside of the property interface,
> reading the property value MAY be out of sync with the actual brightness
> [3].

This is for when the panel is off ? Otherwise it seems like a bad idea
to me.

> [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".

Regards,

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

Re: KMS backlight ABI proposition

[Jani Nikula]

On Mon, 20 Feb 2017, Hans de Goede <hdegoede@redhat.com> wrote:
> On 17-02-17 13:58, Martin Peres wrote:
> So 1 and 2 are closely related the problem is that if we expose a
> fixed number of steps we need to convert in both directions, and if
> userspace tries to increment by doing read, add 1, write and we expose
> 1-100 but the hardware has only 4 levels then the read will keep
> returning the same value. Note I've fixed bugs like this already so
> this is a real problem. As already mentioned in the thread this can be
> fixed by caching the last written value on both sides and invalidating
> both caches on a new write to one side.

I don't think userspace should be doing read-modify-write like this,
*unless* the value read is guaranteed to be the same as the last value
written.

Contrast with the sysfs brightness class interface. RMW should only ever
have been done on the "brightness" attribute alone, never using read on
"actual_brightness" attribute and write on "brightness".

>> The display brightness MUST be a monotonically increasing function of
>> the brightness property.
>
> What does "monotonically increasing" actually mean ? I would prefer
> to clearly define that we are talking about perceived brightness here,
> for 2 reasons:

https://en.wikipedia.org/wiki/Monotonic_function

Increase in the property means the same or higher "perceived
brightness".

> 1) This is what the user actually wants
> 2) Some of the x86 firmware interfaces only give us perceived brightness
> and no way to get back to any other unit of measire.
>
>> 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].
>>
>> If the hardware is not capable of supporting zero brightness, and the
>> driver knows this, value 0 MUST be equal to value 1.
>
> Ok.
>
>> 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].
>
> I agree, but this needs rewording (I had to read it 3 times).
>
>> If the brightness gets changed outside of the property interface,
>> reading the property value MAY be out of sync with the actual brightness
>> [3].
>
> This is for when the panel is off ? Otherwise it seems like a bad idea
> to me.

It means we don't want to go dig through the sysfs class interface to
figure out what was written there, and do lossy conversions back to the
property (if there's scaling involved).


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS backlight ABI proposition

[Stéphane Marchesin]

On Fri, Feb 17, 2017 at 4:58 AM, Martin Peres
<martin.peres@linux.intel.com> 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.
>
> == 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].
>
>  - 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.
>
>  - 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.
>
> Uni-directional would work in all cases, with the caveat that mixing calls
> to the KMS property and the backlight device will not be supported (changes
> mades through the sysfs interface of the backlight driver will not be
> reflected in the KMS property). At boot time, we should however initialize
> the value of the backlight property with a value close to what is currently
> set in the backlight driver.
>
> Giving exclusive access does not sound very good to me, as it would be hard
> for the userspace to deal with disappearing drivers...
>
> === 2) How many steps should be exposed ===
>
> If the KMS property exposes the same number of steps as the backlight
> driver, it allows us to get a bijective function between the two interfaces,
> and allow a bi-directional communication. The downside of this is that it
> forces the userspace to deal with a variable number of steps which can range
> from 4 to 1k+. Also, the userspace would be able to handle the case where
> there are less steps than it would like to expose.
>
> If the KMS property exposes a fixed number of steps (say 100), it becomes
> easy for the userspace to express the wanted brightness. However, on drivers
> exposing less than these 100 steps, we cannot guarantee that any change in
> the value will produce any change. If there is only one possible value (on
> or off), the user may be trying the change the brightness, a GUI would show
> what is the expected backlight state, but no change in the luminance would
> be seen, which is pretty bad.

Yes, I don't think we want to normalize anything here. It would
essentially be hiding functionality from user space, which then can't
expose it in the user interface. As you say, if the backlight slider
moves, but the backlight level didn't change, that's weird. On the
other hand if user space knows the number of levels it can give you a
consistent slider, and normalizing in user space is not that hard
(that's how things currently work after all, so people should be used
to it).


>
> === 3) and 4) ===
>
> These issues are not handled at all by the backlight device sysfs interface.
>
> 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 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.

Yes the ability to turn off the backlight is important. Some
backlights are not stable at low levels, so they don't expose these
low levels and effectively level 0 is not off (it is the lowest level
which works). So I guess the question is how should that non-linearity
be exposed versus the ability to turn it off completely.

>
> 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
>
> ===  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.

Are we saying that we are putting the kernel in charge of  display vs
backlight sequencing? Currently on some ARM boards with separate pwm
backlight drivers that's not the case. Don't get me wrong, I think the
kernel should be in charge of enforcing sequencing because otherwise
user space can damage hardware, I'm just pointing out that right now
it isn't the case.

Stéphane

>
> == 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)
>  - Uni-directional: KMS -> backlight
>  - 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!
>  - Does not expose the current backlight power as we want to let the kernel
> deal with DPMS on its own
>
> === 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].
>
> 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].
>
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS backlight ABI proposition

[Jani Nikula]

On Wed, 22 Feb 2017, Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
> On Fri, Feb 17, 2017 at 4:58 AM, Martin Peres
> <martin.peres@linux.intel.com> wrote:
>> If the KMS property exposes a fixed number of steps (say 100), it becomes
>> easy for the userspace to express the wanted brightness. However, on drivers
>> exposing less than these 100 steps, we cannot guarantee that any change in
>> the value will produce any change. If there is only one possible value (on
>> or off), the user may be trying the change the brightness, a GUI would show
>> what is the expected backlight state, but no change in the luminance would
>> be seen, which is pretty bad.
>
> Yes, I don't think we want to normalize anything here. It would
> essentially be hiding functionality from user space, which then can't
> expose it in the user interface. As you say, if the backlight slider
> moves, but the backlight level didn't change, that's weird. On the
> other hand if user space knows the number of levels it can give you a
> consistent slider, and normalizing in user space is not that hard
> (that's how things currently work after all, so people should be used
> to it).

I listed some of the benefits of normalizing (or re-ranging) in
[1]. Conversely, I haven't seen good answers on how to gracefully handle
the brightness range changing on the fly. That is what not normalizing
would mean. I don't think the current property implementation even
allows changing the range. And then there'd have to be a way to tell the
userspace that the range has changed.

In the same message, I mentioned the idea of providing an API to
increase/decrease brightness. That might be much easier to implement
than allowing the property range change.

[1] http://marc.info/?i=87mvdei7ug.fsf@intel.com

> Yes the ability to turn off the backlight is important. Some
> backlights are not stable at low levels, so they don't expose these
> low levels and effectively level 0 is not off (it is the lowest level
> which works). So I guess the question is how should that non-linearity
> be exposed versus the ability to turn it off completely.

You fail to say *why* the ability to turn off the backlight is
important. I've seen it used as a kind of "light DPMS" that can be done
using the sysfs interface, but I think that's a hack, really. Here,
whoever changes the backlight would be doing it using the DRM APIs
anyway, so it could do actual DPMS anyway. And, of course, not all
backlight hardware is able to switch off the backlight, and not all
drivers will be able to say whether 0 is off or not.

>> 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.
>
> Are we saying that we are putting the kernel in charge of  display vs
> backlight sequencing? Currently on some ARM boards with separate pwm
> backlight drivers that's not the case. Don't get me wrong, I think the
> kernel should be in charge of enforcing sequencing because otherwise
> user space can damage hardware, I'm just pointing out that right now
> it isn't the case.

Whenever the kernel is able to enforce the sequencing, it should. I
believe this is the case for most native backlight implementations. And
in these cases the backlight on/off toggling would really have to be a
substate of enabled display; can't enable backlight without display
enabled.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS backlight ABI proposition

[Hans de Goede]

Hi,

On 23-02-17 09:40, Jani Nikula wrote:
> On Wed, 22 Feb 2017, Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
>> On Fri, Feb 17, 2017 at 4:58 AM, Martin Peres
>> <martin.peres@linux.intel.com> wrote:
>>> If the KMS property exposes a fixed number of steps (say 100), it becomes
>>> easy for the userspace to express the wanted brightness. However, on drivers
>>> exposing less than these 100 steps, we cannot guarantee that any change in
>>> the value will produce any change. If there is only one possible value (on
>>> or off), the user may be trying the change the brightness, a GUI would show
>>> what is the expected backlight state, but no change in the luminance would
>>> be seen, which is pretty bad.
>>
>> Yes, I don't think we want to normalize anything here. It would
>> essentially be hiding functionality from user space, which then can't
>> expose it in the user interface. As you say, if the backlight slider
>> moves, but the backlight level didn't change, that's weird. On the
>> other hand if user space knows the number of levels it can give you a
>> consistent slider, and normalizing in user space is not that hard
>> (that's how things currently work after all, so people should be used
>> to it).
>
> I listed some of the benefits of normalizing (or re-ranging) in
> [1]. Conversely, I haven't seen good answers on how to gracefully handle
> the brightness range changing on the fly. That is what not normalizing
> would mean. I don't think the current property implementation even
> allows changing the range. And then there'd have to be a way to tell the
> userspace that the range has changed.

Ok, so after reading [1], I understand why you want to allow the maximum
to be changed, but that only is an issue if anyone actually has the
drm-node open and has read the max value (I add the and has read the
max value here to not get in the way of boot splashscreens).

Since udev will be changing the binding between backlight interface
and drm connector, we can simply just return -EBUSY if the drm-node
is open and then we can actually save change the max level.

With that done the remaining argument seems to be what about backlight
interfaces which expose more levels then say 100. I agree that it is
probably a good idea to downscale their range to 100, but I'm not
a fan of upscaling older ACPI interfaces which have as little as 8
steps to a 100, that just means userspace will try to make a change
which may very well result in no change at all.

Regards,

Hans

[1] http://marc.info/?i=87mvdei7ug.fsf@intel.com

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

Re: KMS backlight ABI proposition

[Stéphane Marchesin]

On Thu, Feb 23, 2017 at 12:40 AM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Wed, 22 Feb 2017, Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
>> On Fri, Feb 17, 2017 at 4:58 AM, Martin Peres
>> <martin.peres@linux.intel.com> wrote:
>>> If the KMS property exposes a fixed number of steps (say 100), it becomes
>>> easy for the userspace to express the wanted brightness. However, on drivers
>>> exposing less than these 100 steps, we cannot guarantee that any change in
>>> the value will produce any change. If there is only one possible value (on
>>> or off), the user may be trying the change the brightness, a GUI would show
>>> what is the expected backlight state, but no change in the luminance would
>>> be seen, which is pretty bad.
>>
>> Yes, I don't think we want to normalize anything here. It would
>> essentially be hiding functionality from user space, which then can't
>> expose it in the user interface. As you say, if the backlight slider
>> moves, but the backlight level didn't change, that's weird. On the
>> other hand if user space knows the number of levels it can give you a
>> consistent slider, and normalizing in user space is not that hard
>> (that's how things currently work after all, so people should be used
>> to it).
>
> I listed some of the benefits of normalizing (or re-ranging) in
> [1]. Conversely, I haven't seen good answers on how to gracefully handle
> the brightness range changing on the fly. That is what not normalizing
> would mean. I don't think the current property implementation even
> allows changing the range. And then there'd have to be a way to tell the
> userspace that the range has changed.


Let me reply to your points:
- "And the userspace will only use maybe ten steps." That isn't true,
we use all the steps that are available to do smooth transitions in
Chrome OS.
- "Some PWM based backlight allow adjusting the PWM modulation
frequency." you don't need a motivation for *why* I would want to
change the mod freq on the fly, actually in my experience you
shouldn't since this can lead to flickery backlights.
- "The max might change" again you don't say why except that you want
to change the mod freq. Basically point 3 is like point 2.


>
> In the same message, I mentioned the idea of providing an API to
> increase/decrease brightness. That might be much easier to implement
> than allowing the property range change.
>
> [1] http://marc.info/?i=87mvdei7ug.fsf@intel.com
>
>> Yes the ability to turn off the backlight is important. Some
>> backlights are not stable at low levels, so they don't expose these
>> low levels and effectively level 0 is not off (it is the lowest level
>> which works). So I guess the question is how should that non-linearity
>> be exposed versus the ability to turn it off completely.
>
> You fail to say *why* the ability to turn off the backlight is
> important. I've seen it used as a kind of "light DPMS" that can be done
> using the sysfs interface, but I think that's a hack, really. Here,
> whoever changes the backlight would be doing it using the DRM APIs
> anyway, so it could do actual DPMS anyway. And, of course, not all
> backlight hardware is able to switch off the backlight, and not all
> drivers will be able to say whether 0 is off or not.

Turning the on/off the backlight is much quicker than turning on/off
the display through DPMS. So one thing we do is use that to turn a
screen off/on very quickly.



>
>>> 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.
>>
>> Are we saying that we are putting the kernel in charge of  display vs
>> backlight sequencing? Currently on some ARM boards with separate pwm
>> backlight drivers that's not the case. Don't get me wrong, I think the
>> kernel should be in charge of enforcing sequencing because otherwise
>> user space can damage hardware, I'm just pointing out that right now
>> it isn't the case.
>
> Whenever the kernel is able to enforce the sequencing, it should. I


It probably shouldn't be just "it should". If user space can damage
the hw, then the kernel is broken.

Stéphane


> believe this is the case for most native backlight implementations. And
> in these cases the backlight on/off toggling would really have to be a
> substate of enabled display; can't enable backlight without display
> enabled.
>
> BR,
> Jani.
>
>
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS backlight ABI proposition

[Jani Nikula]

On Thu, 23 Feb 2017, Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
> On Thu, Feb 23, 2017 at 12:40 AM, Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
>> On Wed, 22 Feb 2017, Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
>>> On Fri, Feb 17, 2017 at 4:58 AM, Martin Peres
>>> <martin.peres@linux.intel.com> wrote:
>>>> If the KMS property exposes a fixed number of steps (say 100), it becomes
>>>> easy for the userspace to express the wanted brightness. However, on drivers
>>>> exposing less than these 100 steps, we cannot guarantee that any change in
>>>> the value will produce any change. If there is only one possible value (on
>>>> or off), the user may be trying the change the brightness, a GUI would show
>>>> what is the expected backlight state, but no change in the luminance would
>>>> be seen, which is pretty bad.
>>>
>>> Yes, I don't think we want to normalize anything here. It would
>>> essentially be hiding functionality from user space, which then can't
>>> expose it in the user interface. As you say, if the backlight slider
>>> moves, but the backlight level didn't change, that's weird. On the
>>> other hand if user space knows the number of levels it can give you a
>>> consistent slider, and normalizing in user space is not that hard
>>> (that's how things currently work after all, so people should be used
>>> to it).
>>
>> I listed some of the benefits of normalizing (or re-ranging) in
>> [1]. Conversely, I haven't seen good answers on how to gracefully handle
>> the brightness range changing on the fly. That is what not normalizing
>> would mean. I don't think the current property implementation even
>> allows changing the range. And then there'd have to be a way to tell the
>> userspace that the range has changed.
>
>
> Let me reply to your points:
> - "And the userspace will only use maybe ten steps." That isn't true,
> we use all the steps that are available to do smooth transitions in
> Chrome OS.

Fair enough. But using, say, 1000 steps is excessive even for that
because you can't distinguish the steps from each other.

> - "Some PWM based backlight allow adjusting the PWM modulation
> frequency." you don't need a motivation for *why* I would want to
> change the mod freq on the fly, actually in my experience you
> shouldn't since this can lead to flickery backlights.

The modulation frequency is usually an OEM design choice. The higher the
frequency, less flicker, but also fewer user distinguishable levels of
backlight (signal rise/fall times come into play). Occasionally there
have been requests to be able to adjust the frequency. We can of course
decide that's an implementation detail and not let userspace change it.

> - "The max might change" again you don't say why except that you want
> to change the mod freq. Basically point 3 is like point 2.

I guess I wasn't clear enough. If the property max reflects the max of
the underlying backlight class implementation, and userspace
re-associates the property with *another* backlight class implementation
(because the kernel might not always get the association right), it's
likely that the max will not be the same. This re-association was one of
David's original key ideas, so udev could carry the quirks, and
users/developers could use it for debugging.

I don't think we have any good ideas how to solve the property max
changing on the fly. But based on the discussion so far, it's starting
to look like we'll need to study that more thoroughly.

(Thanks for requiring the rationale from me like I asked of
you. Really. We can't figure this out otherwise.)

>> In the same message, I mentioned the idea of providing an API to
>> increase/decrease brightness. That might be much easier to implement
>> than allowing the property range change.
>>
>> [1] http://marc.info/?i=87mvdei7ug.fsf@intel.com
>>
>>> Yes the ability to turn off the backlight is important. Some
>>> backlights are not stable at low levels, so they don't expose these
>>> low levels and effectively level 0 is not off (it is the lowest level
>>> which works). So I guess the question is how should that non-linearity
>>> be exposed versus the ability to turn it off completely.
>>
>> You fail to say *why* the ability to turn off the backlight is
>> important. I've seen it used as a kind of "light DPMS" that can be done
>> using the sysfs interface, but I think that's a hack, really. Here,
>> whoever changes the backlight would be doing it using the DRM APIs
>> anyway, so it could do actual DPMS anyway. And, of course, not all
>> backlight hardware is able to switch off the backlight, and not all
>> drivers will be able to say whether 0 is off or not.
>
> Turning the on/off the backlight is much quicker than turning on/off
> the display through DPMS. So one thing we do is use that to turn a
> screen off/on very quickly.

I suppose you can get away with that because you have control over the
overall product and the userspace, and you can ensure this works. What
would you do if the hardware or the kernel driver didn't support
switching off the backlight? I also presume you'd always know if and
when that's the case; in the generic case that's not always possible.

>>>> 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.
>>>
>>> Are we saying that we are putting the kernel in charge of  display vs
>>> backlight sequencing? Currently on some ARM boards with separate pwm
>>> backlight drivers that's not the case. Don't get me wrong, I think the
>>> kernel should be in charge of enforcing sequencing because otherwise
>>> user space can damage hardware, I'm just pointing out that right now
>>> it isn't the case.
>>
>> Whenever the kernel is able to enforce the sequencing, it should. I
>
> It probably shouldn't be just "it should". If user space can damage
> the hw, then the kernel is broken.

Agreed.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS backlight ABI proposition

[Hans de Goede]

Hi,

On 24-02-17 09:43, Jani Nikula wrote:
> On Thu, 23 Feb 2017, Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
>> On Thu, Feb 23, 2017 at 12:40 AM, Jani Nikula
>> <jani.nikula@linux.intel.com> wrote:
>>> On Wed, 22 Feb 2017, Stéphane Marchesin <stephane.marchesin@gmail.com> wrote:
>>>> On Fri, Feb 17, 2017 at 4:58 AM, Martin Peres
>>>> <martin.peres@linux.intel.com> wrote:
>>>>> If the KMS property exposes a fixed number of steps (say 100), it becomes
>>>>> easy for the userspace to express the wanted brightness. However, on drivers
>>>>> exposing less than these 100 steps, we cannot guarantee that any change in
>>>>> the value will produce any change. If there is only one possible value (on
>>>>> or off), the user may be trying the change the brightness, a GUI would show
>>>>> what is the expected backlight state, but no change in the luminance would
>>>>> be seen, which is pretty bad.
>>>>
>>>> Yes, I don't think we want to normalize anything here. It would
>>>> essentially be hiding functionality from user space, which then can't
>>>> expose it in the user interface. As you say, if the backlight slider
>>>> moves, but the backlight level didn't change, that's weird. On the
>>>> other hand if user space knows the number of levels it can give you a
>>>> consistent slider, and normalizing in user space is not that hard
>>>> (that's how things currently work after all, so people should be used
>>>> to it).
>>>
>>> I listed some of the benefits of normalizing (or re-ranging) in
>>> [1]. Conversely, I haven't seen good answers on how to gracefully handle
>>> the brightness range changing on the fly. That is what not normalizing
>>> would mean. I don't think the current property implementation even
>>> allows changing the range. And then there'd have to be a way to tell the
>>> userspace that the range has changed.
>>
>>
>> Let me reply to your points:
>> - "And the userspace will only use maybe ten steps." That isn't true,
>> we use all the steps that are available to do smooth transitions in
>> Chrome OS.
>
> Fair enough. But using, say, 1000 steps is excessive even for that
> because you can't distinguish the steps from each other.
>
>> - "Some PWM based backlight allow adjusting the PWM modulation
>> frequency." you don't need a motivation for *why* I would want to
>> change the mod freq on the fly, actually in my experience you
>> shouldn't since this can lead to flickery backlights.
>
> The modulation frequency is usually an OEM design choice. The higher the
> frequency, less flicker, but also fewer user distinguishable levels of
> backlight (signal rise/fall times come into play). Occasionally there
> have been requests to be able to adjust the frequency. We can of course
> decide that's an implementation detail and not let userspace change it.
>
>> - "The max might change" again you don't say why except that you want
>> to change the mod freq. Basically point 3 is like point 2.
>
> I guess I wasn't clear enough. If the property max reflects the max of
> the underlying backlight class implementation, and userspace
> re-associates the property with *another* backlight class implementation
> (because the kernel might not always get the association right), it's
> likely that the max will not be the same. This re-association was one of
> David's original key ideas, so udev could carry the quirks, and
> users/developers could use it for debugging.
>
> I don't think we have any good ideas how to solve the property max
> changing on the fly. But based on the discussion so far, it's starting
> to look like we'll need to study that more thoroughly.

As I mentioned in another part of the thread, I think we can just return
-EBUSY if udev tries to change the binding when a drm-node is open
*and* the backlight/brightness property has been accessed (even if
read only) through that drm-node. We can reset the busy flag when
the (last open) drm-node gets closed.

I'm adding the *and* the backlight/brightness property has been accessed
condition so that udev can still do its thing while a boot splash
screen is active. This assumes that boot splash screens do not
touch the brightness.

Regards,

Hans




>
> (Thanks for requiring the rationale from me like I asked of
> you. Really. We can't figure this out otherwise.)
>
>>> In the same message, I mentioned the idea of providing an API to
>>> increase/decrease brightness. That might be much easier to implement
>>> than allowing the property range change.
>>>
>>> [1] http://marc.info/?i=87mvdei7ug.fsf@intel.com
>>>
>>>> Yes the ability to turn off the backlight is important. Some
>>>> backlights are not stable at low levels, so they don't expose these
>>>> low levels and effectively level 0 is not off (it is the lowest level
>>>> which works). So I guess the question is how should that non-linearity
>>>> be exposed versus the ability to turn it off completely.
>>>
>>> You fail to say *why* the ability to turn off the backlight is
>>> important. I've seen it used as a kind of "light DPMS" that can be done
>>> using the sysfs interface, but I think that's a hack, really. Here,
>>> whoever changes the backlight would be doing it using the DRM APIs
>>> anyway, so it could do actual DPMS anyway. And, of course, not all
>>> backlight hardware is able to switch off the backlight, and not all
>>> drivers will be able to say whether 0 is off or not.
>>
>> Turning the on/off the backlight is much quicker than turning on/off
>> the display through DPMS. So one thing we do is use that to turn a
>> screen off/on very quickly.
>
> I suppose you can get away with that because you have control over the
> overall product and the userspace, and you can ensure this works. What
> would you do if the hardware or the kernel driver didn't support
> switching off the backlight? I also presume you'd always know if and
> when that's the case; in the generic case that's not always possible.
>
>>>>> 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.
>>>>
>>>> Are we saying that we are putting the kernel in charge of  display vs
>>>> backlight sequencing? Currently on some ARM boards with separate pwm
>>>> backlight drivers that's not the case. Don't get me wrong, I think the
>>>> kernel should be in charge of enforcing sequencing because otherwise
>>>> user space can damage hardware, I'm just pointing out that right now
>>>> it isn't the case.
>>>
>>> Whenever the kernel is able to enforce the sequencing, it should. I
>>
>> It probably shouldn't be just "it should". If user space can damage
>> the hw, then the kernel is broken.
>
> Agreed.
>
> BR,
> Jani.
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS backlight ABI proposition

[Jani Nikula]

On Fri, 24 Feb 2017, Hans de Goede <hdegoede@redhat.com> wrote:
> On 24-02-17 09:43, Jani Nikula wrote:
>> I don't think we have any good ideas how to solve the property max
>> changing on the fly. But based on the discussion so far, it's starting
>> to look like we'll need to study that more thoroughly.
>
> As I mentioned in another part of the thread, I think we can just return
> -EBUSY if udev tries to change the binding when a drm-node is open
> *and* the backlight/brightness property has been accessed (even if
> read only) through that drm-node. We can reset the busy flag when
> the (last open) drm-node gets closed.

That's certainly easier than coming up with ways to notify the userspace
about property range changes. The obvious downside is that you have to
kill X to do the re-association. That's fine for when everything just
works (i.e. everything happens before the drm node is opened), but for
debugging it's a bit painful. Can we live with that?

> I'm adding the *and* the backlight/brightness property has been accessed
> condition so that udev can still do its thing while a boot splash
> screen is active. This assumes that boot splash screens do not
> touch the brightness.

I presume udev will have to do its job before the drm node is opened, I
think relying on the userspace not touching the brightness property is
going to be racy/flaky.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS backlight ABI proposition

[Hans de Goede]

Hi,

On 24-02-17 10:34, Jani Nikula wrote:
> On Fri, 24 Feb 2017, Hans de Goede <hdegoede@redhat.com> wrote:
>> On 24-02-17 09:43, Jani Nikula wrote:
>>> I don't think we have any good ideas how to solve the property max
>>> changing on the fly. But based on the discussion so far, it's starting
>>> to look like we'll need to study that more thoroughly.
>>
>> As I mentioned in another part of the thread, I think we can just return
>> -EBUSY if udev tries to change the binding when a drm-node is open
>> *and* the backlight/brightness property has been accessed (even if
>> read only) through that drm-node. We can reset the busy flag when
>> the (last open) drm-node gets closed.
>
> That's certainly easier than coming up with ways to notify the userspace
> about property range changes. The obvious downside is that you have to
> kill X to do the re-association. That's fine for when everything just
> works (i.e. everything happens before the drm node is opened), but for
> debugging it's a bit painful. Can we live with that?

Sure, debugging udev rules is somewhat tricky in general (requires
manually triggering udev). When asking users to test udev rule / hwdb
patches I usually just ask them to reboot.

>> I'm adding the *and* the backlight/brightness property has been accessed
>> condition so that udev can still do its thing while a boot splash
>> screen is active. This assumes that boot splash screens do not
>> touch the brightness.
>
> I presume udev will have to do its job before the drm node is opened, I
> think relying on the userspace not touching the brightness property is
> going to be racy/flaky.

Making sure that udev does its job before we show the splashscreen
is tricky, note the idea is that the splashscreen *never* touches
the brightness property and by the time regular X / wayland launches
udev should have had plenty of time to complete its job.

I do not know how hard it will be to add the code to detect triggering
the property being touched, but that is the best I can come up with
to still allow the bootsplash (e.g. plymouth) to use the drm-node.

Regards,

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

Re: KMS backlight ABI proposition

[Hans de Goede]

Hi,

On 24-02-17 10:46, Hans de Goede wrote:
> Hi,
>
> On 24-02-17 10:34, Jani Nikula wrote:
>> On Fri, 24 Feb 2017, Hans de Goede <hdegoede@redhat.com> wrote:
>>> On 24-02-17 09:43, Jani Nikula wrote:
>>>> I don't think we have any good ideas how to solve the property max
>>>> changing on the fly. But based on the discussion so far, it's starting
>>>> to look like we'll need to study that more thoroughly.
>>>
>>> As I mentioned in another part of the thread, I think we can just return
>>> -EBUSY if udev tries to change the binding when a drm-node is open
>>> *and* the backlight/brightness property has been accessed (even if
>>> read only) through that drm-node. We can reset the busy flag when
>>> the (last open) drm-node gets closed.
>>
>> That's certainly easier than coming up with ways to notify the userspace
>> about property range changes. The obvious downside is that you have to
>> kill X to do the re-association. That's fine for when everything just
>> works (i.e. everything happens before the drm node is opened), but for
>> debugging it's a bit painful. Can we live with that?
>
> Sure, debugging udev rules is somewhat tricky in general (requires
> manually triggering udev). When asking users to test udev rule / hwdb
> patches I usually just ask them to reboot.
>
>>> I'm adding the *and* the backlight/brightness property has been accessed
>>> condition so that udev can still do its thing while a boot splash
>>> screen is active. This assumes that boot splash screens do not
>>> touch the brightness.
>>
>> I presume udev will have to do its job before the drm node is opened, I
>> think relying on the userspace not touching the brightness property is
>> going to be racy/flaky.
>
> Making sure that udev does its job before we show the splashscreen
> is tricky, note the idea is that the splashscreen *never* touches
> the brightness property and by the time regular X / wayland launches
> udev should have had plenty of time to complete its job.
>
> I do not know how hard it will be to add the code to detect triggering
> the property being touched, but that is the best I can come up with
> to still allow the bootsplash (e.g. plymouth) to use the drm-node.

p.s.

I realize this aint pretty, alternatively we could just document that
root may change the back-end of the property and that in that case
the max value may change and that it is up to userspace to make sure
that it deals with this (e.g. by not changing the backend at a bad
time).

Regards,

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

Re: KMS backlight ABI proposition

[Hans de Goede]

Hi,

On 24-02-17 10:48, Hans de Goede wrote:
> Hi,
>
> On 24-02-17 10:46, Hans de Goede wrote:
>> Hi,
>>
>> On 24-02-17 10:34, Jani Nikula wrote:
>>> On Fri, 24 Feb 2017, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> On 24-02-17 09:43, Jani Nikula wrote:
>>>>> I don't think we have any good ideas how to solve the property max
>>>>> changing on the fly. But based on the discussion so far, it's starting
>>>>> to look like we'll need to study that more thoroughly.
>>>>
>>>> As I mentioned in another part of the thread, I think we can just return
>>>> -EBUSY if udev tries to change the binding when a drm-node is open
>>>> *and* the backlight/brightness property has been accessed (even if
>>>> read only) through that drm-node. We can reset the busy flag when
>>>> the (last open) drm-node gets closed.
>>>
>>> That's certainly easier than coming up with ways to notify the userspace
>>> about property range changes. The obvious downside is that you have to
>>> kill X to do the re-association. That's fine for when everything just
>>> works (i.e. everything happens before the drm node is opened), but for
>>> debugging it's a bit painful. Can we live with that?
>>
>> Sure, debugging udev rules is somewhat tricky in general (requires
>> manually triggering udev). When asking users to test udev rule / hwdb
>> patches I usually just ask them to reboot.
>>
>>>> I'm adding the *and* the backlight/brightness property has been accessed
>>>> condition so that udev can still do its thing while a boot splash
>>>> screen is active. This assumes that boot splash screens do not
>>>> touch the brightness.
>>>
>>> I presume udev will have to do its job before the drm node is opened, I
>>> think relying on the userspace not touching the brightness property is
>>> going to be racy/flaky.
>>
>> Making sure that udev does its job before we show the splashscreen
>> is tricky, note the idea is that the splashscreen *never* touches
>> the brightness property and by the time regular X / wayland launches
>> udev should have had plenty of time to complete its job.
>>
>> I do not know how hard it will be to add the code to detect triggering
>> the property being touched, but that is the best I can come up with
>> to still allow the bootsplash (e.g. plymouth) to use the drm-node.
>
> p.s.
>
> I realize this aint pretty, alternatively we could just document that
> root may change the back-end of the property and that in that case
> the max value may change and that it is up to userspace to make sure
> that it deals with this (e.g. by not changing the backend at a bad
> time).

Sorry for all the mails, I just realized that this (making it userspace's
problem entirely) is exactly what the input subsystem does. Things like
setkeycodes, but esp. also the fixing of min/max value for absolute axis
for touchpads are done by /lib/udev/hwdb.d/60-evdev.hwdb and the
unwritten rule there is that udev needs to do this before wayland or
xorg start using the touchpad and queries the min/max values (which it
does once and then caches the results).

I just checked the implementation of the EVIOCSABS ioctl and it does
not check that the device is not in use while the changes are applied.

So I think the best solution here is to just document that changing the
backend and thus the max value while it is being used is not the best
idea, but is allowed by the kernel.

Regards,

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

Re: KMS backlight ABI proposition

[Martin Peres]

On 24/02/17 11:59, Hans de Goede wrote:
> Hi,
>
> On 24-02-17 10:48, Hans de Goede wrote:
>> Hi,
>>
>> On 24-02-17 10:46, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 24-02-17 10:34, Jani Nikula wrote:
>>>> On Fri, 24 Feb 2017, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> On 24-02-17 09:43, Jani Nikula wrote:
>>>>>> I don't think we have any good ideas how to solve the property max
>>>>>> changing on the fly. But based on the discussion so far, it's
>>>>>> starting
>>>>>> to look like we'll need to study that more thoroughly.
>>>>>
>>>>> As I mentioned in another part of the thread, I think we can just
>>>>> return
>>>>> -EBUSY if udev tries to change the binding when a drm-node is open
>>>>> *and* the backlight/brightness property has been accessed (even if
>>>>> read only) through that drm-node. We can reset the busy flag when
>>>>> the (last open) drm-node gets closed.
>>>>
>>>> That's certainly easier than coming up with ways to notify the
>>>> userspace
>>>> about property range changes. The obvious downside is that you have to
>>>> kill X to do the re-association. That's fine for when everything just
>>>> works (i.e. everything happens before the drm node is opened), but for
>>>> debugging it's a bit painful. Can we live with that?
>>>
>>> Sure, debugging udev rules is somewhat tricky in general (requires
>>> manually triggering udev). When asking users to test udev rule / hwdb
>>> patches I usually just ask them to reboot.
>>>
>>>>> I'm adding the *and* the backlight/brightness property has been
>>>>> accessed
>>>>> condition so that udev can still do its thing while a boot splash
>>>>> screen is active. This assumes that boot splash screens do not
>>>>> touch the brightness.
>>>>
>>>> I presume udev will have to do its job before the drm node is opened, I
>>>> think relying on the userspace not touching the brightness property is
>>>> going to be racy/flaky.
>>>
>>> Making sure that udev does its job before we show the splashscreen
>>> is tricky, note the idea is that the splashscreen *never* touches
>>> the brightness property and by the time regular X / wayland launches
>>> udev should have had plenty of time to complete its job.
>>>
>>> I do not know how hard it will be to add the code to detect triggering
>>> the property being touched, but that is the best I can come up with
>>> to still allow the bootsplash (e.g. plymouth) to use the drm-node.
>>
>> p.s.
>>
>> I realize this aint pretty, alternatively we could just document that
>> root may change the back-end of the property and that in that case
>> the max value may change and that it is up to userspace to make sure
>> that it deals with this (e.g. by not changing the backend at a bad
>> time).
>
> Sorry for all the mails, I just realized that this (making it userspace's
> problem entirely) is exactly what the input subsystem does. Things like
> setkeycodes, but esp. also the fixing of min/max value for absolute axis
> for touchpads are done by /lib/udev/hwdb.d/60-evdev.hwdb and the
> unwritten rule there is that udev needs to do this before wayland or
> xorg start using the touchpad and queries the min/max values (which it
> does once and then caches the results).
>
> I just checked the implementation of the EVIOCSABS ioctl and it does
> not check that the device is not in use while the changes are applied.
>
> So I think the best solution here is to just document that changing the
> backend and thus the max value while it is being used is not the best
> idea, but is allowed by the kernel.

What's wrong with sending a hotplug event upon changing the backlight 
driver? This would work even when X is running!

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

Re: KMS backlight ABI proposition

[Hans de Goede]

Hi,

On 24-02-17 11:23, Martin Peres wrote:
> On 24/02/17 11:59, Hans de Goede wrote:
>> Hi,
>>
>> On 24-02-17 10:48, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 24-02-17 10:46, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 24-02-17 10:34, Jani Nikula wrote:
>>>>> On Fri, 24 Feb 2017, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>> On 24-02-17 09:43, Jani Nikula wrote:
>>>>>>> I don't think we have any good ideas how to solve the property max
>>>>>>> changing on the fly. But based on the discussion so far, it's
>>>>>>> starting
>>>>>>> to look like we'll need to study that more thoroughly.
>>>>>>
>>>>>> As I mentioned in another part of the thread, I think we can just
>>>>>> return
>>>>>> -EBUSY if udev tries to change the binding when a drm-node is open
>>>>>> *and* the backlight/brightness property has been accessed (even if
>>>>>> read only) through that drm-node. We can reset the busy flag when
>>>>>> the (last open) drm-node gets closed.
>>>>>
>>>>> That's certainly easier than coming up with ways to notify the
>>>>> userspace
>>>>> about property range changes. The obvious downside is that you have to
>>>>> kill X to do the re-association. That's fine for when everything just
>>>>> works (i.e. everything happens before the drm node is opened), but for
>>>>> debugging it's a bit painful. Can we live with that?
>>>>
>>>> Sure, debugging udev rules is somewhat tricky in general (requires
>>>> manually triggering udev). When asking users to test udev rule / hwdb
>>>> patches I usually just ask them to reboot.
>>>>
>>>>>> I'm adding the *and* the backlight/brightness property has been
>>>>>> accessed
>>>>>> condition so that udev can still do its thing while a boot splash
>>>>>> screen is active. This assumes that boot splash screens do not
>>>>>> touch the brightness.
>>>>>
>>>>> I presume udev will have to do its job before the drm node is opened, I
>>>>> think relying on the userspace not touching the brightness property is
>>>>> going to be racy/flaky.
>>>>
>>>> Making sure that udev does its job before we show the splashscreen
>>>> is tricky, note the idea is that the splashscreen *never* touches
>>>> the brightness property and by the time regular X / wayland launches
>>>> udev should have had plenty of time to complete its job.
>>>>
>>>> I do not know how hard it will be to add the code to detect triggering
>>>> the property being touched, but that is the best I can come up with
>>>> to still allow the bootsplash (e.g. plymouth) to use the drm-node.
>>>
>>> p.s.
>>>
>>> I realize this aint pretty, alternatively we could just document that
>>> root may change the back-end of the property and that in that case
>>> the max value may change and that it is up to userspace to make sure
>>> that it deals with this (e.g. by not changing the backend at a bad
>>> time).
>>
>> Sorry for all the mails, I just realized that this (making it userspace's
>> problem entirely) is exactly what the input subsystem does. Things like
>> setkeycodes, but esp. also the fixing of min/max value for absolute axis
>> for touchpads are done by /lib/udev/hwdb.d/60-evdev.hwdb and the
>> unwritten rule there is that udev needs to do this before wayland or
>> xorg start using the touchpad and queries the min/max values (which it
>> does once and then caches the results).
>>
>> I just checked the implementation of the EVIOCSABS ioctl and it does
>> not check that the device is not in use while the changes are applied.
>>
>> So I think the best solution here is to just document that changing the
>> backend and thus the max value while it is being used is not the best
>> idea, but is allowed by the kernel.
>
> What's wrong with sending a hotplug event upon changing the backlight driver? This would work even when X is running!

Propagating that is going to be tricky, does randr even allow changing
the range of a property after it has been registered ?

With that said, yes sending a change uevent when this changes probably
is a good idea regardless.

Regards,

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

Re: KMS backlight ABI proposition

[Martin Peres]

On 24/02/17 12:44, Hans de Goede wrote:
> Hi,
>
> On 24-02-17 11:23, Martin Peres wrote:
>> On 24/02/17 11:59, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 24-02-17 10:48, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 24-02-17 10:46, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 24-02-17 10:34, Jani Nikula wrote:
>>>>>> On Fri, 24 Feb 2017, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>> On 24-02-17 09:43, Jani Nikula wrote:
>>>>>>>> I don't think we have any good ideas how to solve the property max
>>>>>>>> changing on the fly. But based on the discussion so far, it's
>>>>>>>> starting
>>>>>>>> to look like we'll need to study that more thoroughly.
>>>>>>>
>>>>>>> As I mentioned in another part of the thread, I think we can just
>>>>>>> return
>>>>>>> -EBUSY if udev tries to change the binding when a drm-node is open
>>>>>>> *and* the backlight/brightness property has been accessed (even if
>>>>>>> read only) through that drm-node. We can reset the busy flag when
>>>>>>> the (last open) drm-node gets closed.
>>>>>>
>>>>>> That's certainly easier than coming up with ways to notify the
>>>>>> userspace
>>>>>> about property range changes. The obvious downside is that you
>>>>>> have to
>>>>>> kill X to do the re-association. That's fine for when everything just
>>>>>> works (i.e. everything happens before the drm node is opened), but
>>>>>> for
>>>>>> debugging it's a bit painful. Can we live with that?
>>>>>
>>>>> Sure, debugging udev rules is somewhat tricky in general (requires
>>>>> manually triggering udev). When asking users to test udev rule / hwdb
>>>>> patches I usually just ask them to reboot.
>>>>>
>>>>>>> I'm adding the *and* the backlight/brightness property has been
>>>>>>> accessed
>>>>>>> condition so that udev can still do its thing while a boot splash
>>>>>>> screen is active. This assumes that boot splash screens do not
>>>>>>> touch the brightness.
>>>>>>
>>>>>> I presume udev will have to do its job before the drm node is
>>>>>> opened, I
>>>>>> think relying on the userspace not touching the brightness
>>>>>> property is
>>>>>> going to be racy/flaky.
>>>>>
>>>>> Making sure that udev does its job before we show the splashscreen
>>>>> is tricky, note the idea is that the splashscreen *never* touches
>>>>> the brightness property and by the time regular X / wayland launches
>>>>> udev should have had plenty of time to complete its job.
>>>>>
>>>>> I do not know how hard it will be to add the code to detect triggering
>>>>> the property being touched, but that is the best I can come up with
>>>>> to still allow the bootsplash (e.g. plymouth) to use the drm-node.
>>>>
>>>> p.s.
>>>>
>>>> I realize this aint pretty, alternatively we could just document that
>>>> root may change the back-end of the property and that in that case
>>>> the max value may change and that it is up to userspace to make sure
>>>> that it deals with this (e.g. by not changing the backend at a bad
>>>> time).
>>>
>>> Sorry for all the mails, I just realized that this (making it
>>> userspace's
>>> problem entirely) is exactly what the input subsystem does. Things like
>>> setkeycodes, but esp. also the fixing of min/max value for absolute axis
>>> for touchpads are done by /lib/udev/hwdb.d/60-evdev.hwdb and the
>>> unwritten rule there is that udev needs to do this before wayland or
>>> xorg start using the touchpad and queries the min/max values (which it
>>> does once and then caches the results).
>>>
>>> I just checked the implementation of the EVIOCSABS ioctl and it does
>>> not check that the device is not in use while the changes are applied.
>>>
>>> So I think the best solution here is to just document that changing the
>>> backend and thus the max value while it is being used is not the best
>>> idea, but is allowed by the kernel.
>>
>> What's wrong with sending a hotplug event upon changing the backlight
>> driver? This would work even when X is running!
>
> Propagating that is going to be tricky, does randr even allow changing
> the range of a property after it has been registered ?

It does, unless the property was set immutable.

>
> With that said, yes sending a change uevent when this changes probably
> is a good idea regardless.

I think that solves the problem pretty nicely, so yes :)
Will wait until monday to send an updated email with the new proposal.

Dave, I would really like to hear more about your thoughts on this. Did 
we explain sufficiently well why we think pushing work onto the 
userspace will not work?

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

Re: KMS backlight ABI proposition

[Daniel Thompson]

On 24/02/17 08:43, Jani Nikula wrote:
 >> - "Some PWM based backlight allow adjusting the PWM modulation
 >> frequency." you don't need a motivation for *why* I would want to
 >> change the mod freq on the fly, actually in my experience you
 >> shouldn't since this can lead to flickery backlights.
 >
 > The modulation frequency is usually an OEM design choice. The higher
 > the
 > frequency, less flicker, but also fewer user distinguishable levels of
 > backlight (signal rise/fall times come into play). Occasionally there
 > have been requests to be able to adjust the frequency. We can of course
 > decide that's an implementation detail and not let userspace change it.

In principle the backlight max level could also change during monitor 
hotplug; recent versions DDC/CI include a backlight properties. We have 
no driver for this right now but conversation is starting (and proper 
backlight support in DRM connectors will probably help it happen faster).

I'm afraid I have no idea if this is "real" or now. Right now I have no 
clue how many monitors implement it. For regular desktop monitors which 
mirror old school brightness/contrast in their UI then 0 wouldn't 
surprise me much ;-)


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

Re: KMS backlight ABI proposition

[Jani Nikula]

On Fri, 24 Feb 2017, Daniel Thompson <daniel.thompson@linaro.org> wrote:
> On 24/02/17 08:43, Jani Nikula wrote:
>  >> - "Some PWM based backlight allow adjusting the PWM modulation
>  >> frequency." you don't need a motivation for *why* I would want to
>  >> change the mod freq on the fly, actually in my experience you
>  >> shouldn't since this can lead to flickery backlights.
>  >
>  > The modulation frequency is usually an OEM design choice. The higher
>  > the
>  > frequency, less flicker, but also fewer user distinguishable levels of
>  > backlight (signal rise/fall times come into play). Occasionally there
>  > have been requests to be able to adjust the frequency. We can of course
>  > decide that's an implementation detail and not let userspace change it.
>
> In principle the backlight max level could also change during monitor 
> hotplug; recent versions DDC/CI include a backlight properties. We have 
> no driver for this right now but conversation is starting (and proper 
> backlight support in DRM connectors will probably help it happen faster).
>
> I'm afraid I have no idea if this is "real" or now. Right now I have no 
> clue how many monitors implement it. For regular desktop monitors which 
> mirror old school brightness/contrast in their UI then 0 wouldn't 
> surprise me much ;-)

Thanks for the reminder. I think so far this as worked by using the DDC
I2C device nodes directly. And it really works already to some extent,
on some displays. It would be nice to have this controlled the same way
as embedded diplays.

Another interesting aspect with DDC/CI brightness control, one that it
shares with eDP DPCD and DSI DCS brightness controls, is that it could
all happen within DRM, mostly in a generic, driver independent
manner. There would not be a need for a backlight class device or sysfs
API at all, except for legacy needs.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: KMS backlight ABI proposition

[Jasper St. Pierre]

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

On Thu, Feb 23, 2017 at 12:40 AM, Jani Nikula <jani.nikula@linux.intel.com>
wrote:

> On Wed, 22 Feb 2017, Stéphane Marchesin <stephane.marchesin@gmail.com>
> wrote:
> > On Fri, Feb 17, 2017 at 4:58 AM, Martin Peres
> > <martin.peres@linux.intel.com> wrote:
> >> If the KMS property exposes a fixed number of steps (say 100), it
> becomes
> >> easy for the userspace to express the wanted brightness. However, on
> drivers
> >> exposing less than these 100 steps, we cannot guarantee that any change
> in
> >> the value will produce any change. If there is only one possible value
> (on
> >> or off), the user may be trying the change the brightness, a GUI would
> show
> >> what is the expected backlight state, but no change in the luminance
> would
> >> be seen, which is pretty bad.
> >
> > Yes, I don't think we want to normalize anything here. It would
> > essentially be hiding functionality from user space, which then can't
> > expose it in the user interface. As you say, if the backlight slider
> > moves, but the backlight level didn't change, that's weird. On the
> > other hand if user space knows the number of levels it can give you a
> > consistent slider, and normalizing in user space is not that hard
> > (that's how things currently work after all, so people should be used
> > to it).
>
> I listed some of the benefits of normalizing (or re-ranging) in
> [1]. Conversely, I haven't seen good answers on how to gracefully handle
> the brightness range changing on the fly. That is what not normalizing
> would mean. I don't think the current property implementation even
> allows changing the range. And then there'd have to be a way to tell the
> userspace that the range has changed.
>
> In the same message, I mentioned the idea of providing an API to
> increase/decrease brightness. That might be much easier to implement
> than allowing the property range change.
>
> [1] http://marc.info/?i=87mvdei7ug.fsf@intel.com
>

As a consumer of this API, I need the step size. If the max changes and we
have a normalization, then the step size changes, and we run into the same
exact set of problems where now "+5" means something completely different
and I need to know that.


> > Yes the ability to turn off the backlight is important. Some
> > backlights are not stable at low levels, so they don't expose these
> > low levels and effectively level 0 is not off (it is the lowest level
> > which works). So I guess the question is how should that non-linearity
> > be exposed versus the ability to turn it off completely.
>
> You fail to say *why* the ability to turn off the backlight is
> important. I've seen it used as a kind of "light DPMS" that can be done
> using the sysfs interface, but I think that's a hack, really. Here,
> whoever changes the backlight would be doing it using the DRM APIs
> anyway, so it could do actual DPMS anyway. And, of course, not all
> backlight hardware is able to switch off the backlight, and not all
> drivers will be able to say whether 0 is off or not.
>
> >> 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.
> >
> > Are we saying that we are putting the kernel in charge of  display vs
> > backlight sequencing? Currently on some ARM boards with separate pwm
> > backlight drivers that's not the case. Don't get me wrong, I think the
> > kernel should be in charge of enforcing sequencing because otherwise
> > user space can damage hardware, I'm just pointing out that right now
> > it isn't the case.
>
> Whenever the kernel is able to enforce the sequencing, it should. I
> believe this is the case for most native backlight implementations. And
> in these cases the backlight on/off toggling would really have to be a
> substate of enabled display; can't enable backlight without display
> enabled.
>
> BR,
> Jani.
>
>
> --
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>



-- 
  Jasper

[-- Attachment #1.2: Type: text/html, Size: 5997 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