[Intel-gfx] [PATCH 2/2] drm/i915: Add backlight enable on/off delay for DP aux backlight control

[Jani Nikula]

On Thu, 13 Jun 2019, "Lee, Shawn C" <shawn.c.lee at intel.com> wrote:
> Follow generla eDP backlight enable control sequence. Add T8 (valid video
> data to backlight enable) delay before turn backlight_enable on.
> And T9 (backlight disable to end of valida video data) delay after
> backlight_enable off.

There are two things that are wrong here:

First, you're adding *two* waits on the DP AUX backlight enable and
disable paths. IIUC there is no reason to wait between setting the level
and enabling here. The waits have already been done on intel_dp.c level.

Second, the last_power_on, backlight_on_delay, last_backlight_off, and
backlight_off_delay members of intel_dp should all be private to
intel_dp.c. Indeed these waits even have wrappers *within* intel_dp.c so
that only very specific functions in intel_dp.c ever look at these
members. This is a huge red flag.

What's the problem you're trying to solve? Does this fix something
*actual* that you're seeing?

BR,
Jani.


>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Jani Nikula <jani.nikula at intel.com>
> Cc: Jose Roberto de Souza <jose.souza at intel.com>
> Cc: Cooper Chiou <cooper.chiou at intel.com>
>
> Signed-off-by: Lee, Shawn C <shawn.c.lee at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 98210ae17285..b008e887f4e9 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -217,12 +217,25 @@ static void intel_dp_aux_enable_backlight(const struct intel_crtc_state *crtc_st
>  	}
>  
>  	intel_dp_aux_set_backlight(conn_state, connector->panel.backlight.level);
> +
> +	wait_remaining_ms_from_jiffies(intel_dp->last_power_on,
> +				       intel_dp->backlight_on_delay);
> +
>  	set_aux_backlight_enable(intel_dp, true);
>  }
>  
>  static void intel_dp_aux_disable_backlight(const struct drm_connector_state *old_conn_state)
>  {
> +	struct intel_connector *connector = to_intel_connector(old_conn_state->connector);
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
> +
> +	intel_dp->last_backlight_off = jiffies;
> +
>  	set_aux_backlight_enable(enc_to_intel_dp(old_conn_state->best_encoder), false);
> +
> +	wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off,
> +				       intel_dp->backlight_off_delay);
> +
>  	intel_dp_aux_set_backlight(old_conn_state, 0);
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center