Re: [PATCH 4/6] drm/i915/bxt: Set DDI PHY lane latency optimization during modeset

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Tue, 2016-06-07 at 21:24 +0300, Imre Deak wrote:
> So far we configured a static lane latency optimization during driver
> loading/resuming. The specification changed at one point and now this
> configuration depends on the lane count, so move the configuration
> to modeset time accordingly.
> 
> It's not clear when this lane configuration takes effect. The
> specification only requires that the programming is done before enabling
> the port. On CHV OTOH the lanes start to power up already right after
> enabling the PLL. To be safe preserve the current order and set things
> up already before enabling the PLL.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95476
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     | 131 +++++++++++++++++++++++-----------
> -
>  drivers/gpu/drm/i915/intel_display.c |   5 ++
>  drivers/gpu/drm/i915/intel_drv.h     |   6 ++
>  3 files changed, 99 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index dee6dd0..f46117a 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1789,8 +1789,7 @@ static void broxton_phy_wait_grc_done(struct
> drm_i915_private *dev_priv,
>  
>  void bxt_ddi_phy_init(struct drm_i915_private *dev_priv, enum dpio_phy phy)
>  {
> -	enum port port;
> -	u32 ports, val;
> +	u32 val;
>  
>  	if (bxt_ddi_phy_is_enabled(dev_priv, phy)) {
>  		/* Still read out the GRC value for state verification */
> @@ -1825,28 +1824,6 @@ void bxt_ddi_phy_init(struct drm_i915_private
> *dev_priv, enum dpio_phy phy)
>  		DRM_ERROR("timeout during PHY%d power on\n", phy);
>  	}
>  
> -	if (phy == DPIO_PHY0)
> -		ports = BIT(PORT_B) | BIT(PORT_C);
> -	else
> -		ports = BIT(PORT_A);
> -
> -	for_each_port_masked(port, ports) {
> -		int lane;
> -
> -		for (lane = 0; lane < 4; lane++) {
> -			val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane));
> -			/*
> -			 * Note that on CHV this flag is called UPAR, but has
> -			 * the same function.
> -			 */
> -			val &= ~LATENCY_OPTIM;
> -			if (lane != 1)
> -				val |= LATENCY_OPTIM;
> -
> -			I915_WRITE(BXT_PORT_TX_DW14_LN(port, lane), val);
> -		}
> -	}
> -
>  	/* Program PLL Rcomp code offset */
>  	val = I915_READ(BXT_PORT_CL1CM_DW9(phy));
>  	val &= ~IREF0RC_OFFSET_MASK;
> @@ -1956,8 +1933,6 @@ __phy_reg_verify_state(struct drm_i915_private
> *dev_priv, enum dpio_phy phy,
>  bool bxt_ddi_phy_verify_state(struct drm_i915_private *dev_priv,
>  			      enum dpio_phy phy)
>  {
> -	enum port port;
> -	u32 ports;
>  	uint32_t mask;
>  	bool ok;
>  
> @@ -1970,21 +1945,6 @@ bool bxt_ddi_phy_verify_state(struct drm_i915_private
> *dev_priv,
>  
>  	ok = true;
>  
> -	if (phy == DPIO_PHY0)
> -		ports = BIT(PORT_B) | BIT(PORT_C);
> -	else
> -		ports = BIT(PORT_A);
> -
> -	for_each_port_masked(port, ports) {
> -		int lane;
> -
> -		for (lane = 0; lane < 4; lane++)
> -			ok &= _CHK(BXT_PORT_TX_DW14_LN(port, lane),
> -				    LATENCY_OPTIM,
> -				    lane != 1 ? LATENCY_OPTIM : 0,
> -				    "BXT_PORT_TX_DW14_LN(%d, %d)", port,
> lane);
> -	}
> -
>  	/* PLL Rcomp code offset */
>  	ok &= _CHK(BXT_PORT_CL1CM_DW9(phy),
>  		    IREF0RC_OFFSET_MASK, 0xe4 << IREF0RC_OFFSET_SHIFT,
> @@ -2028,6 +1988,75 @@ bool bxt_ddi_phy_verify_state(struct drm_i915_private
> *dev_priv,
>  #undef _CHK
>  }
>  
> +static uint8_t
> +bxt_ddi_phy_calc_lane_lat_optim_mask(struct intel_encoder *encoder,
> +				     struct intel_crtc_state *pipe_config)
> +{
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	int optim_lane_count;
> +
> +	switch (pipe_config->lane_count) {
> +	case 1:
> +		optim_lane_count = 0;
> +		break;
> +	case 2:
> +		optim_lane_count = 3;
> +		break;
> +	case 4:
> +		optim_lane_count = 4;
> +		break;
> +	default:
> +		MISSING_CASE(intel_crtc->config->lane_count);

Since this is called from compute config, pipe_config and intel_crtc->config
will be different. But anyway, I think this belongs in the dpll code. See below.

> +
> +		return 0;
> +	}
> +
> +	return (BIT(optim_lane_count) - 1) & ~BIT(1);

Wouldn't it be easier to just return the right mask from the switch? There's
already a case for each possible lane count.

> +}
> +
> +static void bxt_ddi_pre_pll_enable(struct intel_encoder *encoder)
> +{
> +	struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);
> +	struct drm_i915_private *dev_priv = to_i915(dport->base.base.dev);
> +	enum port port = dport->port;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	int lane;
> +
> +	for (lane = 0; lane < 4; lane++) {
> +		u32 val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane));
> +
> +		/*
> +		 * Note that on CHV this flag is called UPAR, but has
> +		 * the same function.
> +		 */
> +		val &= ~LATENCY_OPTIM;
> +		if (intel_crtc->config->lane_lat_optim_mask & BIT(lane))
> +			val |= LATENCY_OPTIM;
> +
> +		I915_WRITE(BXT_PORT_TX_DW14_LN(port, lane), val);
> +	}
> +}
> +
> +static uint8_t
> +bxt_ddi_phy_get_lane_lat_optim_mask(struct intel_encoder *encoder)
> +{
> +	struct intel_digital_port *dport = enc_to_dig_port(&encoder->base);
> +	struct drm_i915_private *dev_priv = to_i915(dport->base.base.dev);
> +	enum port port = dport->port;
> +	int lane;
> +	uint8_t mask;
> +
> +	mask = 0;
> +	for (lane = 0; lane < 4; lane++) {
> +		u32 val = I915_READ(BXT_PORT_TX_DW14_LN(port, lane));
> +
> +		if (val & LATENCY_OPTIM)
> +			mask |= BIT(lane);
> +	}
> +
> +	return mask;
> +}
> +
>  void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
>  {
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> @@ -2199,13 +2228,19 @@ void intel_ddi_get_config(struct intel_encoder
> *encoder,
>  	}
>  
>  	intel_ddi_clock_get(encoder, pipe_config);
> +
> +	if (IS_BROXTON(dev_priv))
> +		pipe_config->lane_lat_optim_mask =
> +			bxt_ddi_phy_get_lane_lat_optim_mask(encoder);
>  }
>  
>  static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>  				     struct intel_crtc_state *pipe_config)
>  {
> +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
>  	int type = encoder->type;
>  	int port = intel_ddi_get_encoder_port(encoder);
> +	int ret;
>  
>  	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown
> output!\n");
>  
> @@ -2213,9 +2248,17 @@ static bool intel_ddi_compute_config(struct
> intel_encoder *encoder,
>  		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>  
>  	if (type == INTEL_OUTPUT_HDMI)
> -		return intel_hdmi_compute_config(encoder, pipe_config);
> +		ret = intel_hdmi_compute_config(encoder, pipe_config);
>  	else
> -		return intel_dp_compute_config(encoder, pipe_config);
> +		ret = intel_dp_compute_config(encoder, pipe_config);
> +
> +	if (IS_BROXTON(dev_priv) && ret)
> +		pipe_config->lane_lat_optim_mask =
> +			bxt_ddi_phy_calc_lane_lat_optim_mask(encoder,
> +							     pipe_config);
> +
> +	return ret;
> +
>  }
>  
>  static const struct drm_encoder_funcs intel_ddi_funcs = {
> @@ -2314,6 +2357,8 @@ void intel_ddi_init(struct drm_device *dev, enum port
> port)
>  
>  	intel_encoder->compute_config = intel_ddi_compute_config;
>  	intel_encoder->enable = intel_enable_ddi;
> +	if (IS_BROXTON(dev_priv))
> +		intel_encoder->pre_pll_enable = bxt_ddi_pre_pll_enable;
>  	intel_encoder->pre_enable = intel_ddi_pre_enable;
>  	intel_encoder->disable = intel_disable_ddi;
>  	intel_encoder->post_disable = intel_ddi_post_disable;
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 60cba19..4bc7c48 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4841,6 +4841,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
>  						      false);
>  
> +	for_each_encoder_on_crtc(dev, crtc, encoder)
> +		if (encoder->pre_pll_enable)
> +			encoder->pre_pll_enable(encoder);
> +
>  	if (intel_crtc->config->shared_dpll)
>  		intel_enable_shared_dpll(intel_crtc);

Effectively this writes BXT_PORT_TX_DW14_LN before bxt_ddi_pll_enable() is
called. If that would be the first register write in that function, it would
avoid the extra detour. So we can calculate the optimization in bxt_get_dpll(),
store it in struct intel_dpll_hw_state and use the pll state checking to verify
the right values were programmed. That way there's no need to add a value
specific to the bxt phy to struct intel_crtc_state or implement the
pre_pll_enable hook.

Ander

>  
> @@ -12816,6 +12820,7 @@ intel_pipe_config_compare(struct drm_device *dev,
>  
>  	PIPE_CONF_CHECK_I(has_dp_encoder);
>  	PIPE_CONF_CHECK_I(lane_count);
> +	PIPE_CONF_CHECK_X(lane_lat_optim_mask);
>  
>  	if (INTEL_INFO(dev)->gen < 8) {
>  		PIPE_CONF_CHECK_M_N(dp_m_n);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 17445d7..ca98416 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -579,6 +579,12 @@ struct intel_crtc_state {
>  
>  	uint8_t lane_count;
>  
> +	/*
> +	 * Used by platforms having DP/HDMI PHY with programmable lane
> +	 * latency optimization.
> +	 */
> +	uint8_t lane_lat_optim_mask;
> +
>  	/* Panel fitter controls for gen2-gen4 + VLV */
>  	struct {
>  		u32 control;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx