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