Re: [PATCH 4/6] drm/i915: Pass the current pipe from eDP init to backlight setup

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

On Fri, 07 Nov 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> On VLV/CHV both pipes A and B have their own backlight control
> registers. In order to correctly read out the current hardware state at
> init we need to know which pipe is driving the eDP port. Pass that
> information down from the eDP init code into the backlight code.
>
> To determine the correct pipe we first look at which pipe is currently
> configured in the port control register, if that look invalid we look
> at which pipe's PPS is currently controlling the port, and if that
> too looks invalid we just assume pipe A.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h    |  2 +-
>  drivers/gpu/drm/i915/intel_dp.c    | 22 +++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h   |  2 +-
>  drivers/gpu/drm/i915/intel_lvds.c  |  2 +-
>  drivers/gpu/drm/i915/intel_panel.c | 31 +++++++++++++++++--------------
>  5 files changed, 41 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0f00e58..65e4e29 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -510,7 +510,7 @@ struct drm_i915_display_funcs {
>  	/* display clock increase/decrease */
>  	/* pll clock increase/decrease */
>  
> -	int (*setup_backlight)(struct intel_connector *connector);
> +	int (*setup_backlight)(struct intel_connector *connector, enum pipe pipe);
>  	uint32_t (*get_backlight)(struct intel_connector *connector);
>  	void (*set_backlight)(struct intel_connector *connector,
>  			      uint32_t level);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index ceb528f..45b53ff 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5232,6 +5232,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	bool has_dpcd;
>  	struct drm_display_mode *scan;
>  	struct edid *edid;
> +	enum pipe pipe = INVALID_PIPE;
>  
>  	intel_dp->drrs_state.type = DRRS_NOT_SUPPORTED;
>  
> @@ -5300,11 +5301,30 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	if (IS_VALLEYVIEW(dev)) {
>  		intel_dp->edp_notifier.notifier_call = edp_notify_handler;
>  		register_reboot_notifier(&intel_dp->edp_notifier);
> +
> +		/*
> +		 * Figure out the current pipe for the initial backlight setup.
> +		 * If the current pipe isn't valid, try the PPS pipe, and if that
> +		 * fails just assume pipe A.
> +		 */
> +		if (IS_CHERRYVIEW(dev))
> +			pipe = DP_PORT_TO_PIPE_CHV(intel_dp->DP);
> +		else
> +			pipe = PORT_TO_PIPE(intel_dp->DP);
> +
> +		if (pipe != PIPE_A && pipe != PIPE_B)
> +			pipe = intel_dp->pps_pipe;
> +
> +		if (pipe != PIPE_A && pipe != PIPE_B)
> +			pipe = PIPE_A;
> +
> +		DRM_DEBUG_KMS("using pipe %c for initial backlight setup\n",
> +			      pipe_name(pipe));
>  	}
>  
>  	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
>  	intel_connector->panel.backlight_power = intel_edp_backlight_power;
> -	intel_panel_setup_backlight(connector);
> +	intel_panel_setup_backlight(connector, pipe);
>  
>  	return true;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index cb0e9db..0ff011e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1103,7 +1103,7 @@ void intel_gmch_panel_fitting(struct intel_crtc *crtc,
>  			      int fitting_mode);
>  void intel_panel_set_backlight_acpi(struct intel_connector *connector,
>  				    u32 level, u32 max);
> -int intel_panel_setup_backlight(struct drm_connector *connector);
> +int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe);
>  void intel_panel_enable_backlight(struct intel_connector *connector);
>  void intel_panel_disable_backlight(struct intel_connector *connector);
>  void intel_panel_destroy_backlight(struct drm_connector *connector);
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 2b50c98..c03d457 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -1116,7 +1116,7 @@ out:
>  	drm_connector_register(connector);
>  
>  	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
> -	intel_panel_setup_backlight(connector);
> +	intel_panel_setup_backlight(connector, INVALID_PIPE);
>  
>  	return;
>  
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 847d00f9..f6e8d23 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1117,7 +1117,7 @@ static u32 get_backlight_min_vbt(struct intel_connector *connector)
>  		     0, panel->backlight.max);
>  }
>  
> -static int bdw_setup_backlight(struct intel_connector *connector)
> +static int bdw_setup_backlight(struct intel_connector *connector, enum pipe unused)
>  {
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1143,7 +1143,7 @@ static int bdw_setup_backlight(struct intel_connector *connector)
>  	return 0;
>  }
>  
> -static int pch_setup_backlight(struct intel_connector *connector)
> +static int pch_setup_backlight(struct intel_connector *connector, enum pipe unused)
>  {
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1170,7 +1170,7 @@ static int pch_setup_backlight(struct intel_connector *connector)
>  	return 0;
>  }
>  
> -static int i9xx_setup_backlight(struct intel_connector *connector)
> +static int i9xx_setup_backlight(struct intel_connector *connector, enum pipe unused)
>  {
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1202,7 +1202,7 @@ static int i9xx_setup_backlight(struct intel_connector *connector)
>  	return 0;
>  }
>  
> -static int i965_setup_backlight(struct intel_connector *connector)
> +static int i965_setup_backlight(struct intel_connector *connector, enum pipe unused)
>  {
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1232,37 +1232,40 @@ static int i965_setup_backlight(struct intel_connector *connector)
>  	return 0;
>  }
>  
> -static int vlv_setup_backlight(struct intel_connector *connector)
> +static int vlv_setup_backlight(struct intel_connector *connector, enum pipe pipe)
>  {
>  	struct drm_device *dev = connector->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_panel *panel = &connector->panel;
> -	enum pipe pipe;
> +	enum pipe p;
>  	u32 ctl, ctl2, val;
>  
> -	for_each_pipe(dev_priv, pipe) {
> -		u32 cur_val = I915_READ(VLV_BLC_PWM_CTL(pipe));
> +	for_each_pipe(dev_priv, p) {
> +		u32 cur_val = I915_READ(VLV_BLC_PWM_CTL(p));
>  
>  		/* Skip if the modulation freq is already set */
>  		if (cur_val & ~BACKLIGHT_DUTY_CYCLE_MASK)
>  			continue;
>  
>  		cur_val &= BACKLIGHT_DUTY_CYCLE_MASK;
> -		I915_WRITE(VLV_BLC_PWM_CTL(pipe), (0xf42 << 16) |
> +		I915_WRITE(VLV_BLC_PWM_CTL(p), (0xf42 << 16) |
>  			   cur_val);
>  	}

I think we could try a follow-up patch doing the above only for the pipe
passed in. That's the one we're using, and the regs will be written in
backlight enable if the pipe changes.

Anyway, I like this approach. I had a patch trying to figure all this
out harder in the backlight code, but really anyone calling setup
backlight is in a much better position to do it.

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

>  
> -	ctl2 = I915_READ(VLV_BLC_PWM_CTL2(PIPE_A));
> +	if (WARN_ON(pipe != PIPE_A && pipe != PIPE_B))
> +		return -ENODEV;
> +
> +	ctl2 = I915_READ(VLV_BLC_PWM_CTL2(pipe));
>  	panel->backlight.active_low_pwm = ctl2 & BLM_POLARITY_I965;
>  
> -	ctl = I915_READ(VLV_BLC_PWM_CTL(PIPE_A));
> +	ctl = I915_READ(VLV_BLC_PWM_CTL(pipe));
>  	panel->backlight.max = ctl >> 16;
>  	if (!panel->backlight.max)
>  		return -ENODEV;
>  
>  	panel->backlight.min = get_backlight_min_vbt(connector);
>  
> -	val = _vlv_get_backlight(dev, PIPE_A);
> +	val = _vlv_get_backlight(dev, pipe);
>  	panel->backlight.level = intel_panel_compute_brightness(connector, val);
>  
>  	panel->backlight.enabled = (ctl2 & BLM_PWM_ENABLE) &&
> @@ -1271,7 +1274,7 @@ static int vlv_setup_backlight(struct intel_connector *connector)
>  	return 0;
>  }
>  
> -int intel_panel_setup_backlight(struct drm_connector *connector)
> +int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe)
>  {
>  	struct drm_device *dev = connector->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1290,7 +1293,7 @@ int intel_panel_setup_backlight(struct drm_connector *connector)
>  
>  	/* set level and max in panel struct */
>  	mutex_lock(&dev_priv->backlight_lock);
> -	ret = dev_priv->display.setup_backlight(intel_connector);
> +	ret = dev_priv->display.setup_backlight(intel_connector, pipe);
>  	mutex_unlock(&dev_priv->backlight_lock);
>  
>  	if (ret) {
> -- 
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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