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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, May 20, 2014 at 11:47:35AM +0100, Damien Lespiau wrote:
> 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?

Fewer forward decls and non-static functions. If you have a file with a
bunch of functions but they're all called from somewhere else, then the
split-up is imo not functionally sound. intel_ddi.c has a bit that taste
for me.

We could fix this I guess by moving a lot more into intel_ddi.c, like all
the hsw+ modeset code.

> 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch