Re: [PATCH 24/31] drm/i915: asserts for lvds pre_enable

[Imre Deak <imre.deak@intel.com>]

On Wed, 2013-06-05 at 13:34 +0200, Daniel Vetter wrote:
> Lots of bangin my head against the wall^UExperiments have shown that
> we really need to enable the lvds port before we enable plls. Strangely
> that seems to include the fdi rx pll on the pch.
> 
> Anyway, encode this new evidence with a few nice WARNs.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++-------------
>  drivers/gpu/drm/i915/intel_drv.h     | 16 ++++++++++++++++
>  drivers/gpu/drm/i915/intel_lvds.c    | 17 ++++++++++++-----
>  3 files changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5d84fea..7b34a92 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -874,8 +874,8 @@ static const char *state_string(bool enabled)
>  }
>  
>  /* Only for pre-ILK configs */
> -static void assert_pll(struct drm_i915_private *dev_priv,
> -		       enum pipe pipe, bool state)
> +void assert_pll(struct drm_i915_private *dev_priv,
> +		enum pipe pipe, bool state)
>  {
>  	int reg;
>  	u32 val;
> @@ -888,10 +888,8 @@ static void assert_pll(struct drm_i915_private *dev_priv,
>  	     "PLL state assertion failure (expected %s, current %s)\n",
>  	     state_string(state), state_string(cur_state));
>  }
> -#define assert_pll_enabled(d, p) assert_pll(d, p, true)
> -#define assert_pll_disabled(d, p) assert_pll(d, p, false)
>  
> -static struct intel_shared_dpll *
> +struct intel_shared_dpll *
>  intel_crtc_to_shared_dpll(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> @@ -903,9 +901,9 @@ intel_crtc_to_shared_dpll(struct intel_crtc *crtc)
>  }
>  
>  /* For ILK+ */
> -static void assert_shared_dpll(struct drm_i915_private *dev_priv,
> -			       struct intel_shared_dpll *pll,
> -			       bool state)
> +void assert_shared_dpll(struct drm_i915_private *dev_priv,
> +			struct intel_shared_dpll *pll,
> +			bool state)
>  {
>  	bool cur_state;
>  	struct intel_dpll_hw_state hw_state;
> @@ -924,8 +922,6 @@ static void assert_shared_dpll(struct drm_i915_private *dev_priv,
>  	     "%s assertion failure (expected %s, current %s)\n",
>  	     pll->name, state_string(state), state_string(cur_state));
>  }
> -#define assert_shared_dpll_enabled(d, p) assert_shared_dpll(d, p, true)
> -#define assert_shared_dpll_disabled(d, p) assert_shared_dpll(d, p, false)
>  
>  static void assert_fdi_tx(struct drm_i915_private *dev_priv,
>  			  enum pipe pipe, bool state)
> @@ -989,15 +985,16 @@ static void assert_fdi_tx_pll_enabled(struct drm_i915_private *dev_priv,
>  	WARN(!(val & FDI_TX_PLL_ENABLE), "FDI TX PLL assertion failure, should be active but is disabled\n");
>  }
>  
> -static void assert_fdi_rx_pll_enabled(struct drm_i915_private *dev_priv,
> -				      enum pipe pipe)
> +void assert_fdi_rx_pll(struct drm_i915_private *dev_priv,
> +		       enum pipe pipe, bool state)
>  {
>  	int reg;
>  	u32 val;
>  
>  	reg = FDI_RX_CTL(pipe);
>  	val = I915_READ(reg);
> -	WARN(!(val & FDI_RX_PLL_ENABLE), "FDI RX PLL assertion failure, should be active but is disabled\n");
> +	WARN(!!(val & FDI_RX_PLL_ENABLE) != state,
> +	     "FDI RX PLL assertion failure, should be active but is disabled\n");

The message should be updated too.

>  }
>  
>  static void assert_panel_unlocked(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6f28375..ea8aa5e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -732,6 +732,22 @@ extern int intel_overlay_attrs(struct drm_device *dev, void *data,
>  extern void intel_fb_output_poll_changed(struct drm_device *dev);
>  extern void intel_fb_restore_mode(struct drm_device *dev);
>  
> +struct intel_shared_dpll *
> +intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> +
> +void assert_shared_dpll(struct drm_i915_private *dev_priv,
> +			struct intel_shared_dpll *pll,
> +			bool state);
> +#define assert_shared_dpll_enabled(d, p) assert_shared_dpll(d, p, true)
> +#define assert_shared_dpll_disabled(d, p) assert_shared_dpll(d, p, false)
> +void assert_pll(struct drm_i915_private *dev_priv,
> +		enum pipe pipe, bool state);
> +#define assert_pll_enabled(d, p) assert_pll(d, p, true)
> +#define assert_pll_disabled(d, p) assert_pll(d, p, false)
> +void assert_fdi_rx_pll(struct drm_i915_private *dev_priv,
> +		       enum pipe pipe, bool state);
> +#define assert_fdi_rx_pll_enabled(d, p) assert_fdi_rx_pll(d, p, true)
> +#define assert_fdi_rx_pll_disabled(d, p) assert_fdi_rx_pll(d, p, false)
>  extern void assert_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
>  			bool state);
>  #define assert_pipe_enabled(d, p) assert_pipe(d, p, true)
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 159aa9f..36f8901 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -120,12 +120,20 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
>  	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> +	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
>  	struct drm_display_mode *fixed_mode =
>  		lvds_encoder->attached_connector->base.panel.fixed_mode;
> -	int pipe = intel_crtc->pipe;
> +	int pipe = crtc->pipe;
>  	u32 temp;
>  
> +	if (HAS_PCH_SPLIT(dev)) {
> +		assert_fdi_rx_pll_disabled(dev_priv, pipe);
> +		assert_shared_dpll_disabled(dev_priv,
> +					    intel_crtc_to_shared_dpll(crtc));

I think if we pick a shared PLL that is currently used by another port
this will trigger. Should the PLL selection be limited to non-shared
PLLs for LVDS?

> +	} else {
> +		assert_pll_disabled(dev_priv, pipe);
> +	}
> +
>  	temp = I915_READ(lvds_encoder->reg);
>  	temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
>  
> @@ -142,7 +150,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
>  
>  	/* set the corresponsding LVDS_BORDER bit */
>  	temp &= ~LVDS_BORDER_ENABLE;
> -	temp |= intel_crtc->config.gmch_pfit.lvds_border_bits;
> +	temp |= crtc->config.gmch_pfit.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.
>  	 */
> @@ -162,8 +170,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
>  	if (INTEL_INFO(dev)->gen == 4) {
>  		/* Bspec wording suggests that LVDS port dithering only exists
>  		 * for 18bpp panels. */
> -		if (intel_crtc->config.dither &&
> -		    intel_crtc->config.pipe_bpp == 18)
> +		if (crtc->config.dither && crtc->config.pipe_bpp == 18)
>  			temp |= LVDS_ENABLE_DITHER;
>  		else
>  			temp &= ~LVDS_ENABLE_DITHER;