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

[Sam Ravnborg]

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