AW: drm/panel: panel-simple power-off sequencing

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

Hi Doug,

> > 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.
> 
> Circling back, I'm curious what ended up happening here. Did you
> decide that it actually was a backlight enable GPIO, or are you
> planning on sending a patch?

Thank you for coming back to this topic.

We agree that linking the enable signal of "old school" parallel panels with 
the enable of the backlight would work. We tested it and it works with our 
panels.

Yet, we are not convinced that the backlight is part of the panel. And thus, 
architecturally, this should be kept separate. For us, a display is the 
combination of a backlight and a panel.

For adding support of the kind of enable signal required for our "old school" 
parallel panels into panel-simple we see the following possibilities:

- Add another GPIO with a tight coupling to the prepare and unprepare state.
- Add a flag which signals to prepare/ unprepare to handle the existing GPIO 
synchronously.

But we are not convinced that this is in the spirit of a panel *simple*. 
Short-term we will now create our own panel driver which will support our 
parallel panels. Mid-term we would like to revisit the topic and hopefully come 
up with a patch for panel-simple.

Cheers,
Mark

Re: drm/panel: panel-simple power-off sequencing

[Doug Anderson]

Hi,

On Fri, Oct 27, 2023 at 7:31 AM Doug Anderson  wrote:
>
> 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.

Circling back, I'm curious what ended up happening here. Did you
decide that it actually was a backlight enable GPIO, or are you
planning on sending a patch?

-Doug

Re: 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)
 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

Re: 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)
>  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

Re: drm/panel: panel-simple power-off sequencing

[Doug Anderson]

Hi,

On Thu, Oct 26, 2023 at 7:37 AM Jonas Mark (BT-FS/ENG1-GRB)
 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.

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?

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?


-Doug

drm/panel: panel-simple power-off sequencing

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

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?

Mit freundlichen Grüßen / Best regards

Mark Jonas

Building Technologies, Panel Software Fire (BT-FIR/ENG1-Grb)
Bosch Sicherheitssysteme GmbH | Postfach 11 11 | 85626 Grasbrunn | GERMANY | 
www.boschsecurity.com
Tel. +49 89 6290-1233 | Telefax +49 89 6290-281233 | mark.jo...@de.bosch.com

Sitz: Stuttgart, Registergericht: Amtsgericht Stuttgart HRB 23118
Aufsichtsratsvorsitzender: Christian Fischer; Geschäftsführung: Thomas Quante, 
Peter Löffler, Henrik Siegle