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

From: Jani Nikula <jani.nikula@intel.com>
To: "Lee, Shawn C" <shawn.c.lee@intel.com>, intel-gfx@lists.freedesktop.org
Cc: Cooper Chiou <cooper.chiou@intel.com>, Lee@freedesktop.org
Subject: Re: [PATCH 2/2] drm/i915: Add backlight enable on/off delay for DP aux backlight control
Date: Fri, 14 Jun 2019 11:09:06 +0300	[thread overview]
Message-ID: <87blz0wpul.fsf@intel.com> (raw)
In-Reply-To: <1560492565-30405-2-git-send-email-shawn.c.lee@intel.com>

On Thu, 13 Jun 2019, "Lee, Shawn C" <shawn.c.lee@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@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Jose Roberto de Souza <jose.souza@intel.com>
> Cc: Cooper Chiou <cooper.chiou@intel.com>
>
> Signed-off-by: Lee, Shawn C <shawn.c.lee@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx