Re: [PATCH 57/66] drm/i915: State readout and cross-checking for ddi_pll_sel

[Damien Lespiau <damien.lespiau@intel.com>]

On Thu, Apr 24, 2014 at 11:55:33PM +0200, Daniel Vetter wrote:
> To make things a bit more manageable extract a new function for
> reading out common ddi port state. This means a bit of duplication
> between encoders and the core since both look at the same registers,
> but doesn't seem worth to make a fuzz about.
> 
> We can also remove the state readout code in intel_ddi_setup_hw_pll_state.
> That code is only called from the hardware take over and not the cross
> check code, and only after the crtc state is reconstructed. So we can
> rely on an accurate value of crtc->config.ddi_pll_sel already.
> 
> Compared to the old code also trust the hw state more and don't
> special-case port A - we want to cross-check the actual-state, not
> bake in our own assumptions about how this is supposed to all be
> linked up.
> 
> v2: Make use of the read-out ddi_pll_sel in intel_ddi_clock_get.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Moving things out of intel_ddi.c to intel_display.c seems to be the
opposite of what we want. I don't see why we couldn't leave the DDI
readout function inside intel_ddi.c?

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  1 +
>  drivers/gpu/drm/i915/intel_ddi.c     | 40 +-----------------------------
>  drivers/gpu/drm/i915/intel_display.c | 48 ++++++++++++++++++++++++------------
>  3 files changed, 34 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 64d40f22e708..4c1cefb5f3eb 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5366,6 +5366,7 @@ enum punit_power_well {
>  #define  TRANS_DDI_FUNC_ENABLE		(1<<31)
>  /* Those bits are ignored by pipe EDP since it can only connect to DDI A */
>  #define  TRANS_DDI_PORT_MASK		(7<<28)
> +#define  TRANS_DDI_PORT_SHIFT		28
>  #define  TRANS_DDI_SELECT_PORT(x)	((x)<<28)
>  #define  TRANS_DDI_PORT_NONE		(0<<28)
>  #define  TRANS_DDI_MODE_SELECT_MASK	(7<<24)
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 2adcc917806e..571cfe431558 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -481,11 +481,10 @@ static void intel_ddi_clock_get(struct intel_encoder *encoder,
>  				struct intel_crtc_config *pipe_config)
>  {
>  	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> -	enum port port = intel_ddi_get_encoder_port(encoder);
>  	int link_clock = 0;
>  	u32 val, pll;
>  
> -	val = I915_READ(PORT_CLK_SEL(port));
> +	val = pipe_config->ddi_pll_sel;
>  	switch (val & PORT_CLK_SEL_MASK) {
>  	case PORT_CLK_SEL_LCPLL_810:
>  		link_clock = 81000;
> @@ -987,40 +986,6 @@ static bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
>  	return intel_ddi_get_port_state(encoder, pipe, port);
>  }
>  
> -static uint32_t intel_ddi_get_crtc_pll(struct drm_i915_private *dev_priv,
> -				       enum pipe pipe)
> -{
> -	uint32_t temp, ret;
> -	enum port port = I915_MAX_PORTS;
> -	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
> -								      pipe);
> -	int i;
> -
> -	if (cpu_transcoder == TRANSCODER_EDP) {
> -		port = PORT_A;
> -	} else {
> -		temp = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
> -		temp &= TRANS_DDI_PORT_MASK;
> -
> -		for (i = PORT_B; i <= PORT_E; i++)
> -			if (temp == TRANS_DDI_SELECT_PORT(i))
> -				port = i;
> -	}
> -
> -	if (port == I915_MAX_PORTS) {
> -		WARN(1, "Pipe %c enabled on an unknown port\n",
> -		     pipe_name(pipe));
> -		ret = PORT_CLK_SEL_NONE;
> -	} else {
> -		ret = I915_READ(PORT_CLK_SEL(port));
> -		DRM_DEBUG_KMS("Pipe %c connected to port %c using clock "
> -			      "0x%08x\n", pipe_name(pipe), port_name(port),
> -			      ret);
> -	}
> -
> -	return ret;
> -}
> -
>  void intel_ddi_setup_hw_pll_state(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1039,9 +1004,6 @@ void intel_ddi_setup_hw_pll_state(struct drm_device *dev)
>  			continue;
>  		}
>  
> -		intel_crtc->config.ddi_pll_sel = intel_ddi_get_crtc_pll(dev_priv,
> -								 pipe);
> -
>  		switch (intel_crtc->config.ddi_pll_sel) {
>  		case PORT_CLK_SEL_WRPLL1:
>  			dev_priv->ddi_plls.wrpll1_refcount++;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 728b5a25cb80..1601da1b57a1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7006,6 +7006,35 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>  	return 0;
>  }
>  
> +static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
> +				       struct intel_crtc_config *pipe_config)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum port port;
> +	uint32_t tmp;
> +
> +	tmp = I915_READ(TRANS_DDI_FUNC_CTL(pipe_config->cpu_transcoder));
> +
> +	port = (tmp & TRANS_DDI_PORT_MASK) >> TRANS_DDI_PORT_SHIFT;
> +
> +	pipe_config->ddi_pll_sel = I915_READ(PORT_CLK_SEL(port));
> +	/*
> +	 * Haswell has only FDI/PCH transcoder A. It is which is connected to
> +	 * DDI E. So just check whether this pipe is wired to DDI E and whether
> +	 * the PCH transcoder is on.
> +	 */
> +	if ((port == PORT_E) && I915_READ(LPT_TRANSCONF) & TRANS_ENABLE) {
> +		pipe_config->has_pch_encoder = true;
> +
> +		tmp = I915_READ(FDI_RX_CTL(PIPE_A));
> +		pipe_config->fdi_lanes = ((FDI_DP_PORT_WIDTH_MASK & tmp) >>
> +					  FDI_DP_PORT_WIDTH_SHIFT) + 1;
> +
> +		ironlake_get_fdi_m_n_config(crtc, pipe_config);
> +	}
> +}
> +
>  static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  				    struct intel_crtc_config *pipe_config)
>  {
> @@ -7051,22 +7080,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  	if (!(tmp & PIPECONF_ENABLE))
>  		return false;
>  
> -	/*
> -	 * Haswell has only FDI/PCH transcoder A. It is which is connected to
> -	 * DDI E. So just check whether this pipe is wired to DDI E and whether
> -	 * the PCH transcoder is on.
> -	 */
> -	tmp = I915_READ(TRANS_DDI_FUNC_CTL(pipe_config->cpu_transcoder));
> -	if ((tmp & TRANS_DDI_PORT_MASK) == TRANS_DDI_SELECT_PORT(PORT_E) &&
> -	    I915_READ(LPT_TRANSCONF) & TRANS_ENABLE) {
> -		pipe_config->has_pch_encoder = true;
> -
> -		tmp = I915_READ(FDI_RX_CTL(PIPE_A));
> -		pipe_config->fdi_lanes = ((FDI_DP_PORT_WIDTH_MASK & tmp) >>
> -					  FDI_DP_PORT_WIDTH_SHIFT) + 1;
> -
> -		ironlake_get_fdi_m_n_config(crtc, pipe_config);
> -	}
> +	haswell_get_ddi_port_state(crtc, pipe_config);
>  
>  	intel_get_pipe_timings(crtc, pipe_config);
>  
> @@ -9521,6 +9535,8 @@ intel_pipe_config_compare(struct drm_device *dev,
>  
>  	PIPE_CONF_CHECK_I(double_wide);
>  
> +	PIPE_CONF_CHECK_X(ddi_pll_sel);
> +
>  	PIPE_CONF_CHECK_I(shared_dpll);
>  	PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
>  	PIPE_CONF_CHECK_X(dpll_hw_state.dpll_md);
> -- 
> 1.8.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx