Re: [PATCH] backlight: pwm_bl: Avoid backlight flicker if backlight control GPIO is input

On Thu, Jul 22, 2021 at 09:02:04PM +0200, Marek Vasut wrote:

On 7/22/21 1:28 PM, Daniel Thompson wrote:

quoted

On Wed, Jul 21, 2021 at 08:46:42PM +0200, Marek Vasut wrote:

quoted

On 7/21/21 6:12 PM, Daniel Thompson wrote:

quoted

On Wed, Jul 21, 2021 at 05:09:57PM +0200, Marek Vasut wrote:

quoted

On 7/21/21 12:49 PM, Daniel Thompson wrote:

[...]
This sails very close to the
edge of what is in-scope for DT (at least it does it we can read
the inherited state directly from the hardware).

The problem with reading it out of hardware is that the hardware might be in
undefined state and expects Linux to define that state, so that does not
always work. Hence my initial suggestion to add a DT property to define the
state up front, instead of using these fragile heuristics.

To achieve a flicker-free boot we must know the initial state of the
backlight (not just the enable pin).

The backlight hardware might be in uninitialized state and then Linux should
set the state, likely based on something in DT, because there is no previous
state to read.

There is always a previous state. The kernel doesn't care whether that
previous state was imposed by a power-on reset, the bootloader or a
kexec.

For the driver to come up flicker-free in all the different cases we
need to know whether the backlight is currently emitting light or not
and, if it is emitting light, then we need to know what the duty cycle
is (currently we inherit require the PWM driver to correctly inherit the
duty cycle from the hardware).

So far, the previous state has been observable by the lower level
drivers (GPIO, PWM, regulator). I remain reluctant to provide
workarounds for cases where it is not observable without motivating
hardware. I certainly wouldn't want to make such bindings mandatory
since observable hardware registers are a far more reliable source of
truth than what the DT tells us about what it thinks the bootloader
(or power-on reset) actually did ;-).

quoted

quoted

quoted

Overall I have fairly grave concerns that this simply moves
fragility into the bootloader rather then reducing it.

Wait a minute, I think we disconnected somewhere. I would rather prefer to
remove the fragility and bootloader dependency altogether, exactly to avoid
depending on the state the bootloader left the hardware in.

The two fully flicker-free cases we support in pwm_bl are:

1. Backlight off after bootloader has completed. Backlight must be
    off after the probe completes (and never flicker on/off during the
    probe). This allows the display to put a splash image on the screen
    before lighting up the backlight (this avoids a flicker if there are
    a few frames between backlight coming on and the splash image being
    drawn). Naturally this requires help from the display system (and
    that the display system is aware of the backlight to be able to start
    it).

2. Backlight on with a splash image after bootloader has completed.
    Backlight must be on after the probe completes (and never flicker
    off/on during the probe). This also requires that the display system
    can take over the frame buffer without a flicker but that is
    completely independent of backlight.

There is also a simpler case which is not "flicker-free" since the
backlight may change level during the boot and may light up before
there is an image on the screen (although we'd still to minimise
flicker by ensuring there is only one change of backlight state/level
during the probe (something your work will see fixed?):

Actually no, my usecase is the backlight is not initialized by the
bootloader at all, the pins are just strapped to default to the right
values, the init is left to the kernel to do.

It doesn't matter to us who established the initial state. In this case
the backlight is off at handover. That means you are either case #1
(display system will unblank" the backlight automatically when the reset
of the display unblanks) or #3 (BL driver will "unblank" the backlight
during the probe).

Your changes should result in a fix to both these cases!

quoted

3. Backlight is on after the probe completes. This is default if
    we don't know the display system will activate the backlight.
    This is an important legacy case since few userspaces know how
    to change the backlight power-state at boot.

One oddity here is that #3 *also* needs to know the state of the
backlight (on/off) to turn the backlight on without flickering
(so it can figure out how to handle power_pwm_on_delay correctly)
even though the final state is unconditionally on. That is the main
reason I proposed an alternative to your patch (since this is
currently broken).

The other oddity is that the difference between #1 and #3 is due to
*software* (which driver ends up responsible for unmuting the display)
meaning that the bootloader/DT has no business discriminating between
these two cases.

Thus pwm_bl.c is based on making #2/#3 (which are similar) the default
and switching to case #1 if there is a display driver to do the unblank
(software) *and* that the backlight is currently off (currently read
from hardware). Note that this is intentionally designed to that
if the logic does go wrong we should get a small bug (a flicker) rather
than a big one (a black screen).

Wow! That is *way* longer than I intended when I started writing it.
Anyhow I suspect any disconnect comes about due to the difference in
backlight state *after* probe being, in part, to software structure
rather than purely a hardware property.

Maybe this should be added to documentation.

I'll see what I can do.


Daniel.