Re: [PATCH v3 4/7] drm/i915: Clean up DP pipe select bits

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

On Fri, 18 May 2018, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Clean up the DP pipe select bits. To make the whole situation a bit
> less ugly we'll start to share the same code between .get_hw_state(),
> the port state asserts, and the VLV power sequencer code.
>
> v2: Return PIPE_A for cpt/ppt when the port isn't selected by
>     any transcoder. Returning INVALID_PIPE explodes *somewhere*
>     on some machines (can't immediately see where though). This
>     now matches the old behaviour.
> v3: Order the defines shift,mask,value (Jani)
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I could've bikeshedded about some naming here, but I don't want to read
this particular patch again, so

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

> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  24 +++-----
>  drivers/gpu/drm/i915/intel_display.c |  48 +++++----------
>  drivers/gpu/drm/i915/intel_dp.c      | 113 ++++++++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_drv.h     |   3 +
>  4 files changed, 92 insertions(+), 96 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5a103496423c..b888da96caf7 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5182,10 +5182,15 @@ enum {
>  #define CHV_DP_D		_MMIO(VLV_DISPLAY_BASE + 0x64300)
>  
>  #define   DP_PORT_EN			(1 << 31)
> -#define   DP_PIPEB_SELECT		(1 << 30)
> -#define   DP_PIPE_MASK			(1 << 30)
> -#define   DP_PIPE_SELECT_CHV(pipe)	((pipe) << 16)
> -#define   DP_PIPE_MASK_CHV		(3 << 16)
> +#define   DP_PIPE_SEL_SHIFT		30
> +#define   DP_PIPE_SEL_MASK		(1 << 30)
> +#define   DP_PIPE_SEL(pipe)		((pipe) << 30)
> +#define   DP_PIPE_SEL_SHIFT_IVB		29
> +#define   DP_PIPE_SEL_MASK_IVB		(3 << 29)
> +#define   DP_PIPE_SEL_IVB(pipe)		((pipe) << 29)
> +#define   DP_PIPE_SEL_SHIFT_CHV		16
> +#define   DP_PIPE_SEL_MASK_CHV		(3 << 16)
> +#define   DP_PIPE_SEL_CHV(pipe)		((pipe) << 16)
>  
>  /* Link training mode - select a suitable mode for each stage */
>  #define   DP_LINK_TRAIN_PAT_1		(0 << 28)
> @@ -7872,16 +7877,6 @@ enum {
>  #define PCH_DP_AUX_CH_DATA(aux_ch, i)	_MMIO(_PORT((aux_ch) - AUX_CH_B, _PCH_DPB_AUX_CH_DATA1, _PCH_DPC_AUX_CH_DATA1) + (i) * 4) /* 5 registers */
>  
>  /* CPT */
> -#define  PORT_TRANS_A_SEL_CPT	0
> -#define  PORT_TRANS_B_SEL_CPT	(1<<29)
> -#define  PORT_TRANS_C_SEL_CPT	(2<<29)
> -#define  PORT_TRANS_SEL_MASK	(3<<29)
> -#define  PORT_TRANS_SEL_CPT(pipe)	((pipe) << 29)
> -#define  PORT_TO_PIPE(val)	(((val) & (1<<30)) >> 30)
> -#define  PORT_TO_PIPE_CPT(val)	(((val) & PORT_TRANS_SEL_MASK) >> 29)
> -#define  SDVO_PORT_TO_PIPE_CHV(val)	(((val) & (3<<24)) >> 24)
> -#define  DP_PORT_TO_PIPE_CHV(val)	(((val) & (3<<16)) >> 16)
> -
>  #define _TRANS_DP_CTL_A		0xe0300
>  #define _TRANS_DP_CTL_B		0xe1300
>  #define _TRANS_DP_CTL_C		0xe2300
> @@ -7890,7 +7885,6 @@ enum {
>  #define  TRANS_DP_PORT_SEL_MASK		(3 << 29)
>  #define  TRANS_DP_PORT_SEL_NONE		(3 << 29)
>  #define  TRANS_DP_PORT_SEL(port)	(((port) - PORT_B) << 29)
> -#define  TRANS_DP_PIPE_TO_PORT(val)	((((val) & TRANS_DP_PORT_SEL_MASK) >> 29) + PORT_B)
>  #define  TRANS_DP_AUDIO_ONLY	(1<<26)
>  #define  TRANS_DP_ENH_FRAMING	(1<<18)
>  #define  TRANS_DP_8BPC		(0<<9)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d36dea568b82..d397febb5a7f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1303,38 +1303,22 @@ void assert_pch_transcoder_disabled(struct drm_i915_private *dev_priv,
>  	     pipe_name(pipe));
>  }
>  
> -static bool dp_pipe_enabled(struct drm_i915_private *dev_priv,
> -			    enum pipe pipe, u32 port_sel, u32 val)
> -{
> -	if ((val & DP_PORT_EN) == 0)
> -		return false;
> -
> -	if (HAS_PCH_CPT(dev_priv)) {
> -		u32 trans_dp_ctl = I915_READ(TRANS_DP_CTL(pipe));
> -		if ((trans_dp_ctl & TRANS_DP_PORT_SEL_MASK) != port_sel)
> -			return false;
> -	} else if (IS_CHERRYVIEW(dev_priv)) {
> -		if ((val & DP_PIPE_MASK_CHV) != DP_PIPE_SELECT_CHV(pipe))
> -			return false;
> -	} else {
> -		if ((val & DP_PIPE_MASK) != (pipe << 30))
> -			return false;
> -	}
> -	return true;
> -}
> -
>  static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv,
> -				   enum pipe pipe, i915_reg_t reg,
> -				   u32 port_sel)
> +				   enum pipe pipe, enum port port,
> +				   i915_reg_t dp_reg)
>  {
> -	u32 val = I915_READ(reg);
> -	I915_STATE_WARN(dp_pipe_enabled(dev_priv, pipe, port_sel, val),
> -	     "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n",
> -	     i915_mmio_reg_offset(reg), pipe_name(pipe));
> +	enum pipe port_pipe;
> +	bool state;
>  
> -	I915_STATE_WARN(HAS_PCH_IBX(dev_priv) && (val & DP_PORT_EN) == 0
> -	     && (val & DP_PIPEB_SELECT),
> -	     "IBX PCH dp port still using transcoder B\n");
> +	state = intel_dp_port_enabled(dev_priv, dp_reg, port, &port_pipe);
> +
> +	I915_STATE_WARN(state && port_pipe == pipe,
> +			"PCH DP %c enabled on transcoder %c, should be disabled\n",
> +			port_name(port), pipe_name(pipe));
> +
> +	I915_STATE_WARN(HAS_PCH_IBX(dev_priv) && !state && port_pipe == PIPE_B,
> +			"IBX PCH DP %c still using transcoder B\n",
> +			port_name(port));
>  }
>  
>  static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv,
> @@ -1360,9 +1344,9 @@ static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv,
>  {
>  	enum pipe port_pipe;
>  
> -	assert_pch_dp_disabled(dev_priv, pipe, PCH_DP_B, TRANS_DP_PORT_SEL(PORT_B));
> -	assert_pch_dp_disabled(dev_priv, pipe, PCH_DP_C, TRANS_DP_PORT_SEL(PORT_C));
> -	assert_pch_dp_disabled(dev_priv, pipe, PCH_DP_D, TRANS_DP_PORT_SEL(PORT_D));
> +	assert_pch_dp_disabled(dev_priv, pipe, PORT_B, PCH_DP_B);
> +	assert_pch_dp_disabled(dev_priv, pipe, PORT_C, PCH_DP_C);
> +	assert_pch_dp_disabled(dev_priv, pipe, PORT_D, PCH_DP_D);
>  
>  	I915_STATE_WARN(intel_crt_port_enabled(dev_priv, PCH_ADPA, &port_pipe) &&
>  			port_pipe == pipe,
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 102070940095..aab48283f155 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -529,9 +529,9 @@ vlv_power_sequencer_kick(struct intel_dp *intel_dp)
>  	DP |= DP_LINK_TRAIN_PAT_1;
>  
>  	if (IS_CHERRYVIEW(dev_priv))
> -		DP |= DP_PIPE_SELECT_CHV(pipe);
> -	else if (pipe == PIPE_B)
> -		DP |= DP_PIPEB_SELECT;
> +		DP |= DP_PIPE_SEL_CHV(pipe);
> +	else
> +		DP |= DP_PIPE_SEL(pipe);
>  
>  	pll_enabled = I915_READ(DPLL(pipe)) & DPLL_VCO_ENABLE;
>  
> @@ -1999,7 +1999,7 @@ static void intel_dp_prepare(struct intel_encoder *encoder,
>  		if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
>  			intel_dp->DP |= DP_ENHANCED_FRAMING;
>  
> -		intel_dp->DP |= crtc->pipe << 29;
> +		intel_dp->DP |= DP_PIPE_SEL_IVB(crtc->pipe);
>  	} else if (HAS_PCH_CPT(dev_priv) && port != PORT_A) {
>  		u32 trans_dp;
>  
> @@ -2025,9 +2025,9 @@ static void intel_dp_prepare(struct intel_encoder *encoder,
>  			intel_dp->DP |= DP_ENHANCED_FRAMING;
>  
>  		if (IS_CHERRYVIEW(dev_priv))
> -			intel_dp->DP |= DP_PIPE_SELECT_CHV(crtc->pipe);
> -		else if (crtc->pipe == PIPE_B)
> -			intel_dp->DP |= DP_PIPEB_SELECT;
> +			intel_dp->DP |= DP_PIPE_SEL_CHV(crtc->pipe);
> +		else
> +			intel_dp->DP |= DP_PIPE_SEL(crtc->pipe);
>  	}
>  }
>  
> @@ -2649,52 +2649,66 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>  			      mode == DRM_MODE_DPMS_ON ? "enable" : "disable");
>  }
>  
> +static bool cpt_dp_port_selected(struct drm_i915_private *dev_priv,
> +				 enum port port, enum pipe *pipe)
> +{
> +	enum pipe p;
> +
> +	for_each_pipe(dev_priv, p) {
> +		u32 val = I915_READ(TRANS_DP_CTL(p));
> +
> +		if ((val & TRANS_DP_PORT_SEL_MASK) == TRANS_DP_PORT_SEL(port)) {
> +			*pipe = p;
> +			return true;
> +		}
> +	}
> +
> +	DRM_DEBUG_KMS("No pipe for DP port %c found\n", port_name(port));
> +
> +	/* must initialize pipe to something for the asserts */
> +	*pipe = PIPE_A;
> +
> +	return false;
> +}
> +
> +bool intel_dp_port_enabled(struct drm_i915_private *dev_priv,
> +			   i915_reg_t dp_reg, enum port port,
> +			   enum pipe *pipe)
> +{
> +	bool ret;
> +	u32 val;
> +
> +	val = I915_READ(dp_reg);
> +
> +	ret = val & DP_PORT_EN;
> +
> +	/* asserts want to know the pipe even if the port is disabled */
> +	if (IS_IVYBRIDGE(dev_priv) && port == PORT_A)
> +		*pipe = (val & DP_PIPE_SEL_MASK_IVB) >> DP_PIPE_SEL_SHIFT_IVB;
> +	else if (HAS_PCH_CPT(dev_priv) && port != PORT_A)
> +		ret &= cpt_dp_port_selected(dev_priv, port, pipe);
> +	else if (IS_CHERRYVIEW(dev_priv))
> +		*pipe = (val & DP_PIPE_SEL_MASK_CHV) >> DP_PIPE_SEL_SHIFT_CHV;
> +	else
> +		*pipe = (val & DP_PIPE_SEL_MASK) >> DP_PIPE_SEL_SHIFT;
> +
> +	return ret;
> +}
> +
>  static bool intel_dp_get_hw_state(struct intel_encoder *encoder,
>  				  enum pipe *pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> -	enum port port = encoder->port;
> -	u32 tmp;
>  	bool ret;
>  
>  	if (!intel_display_power_get_if_enabled(dev_priv,
>  						encoder->power_domain))
>  		return false;
>  
> -	ret = false;
> +	ret = intel_dp_port_enabled(dev_priv, intel_dp->output_reg,
> +				    encoder->port, pipe);
>  
> -	tmp = I915_READ(intel_dp->output_reg);
> -
> -	if (!(tmp & DP_PORT_EN))
> -		goto out;
> -
> -	if (IS_IVYBRIDGE(dev_priv) && port == PORT_A) {
> -		*pipe = PORT_TO_PIPE_CPT(tmp);
> -	} else if (HAS_PCH_CPT(dev_priv) && port != PORT_A) {
> -		enum pipe p;
> -
> -		for_each_pipe(dev_priv, p) {
> -			u32 trans_dp = I915_READ(TRANS_DP_CTL(p));
> -			if (TRANS_DP_PIPE_TO_PORT(trans_dp) == port) {
> -				*pipe = p;
> -				ret = true;
> -
> -				goto out;
> -			}
> -		}
> -
> -		DRM_DEBUG_KMS("No pipe for dp port 0x%x found\n",
> -			      i915_mmio_reg_offset(intel_dp->output_reg));
> -	} else if (IS_CHERRYVIEW(dev_priv)) {
> -		*pipe = DP_PORT_TO_PIPE_CHV(tmp);
> -	} else {
> -		*pipe = PORT_TO_PIPE(tmp);
> -	}
> -
> -	ret = true;
> -
> -out:
>  	intel_display_power_put(dev_priv, encoder->power_domain);
>  
>  	return ret;
> @@ -3684,8 +3698,9 @@ intel_dp_link_down(struct intel_encoder *encoder,
>  		intel_set_pch_fifo_underrun_reporting(dev_priv, PIPE_A, false);
>  
>  		/* always enable with pattern 1 (as per spec) */
> -		DP &= ~(DP_PIPEB_SELECT | DP_LINK_TRAIN_MASK);
> -		DP |= DP_PORT_EN | DP_LINK_TRAIN_PAT_1;
> +		DP &= ~(DP_PIPE_SEL_MASK | DP_LINK_TRAIN_MASK);
> +		DP |= DP_PORT_EN | DP_PIPE_SEL(PIPE_A) |
> +			DP_LINK_TRAIN_PAT_1;
>  		I915_WRITE(intel_dp->output_reg, DP);
>  		POSTING_READ(intel_dp->output_reg);
>  
> @@ -5320,14 +5335,14 @@ static void intel_edp_panel_vdd_sanitize(struct intel_dp *intel_dp)
>  static enum pipe vlv_active_pipe(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> +	enum pipe pipe;
>  
> -	if ((intel_dp->DP & DP_PORT_EN) == 0)
> -		return INVALID_PIPE;
> +	if (intel_dp_port_enabled(dev_priv, intel_dp->output_reg,
> +				  encoder->port, &pipe))
> +		return pipe;
>  
> -	if (IS_CHERRYVIEW(dev_priv))
> -		return DP_PORT_TO_PIPE_CHV(intel_dp->DP);
> -	else
> -		return PORT_TO_PIPE(intel_dp->DP);
> +	return INVALID_PIPE;
>  }
>  
>  void intel_dp_encoder_reset(struct drm_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a827a2245c18..dc7f81e1e20a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1647,6 +1647,9 @@ void intel_csr_ucode_suspend(struct drm_i915_private *);
>  void intel_csr_ucode_resume(struct drm_i915_private *);
>  
>  /* intel_dp.c */
> +bool intel_dp_port_enabled(struct drm_i915_private *dev_priv,
> +			   i915_reg_t dp_reg, enum port port,
> +			   enum pipe *pipe);
>  bool intel_dp_init(struct drm_i915_private *dev_priv, i915_reg_t output_reg,
>  		   enum port port);
>  bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,

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