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

From: Imre Deak <imre.deak@intel.com>
To: Ander Conselvan De Oliveira <conselvan2@gmail.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 4/6] drm/i915/bxt: Set DDI PHY lane latency optimization during modeset
Date: Wed, 08 Jun 2016 11:54:53 +0300	[thread overview]
Message-ID: <1465376093.12702.7.camel@intel.com> (raw)
In-Reply-To: <1465368115.2909.16.camel@gmail.com>

On ke, 2016-06-08 at 09:41 +0300, Ander Conselvan De Oliveira wrote:
> 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.

Yep, this is a typo left-over before I realized that the two things are
different:) Thanks for catching it.

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

Ok, will simplify it.

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

It's not a PLL specific thing though, rather a property of the PHY
channel. For instance we could share one PLL for two channels in which 
case we'd need to enable only one PLL but configure the lanes for both
channels.

--Imre

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