[Intel-gfx] [PATCH 09/26] drm/i915: split PLL update code out of i9xx_crtc_mode_set

Daniel Vetter daniel at ffwll.ch
Sat Mar 24 00:00:11 CET 2012


On Thu, Mar 22, 2012 at 02:38:51PM -0700, Jesse Barnes wrote:
> Makes it more readable and maintainable.  ValleyView will add its own
> PLL update function in a later patch.
> 
> v2: split LVDS bits out of this patch (Daniel)
> 
> Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>

In-depth review ftw, i.e. two things below. Otherwise this looks good, so
keep everything else exactly as-is (I hate doing these kind of split
things up reviews).
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |  291 ++++++++++++++++++++--------------
>  1 files changed, 172 insertions(+), 119 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f27728c..84480da 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5230,6 +5230,170 @@ static void i9xx_update_pll_dividers(struct drm_crtc *crtc,
>  	}
>  }
>  
> +static void i9xx_update_pll(struct drm_crtc *crtc,
> +			    struct drm_display_mode *mode,
> +			    struct drm_display_mode *adjusted_mode,
> +			    intel_clock_t *clock, intel_clock_t *reduced_clock,
> +			    int num_connectors)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	int pipe = intel_crtc->pipe;
> +	u32 dpll;
> +	bool is_sdvo;
> +
> +	is_sdvo = intel_pipe_has_type(crtc, INTEL_OUTPUT_SDVO) ||
> +		intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI);
> +
> +	dpll = DPLL_VGA_MODE_DIS;
> +
> +	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS))
> +		dpll |= DPLLB_MODE_LVDS;
> +	else
> +		dpll |= DPLLB_MODE_DAC_SERIAL;
> +	if (is_sdvo) {
> +		int pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
> +		if (pixel_multiplier > 1) {
> +			if (IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev))
> +				dpll |= (pixel_multiplier - 1) << SDVO_MULTIPLIER_SHIFT_HIRES;
> +		}
> +		dpll |= DPLL_DVO_HIGH_SPEED;
> +	}
> +	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT))
> +		dpll |= DPLL_DVO_HIGH_SPEED;
> +
> +	/* compute bitmask from p1 value */
> +	if (IS_PINEVIEW(dev))
> +		dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT_PINEVIEW;
> +	else {
> +		dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> +		if (IS_G4X(dev) && reduced_clock)
> +			dpll |= (1 << (reduced_clock->p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
> +	}
> +	switch (clock->p2) {
> +	case 5:
> +		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5;
> +		break;
> +	case 7:
> +		dpll |= DPLLB_LVDS_P2_CLOCK_DIV_7;
> +		break;
> +	case 10:
> +		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_10;
> +		break;
> +	case 14:
> +		dpll |= DPLLB_LVDS_P2_CLOCK_DIV_14;
> +		break;
> +	}
> +	if (INTEL_INFO(dev)->gen >= 4)
> +		dpll |= (6 << PLL_LOAD_PULSE_PHASE_SHIFT);
> +
> +	if (is_sdvo && intel_pipe_has_type(crtc, INTEL_OUTPUT_TVOUT))
> +		dpll |= PLL_REF_INPUT_TVCLKINBC;
> +	else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_TVOUT))
> +		/* XXX: just matching BIOS for now */
> +		/*	dpll |= PLL_REF_INPUT_TVCLKINBC; */
> +		dpll |= 3;
> +	else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) &&
> +		 intel_panel_use_ssc(dev_priv) && num_connectors < 2)
> +		dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
> +	else
> +		dpll |= PLL_REF_INPUT_DREFCLK;
> +
> +	dpll |= DPLL_VCO_ENABLE;
> +	I915_WRITE(DPLL(pipe), dpll & ~DPLL_VCO_ENABLE);
> +	POSTING_READ(DPLL(pipe));
> +	udelay(150);
> +
> +	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT))
> +		intel_dp_set_m_n(crtc, mode, adjusted_mode);
> +
> +	I915_WRITE(DPLL(pipe), dpll);
> +
> +	/* Wait for the clocks to stabilize. */
> +	POSTING_READ(DPLL(pipe));
> +	udelay(150);
> +
> +	if (INTEL_INFO(dev)->gen >= 4) {
> +		u32 temp = 0;
> +		if (is_sdvo) {
> +			temp = intel_mode_get_pixel_multiplier(adjusted_mode);
> +			if (temp > 1)
> +				temp = (temp - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
> +			else
> +				temp = 0;
> +		}
> +		I915_WRITE(DPLL_MD(pipe), temp);
> +	} else {
> +		/* The pixel multiplier can only be updated once the
> +		 * DPLL is enabled and the clocks are stable.
> +		 *
> +		 * So write it again.
> +		 */
> +		I915_WRITE(DPLL(pipe), dpll);
> +	}
> +}
> +
> +static void i8xx_update_pll(struct drm_crtc *crtc,
> +			    struct drm_display_mode *adjusted_mode,
> +			    intel_clock_t *clock,
> +			    int num_connectors)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	int pipe = intel_crtc->pipe;
> +	u32 dpll;
> +
> +	dpll = DPLL_VGA_MODE_DIS;
> +
> +	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
> +		dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> +	} else {
> +		if (clock->p1 == 2)
> +			dpll |= PLL_P1_DIVIDE_BY_TWO;
> +		else
> +			dpll |= (clock->p1 - 2) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> +		if (clock->p2 == 4)
> +			dpll |= PLL_P2_DIVIDE_BY_4;
> +	}
> +
> +	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_TVOUT))
> +		/* XXX: just matching BIOS for now */
> +		/*	dpll |= PLL_REF_INPUT_TVCLKINBC; */
> +		dpll |= 3;
> +	else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) &&
> +		 intel_panel_use_ssc(dev_priv) && num_connectors < 2)
> +		dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;

Just curious: Does gen2 really have ssc on lvds?

> +	else
> +		dpll |= PLL_REF_INPUT_DREFCLK;
> +
> +	dpll |= DPLL_VCO_ENABLE;
> +	I915_WRITE(DPLL(pipe), dpll & ~DPLL_VCO_ENABLE);
> +	POSTING_READ(DPLL(pipe));
> +	udelay(150);
> +
> +	/* The LVDS pin pair needs to be on before the DPLLs are enabled.
> +	 * This is an exception to the general rule that mode_set doesn't turn
> +	 * things on.
> +	 */
> +	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS))
> +		intel_update_lvds(crtc, clock, adjusted_mode);
> +
> +	I915_WRITE(DPLL(pipe), dpll);
> +
> +	/* Wait for the clocks to stabilize. */
> +	POSTING_READ(DPLL(pipe));
> +	udelay(150);
> +
> +	/* The pixel multiplier can only be updated once the
> +	 * DPLL is enabled and the clocks are stable.
> +	 *
> +	 * So write it again.
> +	 */
> +	I915_WRITE(DPLL(pipe), dpll);
> +}
> +
>  static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  			      struct drm_display_mode *mode,
>  			      struct drm_display_mode *adjusted_mode,
> @@ -5243,9 +5407,9 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  	int plane = intel_crtc->plane;
>  	int refclk, num_connectors = 0;
>  	intel_clock_t clock, reduced_clock;
> -	u32 dpll, dspcntr, pipeconf, vsyncshift;
> -	bool ok, has_reduced_clock = false, is_sdvo = false, is_dvo = false;
> -	bool is_crt = false, is_lvds = false, is_tv = false, is_dp = false;
> +	u32 dspcntr, pipeconf, vsyncshift;
> +	bool ok, has_reduced_clock = false, is_sdvo = false;
> +	bool is_lvds = false, is_tv = false;
>  	struct drm_mode_config *mode_config = &dev->mode_config;
>  	struct intel_encoder *encoder;
>  	const intel_limit_t *limit;
> @@ -5267,18 +5431,9 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  			if (encoder->needs_tv_clock)
>  				is_tv = true;
>  			break;
> -		case INTEL_OUTPUT_DVO:
> -			is_dvo = true;
> -			break;
>  		case INTEL_OUTPUT_TVOUT:
>  			is_tv = true;
>  			break;
> -		case INTEL_OUTPUT_ANALOG:
> -			is_crt = true;
> -			break;
> -		case INTEL_OUTPUT_DISPLAYPORT:
> -			is_dp = true;
> -			break;
>  		}
>  
>  		num_connectors++;
> @@ -5322,71 +5477,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  	i9xx_update_pll_dividers(crtc, &clock, has_reduced_clock ?
>  				 &reduced_clock : NULL);
>  
> -	dpll = DPLL_VGA_MODE_DIS;
> -
> -	if (!IS_GEN2(dev)) {
> -		if (is_lvds)
> -			dpll |= DPLLB_MODE_LVDS;
> -		else
> -			dpll |= DPLLB_MODE_DAC_SERIAL;
> -		if (is_sdvo) {
> -			int pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
> -			if (pixel_multiplier > 1) {
> -				if (IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev))
> -					dpll |= (pixel_multiplier - 1) << SDVO_MULTIPLIER_SHIFT_HIRES;
> -			}
> -			dpll |= DPLL_DVO_HIGH_SPEED;
> -		}
> -		if (is_dp)
> -			dpll |= DPLL_DVO_HIGH_SPEED;
> -
> -		/* compute bitmask from p1 value */
> -		if (IS_PINEVIEW(dev))
> -			dpll |= (1 << (clock.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT_PINEVIEW;
> -		else {
> -			dpll |= (1 << (clock.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> -			if (IS_G4X(dev) && has_reduced_clock)
> -				dpll |= (1 << (reduced_clock.p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
> -		}
> -		switch (clock.p2) {
> -		case 5:
> -			dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5;
> -			break;
> -		case 7:
> -			dpll |= DPLLB_LVDS_P2_CLOCK_DIV_7;
> -			break;
> -		case 10:
> -			dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_10;
> -			break;
> -		case 14:
> -			dpll |= DPLLB_LVDS_P2_CLOCK_DIV_14;
> -			break;
> -		}
> -		if (INTEL_INFO(dev)->gen >= 4)
> -			dpll |= (6 << PLL_LOAD_PULSE_PHASE_SHIFT);
> -	} else {
> -		if (is_lvds) {
> -			dpll |= (1 << (clock.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> -		} else {
> -			if (clock.p1 == 2)
> -				dpll |= PLL_P1_DIVIDE_BY_TWO;
> -			else
> -				dpll |= (clock.p1 - 2) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> -			if (clock.p2 == 4)
> -				dpll |= PLL_P2_DIVIDE_BY_4;
> -		}
> -	}
> -
> -	if (is_sdvo && is_tv)
> -		dpll |= PLL_REF_INPUT_TVCLKINBC;
> -	else if (is_tv)
> -		/* XXX: just matching BIOS for now */
> -		/*	dpll |= PLL_REF_INPUT_TVCLKINBC; */
> -		dpll |= 3;
> -	else if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2)
> -		dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
> +	if (IS_GEN2(dev))
> +		i8xx_update_pll(crtc, adjusted_mode, &clock, num_connectors);
>  	else
> -		dpll |= PLL_REF_INPUT_DREFCLK;
> +		i9xx_update_pll(crtc, mode, adjusted_mode, &clock,
> +				has_reduced_clock ? &reduced_clock : NULL,
> +				num_connectors);
>  
>  	/* setup pipeconf */
>  	pipeconf = I915_READ(PIPECONF(pipe));
> @@ -5415,24 +5511,10 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  
>  	/* default to 8bpc */
>  	pipeconf &= ~(PIPECONF_BPP_MASK | PIPECONF_DITHER_EN);
> -	if (is_dp) {
> -		if (mode->private_flags & INTEL_MODE_DP_FORCE_6BPC) {
> -			pipeconf |= PIPECONF_BPP_6 |
> -				    PIPECONF_DITHER_EN |
> -				    PIPECONF_DITHER_TYPE_SP;
> -		}
> -	}

This hunk seems to have gotten lost. Why?

> -
> -	dpll |= DPLL_VCO_ENABLE;
>  
>  	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe == 0 ? 'A' : 'B');
>  	drm_mode_debug_printmodeline(mode);
>  
> -	I915_WRITE(DPLL(pipe), dpll & ~DPLL_VCO_ENABLE);
> -
> -	POSTING_READ(DPLL(pipe));
> -	udelay(150);
> -
>  	/* The LVDS pin pair needs to be on before the DPLLs are enabled.
>  	 * This is an exception to the general rule that mode_set doesn't turn
>  	 * things on.
> @@ -5485,35 +5567,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  		I915_WRITE(LVDS, temp);
>  	}
>  
> -	if (is_dp) {
> -		intel_dp_set_m_n(crtc, mode, adjusted_mode);
> -	}
> -
> -	I915_WRITE(DPLL(pipe), dpll);
> -
> -	/* Wait for the clocks to stabilize. */
> -	POSTING_READ(DPLL(pipe));
> -	udelay(150);
> -
> -	if (INTEL_INFO(dev)->gen >= 4) {
> -		temp = 0;
> -		if (is_sdvo) {
> -			temp = intel_mode_get_pixel_multiplier(adjusted_mode);
> -			if (temp > 1)
> -				temp = (temp - 1) << DPLL_MD_UDI_MULTIPLIER_SHIFT;
> -			else
> -				temp = 0;
> -		}
> -		I915_WRITE(DPLL_MD(pipe), temp);
> -	} else {
> -		/* The pixel multiplier can only be updated once the
> -		 * DPLL is enabled and the clocks are stable.
> -		 *
> -		 * So write it again.
> -		 */
> -		I915_WRITE(DPLL(pipe), dpll);
> -	}
> -
>  	if (HAS_PIPE_CXSR(dev)) {
>  		if (intel_crtc->lowfreq_avail) {
>  			DRM_DEBUG_KMS("enabling CxSR downclocking\n");
> -- 
> 1.7.5.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48



More information about the Intel-gfx mailing list