Re: [PATCH 2/6] drm/i915: get mode clock when reading the pipe config v5

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, May 14, 2013 at 05:08:27PM -0700, Jesse Barnes wrote:
> We need this for comparing modes between configuration changes.
> 
> v2: try harder to calulate non-simple pixel clocks (Daniel)
>     call get_clock after getting the encoder config, needed for pixel multiply
>     (Jesse)
> v3: drop get_clock now that the pixel_multiply has been moved into
>     get_pipe_config
> v4: re-add get_clock; we need to get the pixel multiplier in the
>     encoder, so need to calculate the clock value after the encoder's
>     get_config is called
> v5: drop hsw clock_get, still needs to be written
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Two things:
- the pixel_multiplier should be a separate patch, and I think we should
  add pipe_config check support for it. Maybe if we pick a default value
  of 1 (both for the get_config hw state readout and the compute config
  stuff in the modeset code) that should also simplify a bit since only
  SDVO uses it atm. We still need to implement pixel multiplier stuff for
  native hdmi ports ...
- For the clock readout code I think we should be able to have pipe config
  compare support (with adjusted_mode->clock), with a bit of fuzz at
  least. Not on current dinq, but with my cleanup to give
  adjusted_mode->clock saner semantics:

  http://permalink.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/21750
- There's no reason imo for the new ->get_clock vfunc, it splits exactly
  the same as the existing ->get_pipe_config. So could simply be included
  there.

With those three adjustements I think this is ready to go. For the
follow-up patches though I think we should guard them with a fastboot
module option.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h      |    1 +
>  drivers/gpu/drm/i915/intel_crt.c     |    1 +
>  drivers/gpu/drm/i915/intel_display.c |   78 +++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_dp.c      |    8 ++++
>  drivers/gpu/drm/i915/intel_drv.h     |    3 ++
>  drivers/gpu/drm/i915/intel_dvo.c     |    1 +
>  drivers/gpu/drm/i915/intel_hdmi.c    |    1 +
>  drivers/gpu/drm/i915/intel_lvds.c    |    1 +
>  drivers/gpu/drm/i915/intel_sdvo.c    |   24 +++++++++++
>  9 files changed, 111 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 14817de..2e284bb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -323,6 +323,7 @@ struct drm_i915_display_funcs {
>  	 * fills out the pipe-config with the hw state. */
>  	bool (*get_pipe_config)(struct intel_crtc *,
>  				struct intel_crtc_config *);
> +	void (*get_clock)(struct intel_crtc *, struct intel_crtc_config *);
>  	int (*crtc_mode_set)(struct drm_crtc *crtc,
>  			     int x, int y,
>  			     struct drm_framebuffer *old_fb);
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 789c4ef..bd91e72 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -101,6 +101,7 @@ static void intel_crt_get_config(struct intel_encoder *encoder,
>  		flags |= DRM_MODE_FLAG_NVSYNC;
>  
>  	pipe_config->adjusted_mode.flags |= flags;
> +	pipe_config->pixel_multiplier = 1;
>  }
>  
>  static void intel_disable_crt(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 163b97e..356404c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -45,6 +45,11 @@ bool intel_pipe_has_type(struct drm_crtc *crtc, int type);
>  static void intel_increase_pllclock(struct drm_crtc *crtc);
>  static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
>  
> +static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
> +				struct intel_crtc_config *pipe_config);
> +static void ironlake_crtc_clock_get(struct intel_crtc *crtc,
> +				    struct intel_crtc_config *pipe_config);
> +
>  typedef struct {
>  	int	min, max;
>  } intel_range_t;
> @@ -6906,11 +6911,12 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>  }
>  
>  /* Returns the clock of the currently programmed mode of the given pipe. */
> -static int intel_crtc_clock_get(struct drm_device *dev, struct drm_crtc *crtc)
> +static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
> +				struct intel_crtc_config *pipe_config)
>  {
> +	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	int pipe = intel_crtc->pipe;
> +	int pipe = pipe_config->cpu_transcoder;
>  	u32 dpll = I915_READ(DPLL(pipe));
>  	u32 fp;
>  	intel_clock_t clock;
> @@ -6949,7 +6955,8 @@ static int intel_crtc_clock_get(struct drm_device *dev, struct drm_crtc *crtc)
>  		default:
>  			DRM_DEBUG_KMS("Unknown DPLL mode %08x in programmed "
>  				  "mode\n", (int)(dpll & DPLL_MODE_MASK));
> -			return 0;
> +			pipe_config->adjusted_mode.clock = 0;
> +			return;
>  		}
>  
>  		/* XXX: Handle the 100Mhz refclk */
> @@ -6988,8 +6995,54 @@ static int intel_crtc_clock_get(struct drm_device *dev, struct drm_crtc *crtc)
>  	 * i830PllIsValid() because it relies on the xf86_config connector
>  	 * configuration being accurate, which it isn't necessarily.
>  	 */
> +	pipe_config->adjusted_mode.clock = clock.dot;
> +}
> +
> +static void ironlake_crtc_clock_get(struct intel_crtc *crtc,
> +				    struct intel_crtc_config *pipe_config)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum transcoder cpu_transcoder = pipe_config->cpu_transcoder;
> +	int link_freq, repeat;
> +	u64 clock;
> +	u32 link_m, link_n;
>  
> -	return clock.dot;
> +	repeat = pipe_config->pixel_multiplier;
> +
> +	/*
> +	 * The calculation for the data clock is:
> +	 * pixel_clock = ((m/n)*(link_clock * nr_lanes * repeat))/bpp
> +	 * But we want to avoid losing precison if possible, so:
> +	 * pixel_clock = ((m * link_clock * nr_lanes * repeat)/(n*bpp))
> +	 *
> +	 * and the link clock is simpler:
> +	 * link_clock = (m * link_clock * repeat) / n
> +	 */
> +
> +	/*
> +	 * We need to get the FDI or DP link clock here to derive
> +	 * the M/N dividers.
> +	 *
> +	 * For FDI, we read it from the BIOS or use a fixed 2.7GHz.
> +	 * For DP, it's either 1.62GHz or 2.7GHz.
> +	 * We do our calculations in 10*MHz since we don't need much precison.
> +	 */
> +	if (pipe_config->has_pch_encoder)
> +		link_freq = intel_fdi_link_freq(dev) * 10000;
> +	else
> +		link_freq = pipe_config->cpu_edp_link_rate;
> +
> +	link_m = I915_READ(PIPE_LINK_M1(cpu_transcoder));
> +	link_n = I915_READ(PIPE_LINK_N1(cpu_transcoder));
> +
> +	if (!link_m || !link_n)
> +		return;
> +
> +	clock = ((u64)link_m * (u64)link_freq * (u64)repeat);
> +	do_div(clock, link_n);
> +
> +	pipe_config->adjusted_mode.clock = clock;
>  }
>  
>  /** Returns the currently programmed mode of the given pipe. */
> @@ -7000,6 +7053,7 @@ struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
>  	struct drm_display_mode *mode;
> +	struct intel_crtc_config pipe_config;
>  	int htot = I915_READ(HTOTAL(cpu_transcoder));
>  	int hsync = I915_READ(HSYNC(cpu_transcoder));
>  	int vtot = I915_READ(VTOTAL(cpu_transcoder));
> @@ -7009,7 +7063,10 @@ struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
>  	if (!mode)
>  		return NULL;
>  
> -	mode->clock = intel_crtc_clock_get(dev, crtc);
> +	pipe_config.cpu_transcoder = intel_crtc->pipe;
> +	i9xx_crtc_clock_get(intel_crtc, &pipe_config);
> +
> +	mode->clock = pipe_config.adjusted_mode.clock;
>  	mode->hdisplay = (htot & 0xffff) + 1;
>  	mode->htotal = ((htot & 0xffff0000) >> 16) + 1;
>  	mode->hsync_start = (hsync & 0xffff) + 1;
> @@ -9011,6 +9068,7 @@ static void intel_init_display(struct drm_device *dev)
>  		dev_priv->display.update_plane = ironlake_update_plane;
>  	} else if (HAS_PCH_SPLIT(dev)) {
>  		dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
> +		dev_priv->display.get_clock = ironlake_crtc_clock_get;
>  		dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
>  		dev_priv->display.crtc_enable = ironlake_crtc_enable;
>  		dev_priv->display.crtc_disable = ironlake_crtc_disable;
> @@ -9018,6 +9076,7 @@ static void intel_init_display(struct drm_device *dev)
>  		dev_priv->display.update_plane = ironlake_update_plane;
>  	} else if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
> +		dev_priv->display.get_clock = i9xx_crtc_clock_get;
>  		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
>  		dev_priv->display.crtc_enable = valleyview_crtc_enable;
>  		dev_priv->display.crtc_disable = i9xx_crtc_disable;
> @@ -9025,6 +9084,7 @@ static void intel_init_display(struct drm_device *dev)
>  		dev_priv->display.update_plane = i9xx_update_plane;
>  	} else {
>  		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
> +		dev_priv->display.get_clock = i9xx_crtc_clock_get;
>  		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
>  		dev_priv->display.crtc_enable = i9xx_crtc_enable;
>  		dev_priv->display.crtc_disable = i9xx_crtc_disable;
> @@ -9593,8 +9653,12 @@ setup_pipes:
>  		if (encoder->get_hw_state(encoder, &pipe)) {
>  			crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
>  			encoder->base.crtc = &crtc->base;
> -			if (encoder->get_config)
> +			if (encoder->get_config &&
> +			    dev_priv->display.get_clock) {
>  				encoder->get_config(encoder, &crtc->config);
> +				dev_priv->display.get_clock(crtc,
> +							    &crtc->config);
> +			}
>  		} else {
>  			encoder->base.crtc = NULL;
>  		}
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a423256..c32429d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1376,6 +1376,14 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
>  		flags |= DRM_MODE_FLAG_NVSYNC;
>  
>  	pipe_config->adjusted_mode.flags |= flags;
> +	pipe_config->pixel_multiplier = 1;
> +
> +	if (is_cpu_edp(intel_dp)) {
> +		if ((I915_READ(DP_A) & DP_PLL_FREQ_MASK) == DP_PLL_FREQ_160MHZ)
> +			pipe_config->cpu_edp_link_rate = 162000;
> +		else
> +			pipe_config->cpu_edp_link_rate = 270000;
> +	}
>  }
>  
>  static void intel_disable_dp(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4802483..a7c9293 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -268,6 +268,9 @@ struct intel_crtc_config {
>  	/* FDI configuration, only valid if has_pch_encoder is set. */
>  	int fdi_lanes;
>  	struct intel_link_m_n fdi_m_n;
> +
> +	/* CPU eDP config */
> +	int cpu_edp_link_rate;
>  };
>  
>  struct intel_crtc {
> diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> index 91e9905..d020a9c 100644
> --- a/drivers/gpu/drm/i915/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/intel_dvo.c
> @@ -147,6 +147,7 @@ static void intel_dvo_get_config(struct intel_encoder *encoder,
>  		flags |= DRM_MODE_FLAG_NVSYNC;
>  
>  	pipe_config->adjusted_mode.flags |= flags;
> +	pipe_config->pixel_multiplier = 1;
>  }
>  
>  static void intel_disable_dvo(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 9091655..7b55a8a 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -678,6 +678,7 @@ static void intel_hdmi_get_config(struct intel_encoder *encoder,
>  		flags |= DRM_MODE_FLAG_NVSYNC;
>  
>  	pipe_config->adjusted_mode.flags |= flags;
> +	pipe_config->pixel_multiplier = 1;
>  }
>  
>  static void intel_enable_hdmi(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 6554860..f0cbb02 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -109,6 +109,7 @@ static void intel_lvds_get_config(struct intel_encoder *encoder,
>  		flags |= DRM_MODE_FLAG_PVSYNC;
>  
>  	pipe_config->adjusted_mode.flags |= flags;
> +	pipe_config->pixel_multiplier = 1;
>  }
>  
>  /* The LVDS pin pair needs to be on before the DPLLs are enabled.
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 416d23c..ee7f0ab 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -780,6 +780,11 @@ static bool intel_sdvo_set_clock_rate_mult(struct intel_sdvo *intel_sdvo, u8 val
>  	return intel_sdvo_set_value(intel_sdvo, SDVO_CMD_SET_CLOCK_RATE_MULT, &val, 1);
>  }
>  
> +static bool intel_sdvo_get_clock_rate_mult(struct intel_sdvo *intel_sdvo, u8 *val)
> +{
> +	return intel_sdvo_get_value(intel_sdvo, SDVO_CMD_GET_CLOCK_RATE_MULT, &val, 1);
> +}
> +
>  static void intel_sdvo_get_dtd_from_mode(struct intel_sdvo_dtd *dtd,
>  					 const struct drm_display_mode *mode)
>  {
> @@ -1315,6 +1320,7 @@ static void intel_sdvo_get_config(struct intel_encoder *encoder,
>  	struct intel_sdvo *intel_sdvo = to_intel_sdvo(&encoder->base);
>  	struct intel_sdvo_dtd dtd;
>  	u32 flags = 0;
> +	u8 val;
>  	bool ret;
>  
>  	ret = intel_sdvo_get_input_timing(intel_sdvo, &dtd);
> @@ -1334,6 +1340,24 @@ static void intel_sdvo_get_config(struct intel_encoder *encoder,
>  		flags |= DRM_MODE_FLAG_NVSYNC;
>  
>  	pipe_config->adjusted_mode.flags |= flags;
> +
> +	ret = intel_sdvo_get_clock_rate_mult(intel_sdvo, &val);
> +	if (!ret) {
> +		DRM_DEBUG_DRIVER("failed to retrieve clock rate multiplier\n");
> +		return;
> +	}
> +
> +	switch (val) {
> +	case SDVO_CLOCK_RATE_MULT_1X:
> +		pipe_config->pixel_multiplier = 1;
> +		break;
> +	case SDVO_CLOCK_RATE_MULT_2X:
> +		pipe_config->pixel_multiplier = 2;
> +		break;
> +	case SDVO_CLOCK_RATE_MULT_4X:
> +		pipe_config->pixel_multiplier = 4;
> +		break;
> +	}
>  }
>  
>  static void intel_disable_sdvo(struct intel_encoder *encoder)
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch