Re: [PATCH 2/6] drm/i915: Move the encoder vs. FDI dotclock check out from encoder .get_config()

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

On ke, 2016-02-17 at 21:41 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we check if the encoder's idea of dotclock agrees with what
> we calculated based on the FDI parameters. We do this in the encoder
> .get_config() hooks, which isn't so nice in case the BIOS (or some
> other
> outside party) made a mess of the state and we're just trying to take
> over.
> 
> So as a prep step to being able sanitize such a bogus state, move the
> the sanity check to just after we've read out the entire state. If
> we then need to sanitize a bad state, it should be easier to move the
> sanity check to occur after sanitation instead of before it.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Separating the get-config and check steps makes things more logical in
any case. Looks ok to me:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_crt.c     | 10 +------
>  drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++++++++++----
> ------------
>  drivers/gpu/drm/i915/intel_dp.c      | 11 ++-----
>  drivers/gpu/drm/i915/intel_drv.h     |  3 --
>  drivers/gpu/drm/i915/intel_hdmi.c    |  3 --
>  drivers/gpu/drm/i915/intel_lvds.c    |  8 +----
>  drivers/gpu/drm/i915/intel_sdvo.c    |  4 +--
>  7 files changed, 38 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c
> b/drivers/gpu/drm/i915/intel_crt.c
> index e686a91a416e..f4c88d93a164 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -120,17 +120,9 @@ static unsigned int intel_crt_get_flags(struct
> intel_encoder *encoder)
>  static void intel_crt_get_config(struct intel_encoder *encoder,
>  				 struct intel_crtc_state
> *pipe_config)
>  {
> -	struct drm_device *dev = encoder->base.dev;
> -	int dotclock;
> -
>  	pipe_config->base.adjusted_mode.flags |=
> intel_crt_get_flags(encoder);
>  
> -	dotclock = pipe_config->port_clock;
> -
> -	if (HAS_PCH_SPLIT(dev))
> -		ironlake_check_encoder_dotclock(pipe_config,
> dotclock);
> -
> -	pipe_config->base.adjusted_mode.crtc_clock = dotclock;
> +	pipe_config->base.adjusted_mode.crtc_clock = pipe_config-
> >port_clock;
>  }
>  
>  static void hsw_crt_get_config(struct intel_encoder *encoder,
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index f0f88061a9e5..99001e117517 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -224,12 +224,11 @@ static void intel_update_czclk(struct
> drm_i915_private *dev_priv)
>  }
>  
>  static inline u32 /* units of 100MHz */
> -intel_fdi_link_freq(struct drm_device *dev)
> +intel_fdi_link_freq(struct drm_i915_private *dev_priv)
>  {
> -	if (IS_GEN5(dev)) {
> -		struct drm_i915_private *dev_priv = dev-
> >dev_private;
> +	if (IS_GEN5(dev_priv))
>  		return (I915_READ(FDI_PLL_BIOS_0) &
> FDI_PLL_FB_CLOCK_MASK) + 2;
> -	} else
> +	else
>  		return 27;
>  }
>  
> @@ -6589,7 +6588,7 @@ retry:
>  	 * Hence the bw of each lane in terms of the mode signal
>  	 * is:
>  	 */
> -	link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
> +	link_bw = intel_fdi_link_freq(to_i915(dev)) *
> MHz(100)/KHz(1)/10;
>  
>  	fdi_dotclock = adjusted_mode->crtc_clock;
>  
> @@ -6601,8 +6600,7 @@ retry:
>  	intel_link_compute_m_n(pipe_config->pipe_bpp, lane,
> fdi_dotclock,
>  			       link_bw, &pipe_config->fdi_m_n);
>  
> -	ret = ironlake_check_fdi_lanes(intel_crtc->base.dev,
> -				       intel_crtc->pipe,
> pipe_config);
> +	ret = ironlake_check_fdi_lanes(dev, intel_crtc->pipe,
> pipe_config);
>  	if (ret == -EINVAL && pipe_config->pipe_bpp > 6*3) {
>  		pipe_config->pipe_bpp -= 2*3;
>  		DRM_DEBUG_KMS("fdi link bw constraint, reducing pipe
> bpp to %i\n",
> @@ -10765,19 +10763,18 @@ int intel_dotclock_calculate(int link_freq,
>  static void ironlake_pch_clock_get(struct intel_crtc *crtc,
>  				   struct intel_crtc_state
> *pipe_config)
>  {
> -	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  
>  	/* read out port_clock from the DPLL */
>  	i9xx_crtc_clock_get(crtc, pipe_config);
>  
>  	/*
> -	 * This value does not include pixel_multiplier.
> -	 * We will check that port_clock and
> adjusted_mode.crtc_clock
> -	 * agree once we know their relationship in the encoder's
> -	 * get_config() function.
> +	 * In case there is an active pipe without active ports,
> +	 * we may need some idea for the dotclock anyway.
> +	 * Calculate one based on the FDI configuration.
>  	 */
>  	pipe_config->base.adjusted_mode.crtc_clock =
> -		intel_dotclock_calculate(intel_fdi_link_freq(dev) *
> 10000,
> +		intel_dotclock_calculate(intel_fdi_link_freq(dev_pri
> v) * 10000,
>  					 &pipe_config->fdi_m_n);
>  }
>  
> @@ -12788,6 +12785,24 @@ intel_pipe_config_compare(struct drm_device
> *dev,
>  	return ret;
>  }
>  
> +static void intel_pipe_config_sanity_check(struct drm_i915_private
> *dev_priv,
> +					   const struct
> intel_crtc_state *pipe_config)
> +{
> +	if (pipe_config->has_pch_encoder) {
> +		int fdi_dotclock =
> intel_dotclock_calculate(intel_fdi_link_freq(dev_priv) * 10000,
> +							    &pipe_co
> nfig->fdi_m_n);
> +		int dotclock = pipe_config-
> >base.adjusted_mode.crtc_clock;
> +
> +		/*
> +		 * FDI already provided one idea for the dotclock.
> +		 * Yell if the encoder disagrees.
> +		 */
> +		WARN(!intel_fuzzy_clock_check(fdi_dotclock,
> dotclock),
> +		     "FDI dotclock and encoder dotclock mismatch,
> fdi: %i, encoder: %i\n",
> +		     fdi_dotclock, dotclock);
> +	}
> +}
> +
>  static void check_wm_state(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -12961,6 +12976,8 @@ check_crtc_state(struct drm_device *dev,
> struct drm_atomic_state *old_state)
>  		if (!crtc->state->active)
>  			continue;
>  
> +		intel_pipe_config_sanity_check(dev_priv,
> pipe_config);
> +
>  		sw_config = to_intel_crtc_state(crtc->state);
>  		if (!intel_pipe_config_compare(dev, sw_config,
>  					       pipe_config, false))
> {
> @@ -13033,18 +13050,6 @@ intel_modeset_check_state(struct drm_device
> *dev,
>  	check_shared_dpll_state(dev);
>  }
>  
> -void ironlake_check_encoder_dotclock(const struct intel_crtc_state
> *pipe_config,
> -				     int dotclock)
> -{
> -	/*
> -	 * FDI already provided one idea for the dotclock.
> -	 * Yell if the encoder disagrees.
> -	 */
> -	WARN(!intel_fuzzy_clock_check(pipe_config-
> >base.adjusted_mode.crtc_clock, dotclock),
> -	     "FDI dotclock and encoder dotclock mismatch, fdi: %i,
> encoder: %i\n",
> -	     pipe_config->base.adjusted_mode.crtc_clock, dotclock);
> -}
> -
>  static void update_scanline_offset(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
> @@ -15847,6 +15852,8 @@ static void
> intel_modeset_readout_hw_state(struct drm_device *dev)
>  			drm_calc_timestamping_constants(&crtc->base, 
> &crtc->base.hwmode);
>  			update_scanline_offset(crtc);
>  		}
> +
> +		intel_pipe_config_sanity_check(dev_priv, crtc-
> >config);
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index cbc06596659a..f272b3541e00 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2409,7 +2409,6 @@ static void intel_dp_get_config(struct
> intel_encoder *encoder,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	enum port port = dp_to_dig_port(intel_dp)->port;
>  	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
> -	int dotclock;
>  
>  	tmp = I915_READ(intel_dp->output_reg);
>  
> @@ -2459,13 +2458,9 @@ static void intel_dp_get_config(struct
> intel_encoder *encoder,
>  			pipe_config->port_clock = 270000;
>  	}
>  
> -	dotclock = intel_dotclock_calculate(pipe_config->port_clock,
> -					    &pipe_config->dp_m_n);
> -
> -	if (HAS_PCH_SPLIT(dev_priv->dev) && port != PORT_A)
> -		ironlake_check_encoder_dotclock(pipe_config,
> dotclock);
> -
> -	pipe_config->base.adjusted_mode.crtc_clock = dotclock;
> +	pipe_config->base.adjusted_mode.crtc_clock =
> +		intel_dotclock_calculate(pipe_config->port_clock,
> +					 &pipe_config->dp_m_n);
>  
>  	if (is_edp(intel_dp) && dev_priv->vbt.edp_bpp &&
>  	    pipe_config->pipe_bpp > dev_priv->vbt.edp_bpp) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index f95f8b22939f..c25a8880b4e8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1197,9 +1197,6 @@ void intel_dp_get_m_n(struct intel_crtc *crtc,
>  		      struct intel_crtc_state *pipe_config);
>  void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set
> m_n);
>  int intel_dotclock_calculate(int link_freq, const struct
> intel_link_m_n *m_n);
> -void
> -ironlake_check_encoder_dotclock(const struct intel_crtc_state
> *pipe_config,
> -				int dotclock);
>  bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state, int
> target_clock,
>  			intel_clock_t *best_clock);
>  int chv_calc_dpll_params(int refclk, intel_clock_t *pll_clock);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index 80b44c054087..d8060e6251f8 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -952,9 +952,6 @@ static void intel_hdmi_get_config(struct
> intel_encoder *encoder,
>  	if (pipe_config->pixel_multiplier)
>  		dotclock /= pipe_config->pixel_multiplier;
>  
> -	if (HAS_PCH_SPLIT(dev_priv->dev))
> -		ironlake_check_encoder_dotclock(pipe_config,
> dotclock);
> -
>  	pipe_config->base.adjusted_mode.crtc_clock = dotclock;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c
> b/drivers/gpu/drm/i915/intel_lvds.c
> index 30a8403a8f4f..b35342f7b969 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -109,7 +109,6 @@ static void intel_lvds_get_config(struct
> intel_encoder *encoder,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_lvds_encoder *lvds_encoder =
> to_lvds_encoder(&encoder->base);
>  	u32 tmp, flags = 0;
> -	int dotclock;
>  
>  	tmp = I915_READ(lvds_encoder->reg);
>  	if (tmp & LVDS_HSYNC_POLARITY)
> @@ -130,12 +129,7 @@ static void intel_lvds_get_config(struct
> intel_encoder *encoder,
>  		pipe_config->gmch_pfit.control |= tmp &
> PANEL_8TO6_DITHER_ENABLE;
>  	}
>  
> -	dotclock = pipe_config->port_clock;
> -
> -	if (HAS_PCH_SPLIT(dev_priv->dev))
> -		ironlake_check_encoder_dotclock(pipe_config,
> dotclock);
> -
> -	pipe_config->base.adjusted_mode.crtc_clock = dotclock;
> +	pipe_config->base.adjusted_mode.crtc_clock = pipe_config-
> >port_clock;
>  }
>  
>  static void intel_pre_enable_lvds(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c
> b/drivers/gpu/drm/i915/intel_sdvo.c
> index 4ecc076c4041..fae64bc93c1b 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1398,12 +1398,10 @@ static void intel_sdvo_get_config(struct
> intel_encoder *encoder,
>  	}
>  
>  	dotclock = pipe_config->port_clock;
> +
>  	if (pipe_config->pixel_multiplier)
>  		dotclock /= pipe_config->pixel_multiplier;
>  
> -	if (HAS_PCH_SPLIT(dev))
> -		ironlake_check_encoder_dotclock(pipe_config,
> dotclock);
> -
>  	pipe_config->base.adjusted_mode.crtc_clock = dotclock;
>  
>  	/* Cross check the port pixel multiplier with the sdvo
> encoder state. */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx