Hi Douglas.
On Thu, Nov 05, 2020 at 01:57:39PM -0800, Douglas Anderson wrote:
> 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.
Nice and detaild explanation - thanks.
Reading through this a few times it seems that a simpler approach would
be to change the semantics of the dealys we already have a little and
only add one new field:
struct {
unsigned int prepare;
unsigned int hpd_absent_delay;
unsigned int prepare_to_enable;
unsigned int enable;
unsigned int disable;
unsigned int unprepare;
} delay;
The lines marked "//*" are new or changed:
prepare()
//*enforce unprepare time
// enable regulator
// set enable gpio
// do fixed prepare delay (this is time until we can trust hpd)
// wait for HPD GPIO if applicable, otherwise do fixed hpd_absent_delay
//*start counting for prepare_to_enable
enable()
//*enforce prepare_to_enable min time
// enable backlight if applicable
panel shows nice pictures of kitties
disable()
// disable backlight is applicable
// fixed disable delay
unprepare()
// unset enable gpio
// disable regulator
//*start counting for unprepare delay
This should not break any exisitng panels - and we avoid that we have
two delays that are almost the same.
Would that work for you?
Note that no new struct was introduced - this is all dealys so keep it in
the same struct.
A dew details in the following.
Sam
>
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
>
> Changes in v3:
> - Fixed totally backwards "if" tests. :(
>
> Changes in v2:
> - Inline the kernel doc for the two new members.
A nice follow-up patch would be to move the remaining fields as inline
doc. But let us settle on this first.
> struct panel_simple {
> struct drm_panel base;
> - bool prepared;
> bool enabled;
> bool no_hpd;
>
> + ktime_t prepared_time;
> + ktime_t unprepared_time;
> +
> const struct panel_desc *desc;
>
> struct regulator *supply;
> @@ -230,6 +294,20 @@ static int panel_simple_get_non_edid_modes(struct panel_simple *panel,
> return num;
> }
>
> +static void panel_simple_wait_min_time(ktime_t start_ktime, unsigned int min_ms)
> +{
> + ktime_t now_ktime, min_ktime;
> +
> + if (!min_ms)
> + return;
> +
> + min_ktime = ktime_add(start_ktime, ms_to_ktime(min_ms));
> + now_ktime = ktime_get();
> +
> + if (ktime_before(now_ktime, min_ktime))
> + msleep(ktime_to_ms(ktime_sub(min_ktime, now_ktime)) + 1);
> +}
panel_simple_wait()?
> +
> static int panel_simple_disable(struct drm_panel *panel)
> {
> struct panel_simple *p = to_panel_simple(panel);
> @@ -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)
> return 0;
>
> gpiod_set_value_cansleep(p->enable_gpio, 0);
>
> regulator_disable(p->supply);
>
> + p->prepared_time = 0;
> + p->unprepared_time = ktime_get();
> +
> if (p->desc->delay.unprepare)
> msleep(p->desc->delay.unprepare);
>
> - p->prepared = false;
> -
> return 0;
> }
>
> @@ -296,9 +375,12 @@ static int panel_simple_prepare(struct drm_panel *panel)
> int err;
> int hpd_asserted;
>
> - if (p->prepared)
> + if (p->prepared_time != 0)
> return 0;
>
> + panel_simple_wait_min_time(p->unprepared_time,
> + p->desc->min_times.unprepare_to_prepare);
> +
> err = regulator_enable(p->supply);
> if (err < 0) {
> dev_err(panel->dev, "failed to enable supply: %d\n", err);
> @@ -333,7 +415,7 @@ static int panel_simple_prepare(struct drm_panel *panel)
> }
> }
>
> - p->prepared = true;
> + p->prepared_time = ktime_get();
>
> return 0;
> }
> @@ -348,6 +430,9 @@ static int panel_simple_enable(struct drm_panel *panel)
> if (p->desc->delay.enable)
> msleep(p->desc->delay.enable);
>
> + panel_simple_wait_min_time(p->prepared_time,
> + p->desc->min_times.prepare_to_enable);
> +
> p->enabled = true;
>
> return 0;
> @@ -514,7 +599,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
> return -ENOMEM;
>
> panel->enabled = false;
> - panel->prepared = false;
> + panel->prepared_time = 0;
> panel->desc = desc;
>
> panel->no_hpd = of_property_read_bool(dev->of_node, "no-hpd");
> --
> 2.29.1.341.ge80a0c044ae-goog
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
