[PATCH] drm/panel: simple: Initialize unprepared_time in probe

[Doug Anderson]

Hi,

On Tue, Jul 18, 2023 at 10:37 AM Marek Vasut <marex at denx.de> wrote:
>
> On 7/18/23 18:15, Doug Anderson wrote:
> > Hi,
>
> Hi,
>
> > On Tue, Jul 18, 2023 at 8:36 AM Marek Vasut <marex at denx.de> wrote:
> >>
> >> On 7/18/23 16:17, Doug Anderson wrote:
> >>> Hi,
> >>
> >> Hi,
> >>
> >>> On Sun, Jul 9, 2023 at 6:52 AM Marek Vasut <marex at denx.de> wrote:
> >>>>
> >>>> The unprepared_time has to be initialized during probe to probe time
> >>>> ktime, otherwise panel_simple_resume() panel_simple_wait() call may
> >>>> wait too short time, or no time at all, which would violate the panel
> >>>> timing specification. Initializing the unprepared_time() to probe time
> >>>> ktime assures the delay is at least what the panel requires from the
> >>>> time kernel started. The unprepared_time is then updated every time
> >>>> the panel is suspended in panel_simple_suspend() too.
> >>>>
> >>>> Fixes: e5e30dfcf3db ("drm: panel: simple: Defer unprepare delay till next prepare to shorten it")
> >>>> Signed-off-by: Marek Vasut <marex at denx.de>
> >>>
> >>> Can you talk in more detail about the problem you're seeing? Your
> >>> patch will likely cause boot speed regressions. While correctness
> >>> trumps performance, I'd like to make sure this is right before landing
> >>> it.
> >>
> >> With AUO T215HVN01 panel, connected to LT9211 DSI-to-LVDS bridge,
> >> connected to MX8M Mini DSIM , the panel just would not come up correctly
> >> because this unprepare_time is not observed. The panel would only show
> >> blue stripe on the left side, instead of actual image.
> >>
> >>> Specifically, I think your patch is nearly the opposite as what I did
> >>> in commit 691c1fcda535 ("regulator: core: Shorten off-on-delay-us for
> >>> always-on/boot-on by time since booted"). I think many of the same
> >>> arguments I made in that commit message argue against your patch.
> >>
> >> You cannot guarantee in which state the panel is after boot/reboot,
> >
> > Agreed. To the best extent possible, whatever solution we arrive at
> > should work regardless of how the bootloader left things.
> >
> >
> >> so
> >> I believe the kernel has to shut it down, and then bring it up, with the
> >> correct timings.
> >
> > If that's required for your panel then the driver should do what it
> > needs to do to ensure this.
>
> The panel-simple driver used to do it. Now it no longer does, which
> means the kernel is now running this AUO and possibly other panels out
> of specification.

OK, I think the more I read this thread the more confused I get. :(
Hopefully we can arrive at some clarity.

1. I guess first off, nothing about the old kernel would have ensured
that the regulator would have been shut off. Looking at the old code
(before e5e30dfcf3db, the commit yous "Fixes") the panel-simple driver
just did:

regulator_get()
regulator_enable()

If the regulator was left on by the bootloader and managed by a
regulator driver that can read back initial regulator states then the
old driver would have done nothing at all to guarantee that a
regulator went off. If you want some proof of this, it's even
documented in `Documentation/power/regulator/consumer.rst`:

NOTE:
  The supply may already be enabled before regulator_enabled() is called.
  This may happen if the consumer shares the regulator or the regulator has been
  previously enabled by bootloader or kernel board initialization code.

If you really need to make sure that your regulator was disabled at
boot, you could probably do something like this psuedocode:

supply = regulator_get(...)
if (regulator_is_enabled(supply)) {
  /* Enable and disable and that should sync it up */
  regulator_enable(supply);
  regulator_disable(supply);
  if (regulator_is_enabled(supply)) {
    pr_err("Crud, we couldn't disable\n");
    return -E_LIFESUCKS;
  }
}


2. Looking more closely at the commit you're fixing, though, I'm even
more confused.

I _think_ your assertion here is that the longer delay is needed on
the first power on of the panel at bootup. Is that correct? This is
why you need to initialize "unprepared_time" in the probe() function.
However, when I go back to the old code (before e5e30dfcf3db, the
commit yours "Fixes") you can actually see that there was no delay at
all before the first power on of the panel. The only delay was if you
turned the panel off and then turned it back on again. ...so the only
thing that the commit should have broken would have been the power-ons
of the panel _after_ the first. ...but your patch only affects the
delay for the first power on.

Huh?


> > As indicated by my other comments, I
> > actually don't think your patch currently does in all cases. If the
> > panel is powered by a PMIC and the bootloader left the power on, your
> > patch series _won't_ shut it down and bring it back up, will it?
>
> That depends on the regulator configuration. That itself is a separate
> issue however, one which has been present even before any of this boot
> time optimization attempt.
>
> > In any case, if your panel requires extra delays, it would be ideal if
> > this didn't inflict a penalty on all panels. I haven't personally
> > worked on any panels currently serviced by panel-simple, but for most
> > eDP panels the only strong timing requirement is that once you turn
> > off the main power rail that you don't turn it on again for ~500ms.
>
> The extra delay is actually only inflicted on panels which do set delay
> { .unprepare = ... } constraint in their timing specification, and those
> panels most certainly do need those extra delays to operate correctly.
>
> > For most panels it's OK to turn it on early (like as soon as the
> > regulator proves) and also OK if the main power rail stays on between
> > the bootloader and the kernel.
>
> I would debate the "most" part, as that is not my experience with DPI
> and LVDS panels, which, if they are not correctly power sequenced, can
> go all kinds of weird and that weirdness is often very subtle. Or worse,
> those panels start failing in deployment.
>
> > For eDP the one exception I've seen was
> > the "samsung-atna33xc20" panel and that panel has its own driver
> > specifically to deal with quirks like this. I talk about this a little
> > bit in commit 23ff866987de ("arm64: dts: qcom: sc7180: Start the
> > trogdor eDP/touchscreen regulator on") since homestar uses
> > "samsung-atna33xc20"
>
> I seldom work with eDP panels, so I cannot comment on that part.
>
> It is well possible the more complex electronics of the panel hides a
> lot of the power sequencing details, I wouldn't be surprised by that.
>
> >>> ...however, I guess in the case of the panel, things could be
> >>> different because regulators aren't directly controlled by the panel
> >>> code. Thus, I could imagine that your situation is this:
> >>>
> >>> 1. Bootloader runs and leaves the panel powered on.
> >>
> >> Bootloader does not touch the panel at all.
> >
> > Huh, then I'm pretty confused. Where is the timing violation then? If
> > the panel was off when the device started booting and the bootloader
> > didn't touch the panel, then the existing code should work fine. The
> > current code will make sure that we delay at least "unprepare" ms
> > since the kernel booted and so no specs should be violated.
> >
> > Are you sure you aren't running into something like a case of
> > -EPROBE_DEFER where panel-simple powers the regulator on, then
> > un-probes, and then tries probing again? ...or maybe the default state
> > of the regulator at bootup _is_ powered on and that's the problem?
>
> Have a look at panel_simple_resume() panel_simple_wait(), this is where
> the extra delay is needed. You cannot predict how long the bootloader
> took to reach the kernel time t=0 and you cannot know what happened
> before the bootloader started (maybe abrupt sysrq reset), not on all
> platforms anyway, so the best you can do is assume the worst, i.e. full
> unprepare delay.

I feel like there is a confusion here. With the old code,
"unprepared_time" was implicitly set to 0 (because the whole structure
was zero initialized). 0 is actually a valid time and represents the
time that the kernel booted (well, more correctly when ktime finished
initting, but that's pretty early).

Let's look at a few concerte cases. In this example I'll go with what
I think you've said is happening in your system: the bootloader
doesn't touch the panel and the panels power rails are off at bootup.


Case 1: everything boots absurdly fast and "unprepared_time" is 1000 ms.

1. CPU resets and starts executing the bootloader. Panel is fully powered off.

2. Let's imagine the bootloader finishes in an absurdly fast 10 ms and
starts Linux.

3. Linux starts and inits its clock. It does this in 10 ms. Kernel
time is 0 now and it's been 20 ms since CPU reset.

4. Linux gets to panel init code after another 10 ms. Kernel time is
10 ms and it's been 20 ms since CPU reset.

5. We try to turn the panel on after another 10 ms. Kernel time is 20
ms and it's been 30 ms since CPU reset.

6. We look at kernel time (30 ms) and the unprepare delay (1000 ms)
and we'll delay 970 ms.

7. After the delay, kernel time will be 1000 ms and it will have been
1010 ms since CPU reset.

...so if the panel was truly untouched by the bootloader and the
panel's power truly initted to off at bootup then we should be fine
since it's been at least 1010 ms since the panel was powered off.


Case 2: everything boots absurdly slowly and "unprepared_time" is 1000 ms.

1. CPU resets and starts executing the bootloader. Panel is fully powered off.

2. Let's imagine the bootloader finishes in an absurdly slow 2000 ms
and starts Linux.

3. Linux starts and inits its clock. It does this in 2000 ms. Kernel
time is 0 now and it's been 4000 ms since CPU reset.

4. Linux gets to panel init code after another 2000 ms. Kernel time is
2000 ms and it's been 6000 ms since CPU reset.

5. We try to turn the panel on after another 2000 ms. Kernel time is
4000 ms and it's been 8000 ms since CPU reset.

6. We look at kernel time (4000 ms) and the unprepare delay (1000 ms)
and we'll delay 0 ms (no delay)

...so if the panel was truly untouched by the bootloader and the
panel's power truly initted to off at bootup then we should be fine
since it's been at least 8000 ms since the panel was powered off.


Since the existing code should be correctly honoring the delay in both
of the two cases, I'd like to find out what assumption is wrong.

> > In
> > either case, it feels like the regulator "off-on-delay" constraint
> > might be better here.
>
> Please stop suggesting that we should work around the existing defect of
> this driver, which does not correctly honor the .delay.unprepare time of
> a panel and causes actual failures on existing panels, instead of fixing
> it properly, only because this impedes boot time. Sure, it does, but
> correctly bringing up the panel is more important than reducing boot
> time at all costs, because if I only see blue stripe on the left side of
> the panel, I do not care if the kernel booted 100ms faster, I care about
> the non-working panel .
>
> This could be a good optimization, but it certainly is not a fix for the
> issue at hand.

There are simply too many things about the problem that don't add up.
The reason I keep pushing the off-on-delay is that it has more to do
with the root cause of the problem here.

-Doug