Re: [PATCH v2 1/3] drm: panel: simple: Allow timing constraints, not fixed delays

2020-11-05 Thread Doug Anderson
Hi,

On Wed, Oct 28, 2020 at 3:58 PM Douglas Anderson  wrote:
> @@ -249,18 +327,19 @@ static int panel_simple_unprepare(struct drm_panel 
> *panel)
>  {
> struct panel_simple *p = to_panel_simple(panel);
>
> -   if (!p->prepared)
> +   if (p->prepared_time != 0)

Though I swear I tested this, clearly I didn't test well enough.  I
was poking with this patch series some more and realized that I got
the above (and the one in _prepare) totally and utterly backwards.
I'll send out a v3 now...

-Doug
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH v2 1/3] drm: panel: simple: Allow timing constraints, not fixed delays

2020-10-28 Thread Douglas Anderson
The simple panel code currently allows panels to define fixed delays
at certain stages of initialization.  These work OK, but they don't
really map all that clearly to the requirements presented in many
panel datasheets.  Instead of defining a fixed delay, those datasheets
provide a timing diagram and specify a minimum amount of time that
needs to pass from event A to event B.

Because of the way things are currently defined, most panels end up
over-delaying.  One prime example here is that a number of panels I've
looked at define the amount of time that must pass between turning a
panel off and turning it back on again.  Since there is no way to
specify this, many developers have listed this as the "unprepare"
delay.  However, if nobody ever tried to turn the panel on again in
the next 500 ms (or whatever the delay was) then this delay was
pointless.  It's better to do the delay only in the case that someone
tried to turn the panel on too quickly.

Let's support specifying delays as constraints.  We'll start with the
one above and also a second one: the minimum time between prepare
being done and doing the enable.  On the panel I'm looking at, there's
an 80 ms minimum time between HPD being asserted by the panel and
setting the backlight enable GPIO.  By specifying as a constraint we
can enforce this without over-delaying.  Specifically the link
training is allowed to happen in parallel with this delay so adding a
fixed 80 ms delay isn't ideal.

Signed-off-by: Douglas Anderson 
---

Changes in v2:
- Inline the kernel doc for the two new members.
- Beefed up kernel doc saying exactly when the delay happens.
- Removed "_ms" from the end of members to shorten them.
- Renamed "timing_constraints" to "min_times" to shorten it.
- Renamed "enforce_constraint()" to "wait_min_time()" to shorten it.
- Check "prepared_time" against 0 to see if we've been prepared.

 drivers/gpu/drm/panel/panel-simple.c | 99 ++--
 1 file changed, 92 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c 
b/drivers/gpu/drm/panel/panel-simple.c
index 2be358fb46f7..2613b9434548 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -92,6 +92,68 @@ struct panel_desc {
unsigned int unprepare;
} delay;
 
+   struct {
+   /**
+* @prepare_to_enable: Time between prepare and enable.
+*
+* The minimum time, in milliseconds, that needs to have passed
+* between when prepare finished and enable may begin. If at
+* enable time less time has passed since prepare finished,
+* the driver waits for the remaining time.
+*
+* If a fixed enable delay is also specified, we'll start
+* counting before delaying for the fixed delay.
+*
+* If a fixed prepare delay is also specified, we won't start
+* counting until after the fixed delay. We can't overlap this
+* fixed delay with the min time because the fixed delay
+* doesn't happen at the end of the function if a HPD GPIO was
+* specified.
+*
+* In other words:
+*   prepare()
+* ...
+* // do fixed prepare delay
+* // wait for HPD GPIO if applicable
+* // start counting for prepare_to_enable
+*
+*   enable()
+* // do fixed enable delay
+* // enforce prepare_to_enable min time
+*/
+   unsigned int prepare_to_enable;
+
+   /**
+* @unprepare_to_prepare: Time between unprepare and prepare.
+*
+* The minimum time, in milliseconds, that needs to have passed
+* between when unprepare finished and prepare may begin. If at
+* prepare time less time has passed since unprepare finished,
+* the driver waits for the remaining time.
+*
+* If a fixed unprepare delay is also specified, we'll start
+* counting before delaying for the fixed delay.
+*
+* If a fixed prepare delay is also specified, it will happen
+* separately and after we've enforced this minimum. We can't
+* overlap this fixed delay with the min time because the
+* fixed delay doesn't happen at the start of the function
+* if a regulator or enable GPIO was specified.
+*
+* In other words:
+*   unprepare():
+* ...
+* // start counting for unprepare_to_prepare
+* // do fixed unprepare delay
+*
+