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

[Doug Anderson]

Hi,

On Sun, Jul 23, 2023 at 3:47 PM Marek Vasut <marex at denx.de> wrote:
>
> On 7/18/23 21:33, Doug Anderson wrote:
> > 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.
>
> Maybe the EPROBE_DEFER actually happens and triggers the failure ?

I could certainly believe that EPROBE_DEFER is involved. In any case,
it sounds as if you need to dig into the failure more and understand
it better before we land a fix. As it is, I don't think it's clear
how/why the delay you're adding is actually helping your situation
and, unless my logic is wrong, I don't think it's just putting the
delay back to how things were before commit e5e30dfcf3db ("drm: panel:
simple: Defer unprepare delay till next prepare to shorten it").

-Doug