[Intel-gfx] [RFC PATCH 2/2] drm/i915: respect the VBT minimum backlight brightness

[Stéphane Marchesin]

On Wed, Jun 4, 2014 at 2:11 AM, Jani Nikula <jani.nikula at intel.com> wrote:
> On Wed, 04 Jun 2014, Stéphane Marchesin <marcheu at chromium.org> wrote:
>> On Tue, Jun 3, 2014 at 1:26 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
>>> On Tue, Jun 3, 2014 at 6:40 PM, Stéphane Marchesin <marcheu at chromium.org> wrote:
>>>> On Tue, Apr 29, 2014 at 1:30 PM, Jani Nikula <jani.nikula at intel.com> wrote:
>>>>> Historically we've exposed the full backlight PWM duty cycle range to
>>>>> the userspace, in the name of "mechanism, not policy". However, it turns
>>>>> out there are both panels and board designs where there is a minimum
>>>>> duty cycle that is required for proper operation. The minimum duty cycle
>>>>> is available in the VBT.
>>>>>
>>>>> The backlight class sysfs interface does not make any promises to the
>>>>> userspace about the physical meaning of the range
>>>>> 0..max_brightness. Specifically there is no guarantee that 0 means off;
>>>>> indeed for acpi_backlight 0 usually is not off, but the minimum
>>>>> acceptable value.
>>>>
>>>> This is the part we've been relying on in Chrome OS (we assume that 0
>>>> means off). It seems to me that i915 would be the first, or one of the
>>>> first drivers to violate this rule (in particular you can find a lot
>>>> of backlight drivers trying hard to ensure that 0 means off in the
>>>> backlight drivers directory).
>>>>
>>>> For context, we use backlight = 0 as a quick "turn the panel off"
>>>> function where possible. This allows us to avoid the panel off delay
>>>> where possible. Breaking this assumption means that we'd pay the panel
>>>> off delay penalty everywhere, not just with BYT.
>>>
>>> Well kde is the opposite and I've had users yelling at me that they
>>> can't use their system, since acpi pretty much always leaves the
>>> backlight on when set to 0. And acpi kinda rules on intel platforms.
>>> So I think I'll merge this one since black screens trumps a slight
>>> functional degration and we didn't duct-tape over the kde assumptions
>>> either. Essentially you can't assume anything about backlight values
>>> besides that bigger values tends to mean brigther. Linearity, absolute
>>> values and endpoints are kinda all off.
>>
>> It seems fairly wrong to model the backlight behavior after the only
>> thing everyone agrees is broken (ACPI).
>
> Please don't put words into everyone's mouth.
>
> Talking to folks on IRC, Dave Airlie said, "I think its been disagreed
> over the years to be implementation dependent." and Matthew Garrett
> said, "It's currently implementation dependent. It can never mean off
> under all circumstances (not all interfaces offer a way to turn it
> off)."
>
> I don't know whether they think ACPI is broken, but clearly there's no
> one true answer, and I think it's not the correct approach for userspace
> to assume 0 means off.

Well, for example ACPI backlight isn't the default on intel, instead
i915 implements its own backlight. ACPI backlight doesn't support as
many backlight levels as the native backlight for example. So I do
think that ACPI backlight is inferior.

>
>> If you go through the backlight drivers, (at least all the ones I've
>> looked at) ensure that 0 really means off.
>
> And systemd goes out of it's way to not switch backlight off on those
> interfaces... and it's obvious they can't assume anything about the
> meaning of 0 either:
>
> http://cgit.freedesktop.org/systemd/systemd/commit/?id=7b909d7407965c03caaba30daae7aee113627a83

This seems unrelated, they just turn the backlight to non-zero to make
things visible?

>
>>> If we want to specifically expose an intermediate "panel off" level
>>> because the full link training is too expensive we imo should do this
>>> as an intermediate dpms level, e.g. dpms standby.
>>
>> Training isn't the problem, the panel delays are the problem here (and
>> not something we can work around because it's part of the panel
>> specifications which we have to follow). Using dpms wouldn't solve
>> anything.
>
> We could have dpms standby mean backlight off, everything else on,
> similarly to what you want by 0 backlight meaning off. So you could
> switch between dpms on/standby more quickly. The point in that is that
> it's the right API for doing panel power sequencing.
>
> You see, even with defining backlight 0 = off, we'd have to respect the
> panel sequencing and delays for backlight (admittedly we currently don't
> do that, which is why it's fast - and broken).

There are multiple delays at play here, and typically the longest
panel delays have nothing to do with the backlight but they are the
delays for turning the panel on/off (usually the panel on/off delays
are much bigger, up to 500ms on some panels). These are the delays I'm
trying to avoid.

> Doing the panel
> sequencing properly from the backlight interface gets *much* harder
> because it's all backwards in the stack then.

If you implement some interface which does "set backlight to 0 on
platforms which support it, and do panel off on other platforms",
that's fine by me; however I don't want to regress all the other
platforms just because BYT has issues. In the current state of things,
the functionality set which is made available to user space happens to
shrink with your patch.

Stéphane