Le mar. 25 juin 2019 à 5:47, Thierry Reding <thierry.reding@xxxxxxxxx>
a écrit :
On Mon, Jun 24, 2019 at 04:31:57PM +0200, Paul Cercueil wrote:
Le lun. 24 juin 2019 à 13:28, Daniel Thompson
<daniel.thompson@xxxxxxxxxx> a
écrit :
> On Fri, Jun 21, 2019 at 03:56:08PM +0200, Thierry Reding wrote:
> > On Fri, Jun 21, 2019 at 01:41:45PM +0100, Daniel Thompson
wrote:
> > > On 22/05/2019 17:34, Paul Cercueil wrote:
> > > > When the driver probes, the PWM pin is automatically
configured
> > to its
> > > > default state, which should be the "pwm" function.
> > >
> > > At which point in the probe... and by who?
> >
> > The driver core will select the "default" state of a device
right
> > before
> > calling the driver's probe, see:
> >
> > drivers/base/pinctrl.c: pinctrl_bind_pins()
> >
> > which is called from:
> >
> > drivers/base/dd.c: really_probe()
> >
>
> Thanks. I assumed it would be something like that... although
given
> pwm-backlight is essentially a wrapper driver round a PWM I
wondered why
> the pinctrl was on the backlight node (rather than the PWM node).
>
> Looking at the DTs in the upstream kernel it looks like ~20% of
the
> backlight drivers have pinctrl on the backlight node. Others
presumable
> have none or have it on the PWM node (and it looks like support
for
> sleeping the pins is *very* rare amoung the PWM drivers).
If your PWM driver has more than one channel and has the pinctrl
node, you
cannot fine-tune the state of individual pins. They all share the
same
state.
But that's something that could be changed, right? We could for
example
extend the PWM bindings to allow describing each PWM instance via a
sub-
node of the controller node. Pin control states could be described on
a
per-channel basis that way.
There could be an API to dynamically add/remove pin groups to a given
pinctrl state. The PWM driver would start with an empty (no groups)
"default" state, then when enabling e.g. PWM1, the driver would call
a function to add the "pwm1" pin group to the "default" state.
Does that sound like a good idea?
Thanks,
-Paul
> > > > However, at this
> > > > point we don't know the actual level of the pin, which may
be
> > active or
> > > > inactive. As a result, if the driver probes without
enabling the
> > > > backlight, the PWM pin might be active, and the backlight
would
> > be
> > > > lit way before being officially enabled.
> > > >
> > > > To work around this, if the probe function doesn't enable
the
> > backlight,
> > > > the pin is set to its sleep state instead of the default
one,
> > until the
> > > > backlight is enabled. Whenk the backlight is disabled, the
pin
> > is reset
> > > > to its sleep state.
> > > Doesn't this workaround result in a backlight flash between
> > whatever enables
> > > it and the new code turning it off again?
> >
> > Yeah, I think it would. I guess if you're very careful on how
you
> > set up
> > the device tree you might be able to work around it. Besides
the
> > default
> > and idle standard pinctrl states, there's also the "init"
state. The
> > core will select that instead of the default state if
available.
> > However
> > there's also pinctrl_init_done() which will try again to
switch to
> > the
> > default state after probe has finished and the driver didn't
switch
> > away
> > from the init state.
> >
> > So you could presumably set up the device tree such that you
have
> > three
> > states defined: "default" would be the one where the PWM pin is
> > active,
> > "idle" would be used when backlight is off (PWM pin inactive)
and
> > then
> > another "init" state that would be the same as "idle" to be
used
> > during
> > probe. During probe the driver could then switch to the "idle"
> > state so
> > that the pin shouldn't glitch.
> >
> > I'm not sure this would actually work because I think the way
that
> > pinctrl handles states both "init" and "idle" would be the same
> > pointer
> > values and therefore pinctrl_init_done() would think the driver
> > didn't
> > change away from the "init" state because it is the same
pointer
> > value
> > as the "idle" state that the driver selected. One way to work
around
> > that would be to duplicate the "idle" state definition and
> > associate one
> > instance of it with the "idle" state and the other with the
"init"
> > state. At that point both states should be different (different
> > pointer
> > values) and we'd get the init state selected automatically
before
> > probe,
> > select "idle" during probe and then the core will leave it
alone.
> > That's
> > of course ugly because we duplicate the pinctrl state in DT,
but
> > perhaps
> > it's the least ugly solution.
> > Adding Linus for visibility. Perhaps he can share some insight.
>
> To be honest I'm happy to summarize in my head as "if it flashes
then
> it's not
> a pwm_bl.c's problem" ;-).
It does not flash. But the backlight lits way too early, so we have
a 1-2
seconds
of "white screen" before the panel driver starts.
I think this always goes both ways. If you set the sleep state for the
PWM on backlight probe with this patch, you may be able to work around
the problem of the backlight lighting up too early. But what if your
bootloader had already enabled the backlight and is showing a splash
screen during boot? Your patch would turn off the backlight and then
it
would turn on again after everything else was initialized. That's one
type of flashing.
What we need in this case are explicit pin control states that will
enable fine-grained control over what happens. Anything implicit is
bound to fail because it bakes in an assumption (either that the
backlight is off during boot, or that it has been turned on already).
Ideally we'd need to detect that the backlight is on and if it is we
just don't do anything with it. Actually, I think that's what we want
even if the backlight is off. During probe the backlight state should
not be modified. You only want to modify it when you know that some
display driver is going to take over. If you can't seamlessly
transition
to the kernel display driver, flashing may be okay. If your display
driver can take over seamlessly, then the backlight is likely already
in
the desired state anyway.
Thierry
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
