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@chromium.org
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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel