drm/panel: panel-simple power-off sequencing

[Doug Anderson]

Hi,

On Fri, Oct 27, 2023 at 5:30 AM Jonas Mark (BT-FS/ENG1-GRB)
<Mark.Jonas at de.bosch.com> wrote:
>
> > 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.

I would first ask you to take another look before saying that it's
wrong to put the enable pin in the backlight driver. At least on most
displays I've seen (though I've spent most time looking at eDP), the
backlight and panel are really not separable entities.

Take a look at the ASCII art timing diagram in
Documentation/devicetree/bindings/display/panel/panel-edp.yaml, for
instance. This is typical of eDP panel diagrams and it includes _both_
the backlight and the display timings. In general, it's always made
sense for me to think of the "LED_EN" line in that diagram as the
backlight enable.


> 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.

That current behavior is not random and I don't think it would be
possible for it to change. Too many things rely on the current order.


> 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.

If you're certain that your enable GPIO really shouldn't be modeled
with the backlight, then IMO you should submit a patch to panel-simple
that allows an "enable" GPIO to be controlled in the "enable"
function. We'd have to have a discussion about how to best do this.
The fact that the existing "enable" GPIO is considered a "power
enable" predates my involvement in the driver. In other words I think
it's always been in the "prepare/unprepare" functions. It always felt
wrong to me, too. ;-)

I guess the "easiest" (though a bit ugly) solution is to either add a
per-panel boolean flag that says whether that panel wants the enable
GPIO controlled in enable/disable or prepare/enable.

Another solution might be to introduce a 2nd GPIO, though you'd have
to think about what to call it since the existing one is kinda stuck
as "enable" given the DT bindings.

I guess a 3rd solution would be to audit users and see if anyone
actually needs the current "enable" GPIO as it is and whether those
cases would be better modeled as a GPIO-controlled regulator. Of
course, if you have to change how boards model this, then you start
getting into the argument about DT backward compatibility.


I guess, in summary, I'm hoping you'll look again and find that this
really is a backlight enable. If not, I'd probably advocate for a
per-panel boolean.


FWIW, looking a bit at the history and going back to 2014 in commit
f673c37ec453 ("drm/panel: simple: Support delays in panel functions"):

* The backlight calls used to be made directly from panel-simple

* The "unprepare" delay was documented as "the time (in milliseconds)
that it takes for the panel to power itself down completely" and makes
me believe that, even originally, it was about not turning the panel
back on before it fully turns off (T12 in the eDP timing diagram I
pointed at earlier).

* The "enable" GPIO has been controlled from prepare/unprepare since
those functions were introduced in commit 613a633e7a56 ("drm/panel:
simple: Add proper definition for prepare and unprepare"). It kinda
feels like the problem originated here...


-Doug