KMS backlight ABI proposition

[Jani Nikula]

On Thu, 23 Feb 2017, Stéphane Marchesin <stephane.marchesin at gmail.com> wrote:
> On Thu, Feb 23, 2017 at 12:40 AM, Jani Nikula
> <jani.nikula at linux.intel.com> wrote:
>> On Wed, 22 Feb 2017, Stéphane Marchesin <stephane.marchesin at gmail.com> wrote:
>>> On Fri, Feb 17, 2017 at 4:58 AM, Martin Peres
>>> <martin.peres at 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