drm/panel: panel-simple power-off sequencing

[Jonas Mark (BT-FS/ENG1-GRB)]

Hi Doug,

Many thanks for your reply.

> Hi,
> 
> On Thu, Oct 26, 2023 at 7:37 AM Jonas Mark (BT-FS/ENG1-GRB)
> <Mark.Jonas at de.bosch.com> wrote:
> >
> > Hi,
> >
> > We have a parallel LCD panel which is driven by panel/panel-simple.
> The power-off sequence specified in the datasheet requires that the
> enable-gpio must be deasserted for a number of VSYNC cycles before
> shutting down all other control signals. See the diagram below:
> >                         __  __  __  __  __ CLK, VSYNC, DE, HSYNC:
> > __><__><__><__><__\_____________________
> >                         ______
> > enable-gpio          :        \_________________________________
> >
> > So far, in kernel 5.4 we relied on the unprepare delay time for
> making
> > sure that the enable-gpio timing requirements are fulfilled. That
> is,
> > the
> > panel_simple_unprepare() would:
> >
> > 1. Deassert the enable-gpio
> > 2. Switch off the voltage regulator
> > 3. Wait display_desc.delay.unprepare milliseconds
> >
> > Afterwards the IPU was shutdown, and all the control signals
> stopped.
> >
> > But with the below commits:
> >
> >  - 3235b0f20a0a4135e9053f1174d096eff166d0fb
> >    "drm/panel: panel-simple: Use runtime pm to avoid excessive
> unprepare / prepare"
> >  - e5e30dfcf3db1534019c40d94ed58fd2869f9359
> >    "drm: panel: simple: Defer unprepare delay till next prepare to
> shorten it"
> >
> > The enable-gpio is now deasserted in panel_simple_suspend(), which
> is called some time after the disablement of control signals are
> stopped:
> >                         __  __  __  __  __ CLK, VSYNC, DE, HSYNC:
> > __><__><__><__><__\_____________________
> >                         __________________________
> > enable-gpio          :                            \_____________
> >
> > With the latest panel-simple, is there a way which allows us to
> deassert enable-gpio before the control signals stop?
> 
> As I understood it, the "unprepare" time was originally intended to
> meet minimum power off timings and that's how I always saw it used,
> but it doesn't totally surprise me that there was someone relying on
> the old behavior. I personally wouldn't object to adding another field
> to panel-simple that allowed you to get the delay you needed and then
> change your panel details to use that new field instead of the
> "unprepare" milliseconds. ...or you could rename the current
> "unprepare" delay to something like "min_poweroff" and then re-
> introduce an "unprepare" delay that does what you want.
> 
> Oh! ...but even this won't _really_ do what you want, right? The
> bigger issue here is that panel-simple is using auto-suspend now and
> thus the enable line can go off much, much later.

Yes, exactly.

> What it kind-of sounds like is that you want the "enable" GPIO to be
> controlled by the "enable" and "disable" functions of panel-simple.
> Then you could use the "disable" delay, right?

The disable delay alone does not help. We are on an i.MX6 with a
parallel display and thus drm/imx/parallel-display.c,
imx_pd_bridge_disable() simply calls

	drm_panel_disable(imxpd->panel);
	drm_panel_unprepare(imxpd->panel);

which will then call the matching panel-simple functions. Thus, this
would only delay calling panel_simple_unprepare(). We did not see any
other exploitable side effects.

> I think I've looked at this exact case before and then realized that
> there's a better solution. At least in all cases I looked at the
> "enable-gpio" you're talking about was actually better modeled as a
> _backlight_ enable GPIO. The "backlight" is turned off before panel-
> simple's disable() function is called (see drm_panel_disable().
> So if you move the GPIO to the backlight and add a "disable" delay
> then you're all set.
> 
> Does that work for you? Does it make sense for this GPIO to be modeled
> as a backlight GPIO?

In combination with setting the "disable" delay this works *. Yet, it
feels wrong.

*: backlight-pwm only accepts one GPIO but that can be easily resolved.

It feels wrong that the backlight driver takes over part of the panel
control. On top, it still needs cooperation of the panel driver for the
proper timing. Lastly, it relies on the current behavior of drm-panel
that the panel driver is prepared/ enabled first and then the backlight
is switched on; and the other way around at power off.

We think that actually more panels in products are affected: We have
three panels from three different vendors (Sharp, Powertip, and Tianma)
and only one is visually affected. Yet, all of them specify a number of
vsyncs after de-asserting the enable GPIO.

Thank you for your reply,
Mark