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

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