Re: [PATCH] drm/i915: Don't enable backlight at setup time.

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 12 Jun 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> wrote:
> Maarten and Ville noticed that we are enabling backlight via DP aux very
> early in the modeset_init path via the intel_dp_aux_setup_backlight()
> function. Looks like all we need to do during _setup_backlight() is
> read the current brightness state instead of modifying it, so I don't
> why need to _enable_backlight() from _setup_backlight().

Please always use git blame to find the commit, and Cc the relevant
folks. Done now. It's e7156c833903 ("drm/i915: Add Backlight Control
using DPCD for eDP connectors (v9)").

The changelog says the call was intentionally moved there. ("v5: Moved
call to initialize backlight registers to dp_aux_setup_backlight"). I
have no recollection of why, and seems wrong, regardless of me signing
off on the patch.

We might be seeing some of the fallout also because of the recent
changes to the enable implementation by Puthikorn, though I agree the
whole call does seem wrong to begin with. Cc Puthikorn too.


Acked-by: Jani Nikula <jani.nikula@intel.com>


BR,
Jani.


>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 10 ----------
>  1 file changed, 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 6cc6298..228ca06 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -80,10 +80,6 @@ static uint32_t intel_dp_aux_get_backlight(struct intel_connector *connector)
>  static void
>  intel_dp_aux_set_backlight(const struct drm_connector_state *conn_state, u32 level)
>  {
> -	/*
> -	 * conn_state->best_encoder is likely NULL when called from
> -	 * intel_dp_aux_setup_backlight()
> -	 */
>  	struct intel_connector *connector = to_intel_connector(conn_state->connector);
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
>  	uint8_t vals[2] = { 0x0 };
> @@ -106,10 +102,6 @@ static void intel_dp_aux_enable_backlight(const struct intel_crtc_state *crtc_st
>  					  const struct drm_connector_state *conn_state)
>  {
>  	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> -	/*
> -	 * conn_state->best_encoder (and crtc_state) are NULL when called from
> -	 * intel_dp_aux_setup_backlight()
> -	 */
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
>  	uint8_t dpcd_buf = 0;
>  	uint8_t edp_backlight_mode = 0;
> @@ -156,8 +148,6 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector,
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
>  	struct intel_panel *panel = &connector->panel;
>  
> -	intel_dp_aux_enable_backlight(NULL, connector->base.state);
> -
>  	if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
>  		panel->backlight.max = 0xFFFF;
>  	else

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx