[Intel-gfx] [PATCH 3/3] drm/i915: split out DPLL update code from i9xx_crtc_mode_set

Daniel Vetter daniel at ffwll.ch
Mon Jan 16 22:35:03 CET 2012


On Thu, Dec 15, 2011 at 12:30:38PM -0800, Jesse Barnes wrote:
> More i9xx mode set cleanups, further simplifying the mode set path and
> making it easier to extend.
> 
> Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>

This is imo too much refactoring for just one patch. I'd like the extract
intel_update_lvds in a separate patch. That one intermingled with the
update_pll extraction makes things hard to follow.

Patches 1&2 of this series are queued for -next. Also, can we _please_
have the same treatment for ironlake_crtc_mode_set? That monstrositiy is
already around 450 lines and growing ...

Yours, Daniel
> ---
>  drivers/gpu/drm/i915/intel_display.c |  402 ++++++++++++++++++++--------------
>  1 files changed, 236 insertions(+), 166 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b08089c..0eff163 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4848,7 +4848,7 @@ static void i9xx_update_pll_dividers(struct drm_crtc *crtc,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_crtc->pipe;
> -	u32 fp, fp2;
> +	u32 fp, fp2 = 0;
>  
>  	if (IS_PINEVIEW(dev)) {
>  		fp = (1 << clock->n) << 16 | clock->m1 << 8 | clock->m2;
> @@ -4874,6 +4874,233 @@ static void i9xx_update_pll_dividers(struct drm_crtc *crtc,
>  	}
>  }
>  
> +static void intel_update_lvds(struct drm_crtc *crtc, intel_clock_t *clock,
> +			      struct drm_display_mode *adjusted_mode)
> +{
> +	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 temp, lvds_sync = 0;
> +
> +	temp = I915_READ(LVDS);
> +	temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
> +	if (pipe == 1) {
> +		temp |= LVDS_PIPEB_SELECT;
> +	} else {
> +		temp &= ~LVDS_PIPEB_SELECT;
> +	}
> +	/* set the corresponsding LVDS_BORDER bit */
> +	temp |= dev_priv->lvds_border_bits;
> +	/* Set the B0-B3 data pairs corresponding to whether we're going to
> +	 * set the DPLLs for dual-channel mode or not.
> +	 */
> +	if (clock->p2 == 7)
> +		temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
> +	else
> +		temp &= ~(LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP);
> +
> +	/* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
> +	 * appropriately here, but we need to look more thoroughly into how
> +	 * panels behave in the two modes.
> +	 */
> +	/* set the dithering flag on LVDS as needed */
> +	if (INTEL_INFO(dev)->gen >= 4) {
> +		if (dev_priv->lvds_dither)
> +			temp |= LVDS_ENABLE_DITHER;
> +		else
> +			temp &= ~LVDS_ENABLE_DITHER;
> +	}
> +	if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
> +		lvds_sync |= LVDS_HSYNC_POLARITY;
> +	if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC)
> +		lvds_sync |= LVDS_VSYNC_POLARITY;
> +	if ((temp & (LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY))
> +	    != lvds_sync) {
> +		char flags[2] = "-+";
> +		DRM_INFO("Changing LVDS panel from "
> +			 "(%chsync, %cvsync) to (%chsync, %cvsync)\n",
> +			 flags[!(temp & LVDS_HSYNC_POLARITY)],
> +			 flags[!(temp & LVDS_VSYNC_POLARITY)],
> +			 flags[!(lvds_sync & LVDS_HSYNC_POLARITY)],
> +			 flags[!(lvds_sync & LVDS_VSYNC_POLARITY)]);
> +		temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
> +		temp |= lvds_sync;
> +	}
> +	I915_WRITE(LVDS, temp);
> +}
> +
> +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);
> +
> +	/* 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);
> +
> +	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;
> +	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,
> @@ -4887,15 +5114,13 @@ 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;
> -	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;
> +	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;
>  	int ret;
> -	u32 temp;
> -	u32 lvds_sync = 0;
>  
>  	list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
>  		if (encoder->base.crtc != crtc)
> @@ -4911,18 +5136,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++;
> @@ -4969,71 +5185,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));
> @@ -5062,97 +5219,10 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  			pipeconf &= ~PIPECONF_DOUBLE_WIDE;
>  	}
>  
> -	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.
> -	 */
> -	if (is_lvds) {
> -		temp = I915_READ(LVDS);
> -		temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
> -		if (pipe == 1) {
> -			temp |= LVDS_PIPEB_SELECT;
> -		} else {
> -			temp &= ~LVDS_PIPEB_SELECT;
> -		}
> -		/* set the corresponsding LVDS_BORDER bit */
> -		temp |= dev_priv->lvds_border_bits;
> -		/* Set the B0-B3 data pairs corresponding to whether we're going to
> -		 * set the DPLLs for dual-channel mode or not.
> -		 */
> -		if (clock.p2 == 7)
> -			temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
> -		else
> -			temp &= ~(LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP);
> -
> -		/* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
> -		 * appropriately here, but we need to look more thoroughly into how
> -		 * panels behave in the two modes.
> -		 */
> -		/* set the dithering flag on LVDS as needed */
> -		if (INTEL_INFO(dev)->gen >= 4) {
> -			if (dev_priv->lvds_dither)
> -				temp |= LVDS_ENABLE_DITHER;
> -			else
> -				temp &= ~LVDS_ENABLE_DITHER;
> -		}
> -		if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
> -			lvds_sync |= LVDS_HSYNC_POLARITY;
> -		if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC)
> -			lvds_sync |= LVDS_VSYNC_POLARITY;
> -		if ((temp & (LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY))
> -		    != lvds_sync) {
> -			char flags[2] = "-+";
> -			DRM_INFO("Changing LVDS panel from "
> -				 "(%chsync, %cvsync) to (%chsync, %cvsync)\n",
> -				 flags[!(temp & LVDS_HSYNC_POLARITY)],
> -				 flags[!(temp & LVDS_VSYNC_POLARITY)],
> -				 flags[!(lvds_sync & LVDS_HSYNC_POLARITY)],
> -				 flags[!(lvds_sync & LVDS_VSYNC_POLARITY)]);
> -			temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
> -			temp |= lvds_sync;
> -		}
> -		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.4.1
> 
> _______________________________________________
> 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