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

[Pandiyan, Dhinakaran]

On Tue, 2017-06-13 at 09:48 +0300, Jani Nikula wrote:
> On Mon, 12 Jun 2017, Dhinakaran Pandiyan <dhinakaran.pandiyan at 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)").

I'll amend the commit message to refer to the orginal patch and resend.

> 
> 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.

I looked at the conversation history, looks like it was intentionally
moved from _display_control_capable() to _setup_backlight() but not
clear why it was added to _display_control_capable() in the first place.

> 
> 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 at intel.com>
> 
> 
> BR,
> Jani.
> 
> 
> >
> > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Cc: Jani Nikula <jani.nikula at intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at 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
>