Re: [PATCH 10/26] drm/i915: split LVDS update code out of i9xx_crtc_mode_set

From: Daniel Vetter <daniel@ffwll.ch>
To: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 10/26] drm/i915: split LVDS update code out of i9xx_crtc_mode_set
Date: Sat, 24 Mar 2012 00:04:58 +0100	[thread overview]
Message-ID: <20120323230458.GD29390@phenom.ffwll.local> (raw)
In-Reply-To: <1332452348-8814-11-git-send-email-jbarnes@virtuousgeek.org>

On Thu, Mar 22, 2012 at 02:38:52PM -0700, Jesse Barnes wrote:
> Just to make things clearer and reduce the size of this monstrosity.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

In-depth review ftw. This time the code looks good, but the call the the
new funtion looks a bit misplaced. See below.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |  117 ++++++++++++++++++----------------
>  1 files changed, 63 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 84480da..adfa19b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5230,6 +5230,62 @@ 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,
> @@ -5305,6 +5361,13 @@ static void i9xx_update_pll(struct drm_crtc *crtc,
>  	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);
> +

Why is this code now at a different place? Especially since it looks like
this will break my dear old i855gm :(

>  	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT))
>  		intel_dp_set_m_n(crtc, mode, adjusted_mode);
>  
> @@ -5414,8 +5477,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  	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)
> @@ -5515,58 +5576,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe == 0 ? 'A' : 'B');
>  	drm_mode_debug_printmodeline(mode);
>  
> -	/* 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 (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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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