[Intel-gfx] [PATCH v4] drm/i915: Fix port_clock and adjusted_mode.clock readout all over
Jani Nikula
jani.nikula at linux.intel.com
Mon Sep 16 13:16:44 CEST 2013
On Fri, 13 Sep 2013, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> Now that adjusted_mode.clock no longer contains the pixel_multiplier, we
> can kill the get_clock() callback and instead do the clock readout
> in get_pipe_config().
>
> Also i9xx_crtc_clock_get() can now extract the frequency of the PCH
> DPLL, so use it to populate port_clock accurately for PCH encoders.
> For DP in port A the encoder is still responsible for filling in
> port_clock. The FDI adjusted_mode.clock extraction is kept in place
> for some extra sanity checking, but we no longer need to pretend it's
> also the port_clock.
>
> In the encoder get_config() functions fill out adjusted_mode.clock
> based on port_clock and other details such as the DP M/N values,
> HDMI 12bpc and SDVO pixel_multiplier. For PCH encoders we will then
> do an extra sanity check to make sure the dotclock we derived from
> the FDI configuratiuon matches the one we derive from port_clock.
>
> DVO doesn't exist on PCH platforms, so it doesn't need to anything
> but assign adjusted_mode.clock=port_clock. And DDI is HSW only, so
> none of the changes apply there.
>
> v2: Use hdmi_reg color format to detect 12bpc HDMI case
> v3: Set adjusted_mode.clock for LVDS too
> v4: Rename ironlake_crtc_clock_get to ironlake_pch_clock_get,
> eliminate the useless link_freq variable.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
Daniel, I think the patch is right, but I feel it deserves another set
of eyeballs.
Reviewed-by: Jani Nikula <jani.nikula at intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 -
> drivers/gpu/drm/i915/i915_reg.h | 1 +
> drivers/gpu/drm/i915/intel_crt.c | 8 ++++
> drivers/gpu/drm/i915/intel_display.c | 74 ++++++++++++++++++------------------
> drivers/gpu/drm/i915/intel_dp.c | 11 +++++-
> drivers/gpu/drm/i915/intel_drv.h | 2 +
> drivers/gpu/drm/i915/intel_dvo.c | 2 +
> drivers/gpu/drm/i915/intel_hdmi.c | 11 ++++++
> drivers/gpu/drm/i915/intel_lvds.c | 8 ++++
> drivers/gpu/drm/i915/intel_sdvo.c | 8 ++++
> 10 files changed, 86 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7caf71d..8b16d47 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -371,7 +371,6 @@ 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/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index bcee89b..384adfb 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2071,6 +2071,7 @@
>
> /* Gen 4 SDVO/HDMI bits: */
> #define SDVO_COLOR_FORMAT_8bpc (0 << 26)
> +#define SDVO_COLOR_FORMAT_MASK (7 << 26)
> #define SDVO_ENCODING_SDVO (0 << 10)
> #define SDVO_ENCODING_HDMI (2 << 10)
> #define HDMI_MODE_SELECT_HDMI (1 << 9) /* HDMI only */
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index f5f89c3..6f101d5 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -89,6 +89,7 @@ static void intel_crt_get_config(struct intel_encoder *encoder,
> struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> struct intel_crt *crt = intel_encoder_to_crt(encoder);
> u32 tmp, flags = 0;
> + int dotclock;
>
> tmp = I915_READ(crt->adpa_reg);
>
> @@ -103,6 +104,13 @@ static void intel_crt_get_config(struct intel_encoder *encoder,
> flags |= DRM_MODE_FLAG_NVSYNC;
>
> pipe_config->adjusted_mode.flags |= flags;
> +
> + dotclock = pipe_config->port_clock;
> +
> + if (HAS_PCH_SPLIT(dev_priv->dev))
> + ironlake_check_encoder_dotclock(pipe_config, dotclock);
> +
> + pipe_config->adjusted_mode.clock = dotclock;
> }
>
> /* Note: The caller is required to filter out dpms modes not supported by the
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fed3804..42c7dc6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -47,8 +47,8 @@ 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);
> +static void ironlake_pch_clock_get(struct intel_crtc *crtc,
> + struct intel_crtc_config *pipe_config);
>
> static int intel_set_mode(struct drm_crtc *crtc, struct drm_display_mode *mode,
> int x, int y, struct drm_framebuffer *old_fb);
> @@ -5045,6 +5045,8 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
> DPLL_PORTB_READY_MASK);
> }
>
> + i9xx_crtc_clock_get(crtc, pipe_config);
> +
> return true;
> }
>
> @@ -6004,6 +6006,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
> pipe_config->pixel_multiplier =
> ((tmp & PLL_REF_SDVO_HDMI_MULTIPLIER_MASK)
> >> PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT) + 1;
> +
> + ironlake_pch_clock_get(crtc, pipe_config);
> } else {
> pipe_config->pixel_multiplier = 1;
> }
> @@ -7411,7 +7415,12 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
> i9xx_clock(refclk, &clock);
> }
>
> - pipe_config->adjusted_mode.clock = clock.dot;
> + /*
> + * This value includes pixel_multiplier. We will use
> + * port_clock to compute adjusted_mode.clock in the
> + * encoder's get_config() function.
> + */
> + pipe_config->port_clock = clock.dot;
> }
>
> int intel_dotclock_calculate(int link_freq,
> @@ -7433,31 +7442,23 @@ int intel_dotclock_calculate(int link_freq,
> return div_u64((u64)m_n->link_m * link_freq, m_n->link_n);
> }
>
> -static void ironlake_crtc_clock_get(struct intel_crtc *crtc,
> - struct intel_crtc_config *pipe_config)
> +static void ironlake_pch_clock_get(struct intel_crtc *crtc,
> + struct intel_crtc_config *pipe_config)
> {
> struct drm_device *dev = crtc->base.dev;
> - int link_freq;
> +
> + /* read out port_clock from the DPLL */
> + i9xx_crtc_clock_get(crtc, pipe_config);
>
> /*
> - * 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.
> + * This value does not include pixel_multiplier.
> + * We will check that port_clock and adjusted_mode.clock
> + * agree once we know their relationship in the encoder's
> + * get_config() function.
> */
> - if (pipe_config->has_pch_encoder) {
> - link_freq = intel_fdi_link_freq(dev) * 10000;
> -
> - pipe_config->adjusted_mode.clock =
> - intel_dotclock_calculate(link_freq, &pipe_config->fdi_m_n);
> - } else {
> - link_freq = pipe_config->port_clock;
> -
> - pipe_config->adjusted_mode.clock =
> - intel_dotclock_calculate(link_freq, &pipe_config->dp_m_n);
> - }
> + pipe_config->adjusted_mode.clock =
> + intel_dotclock_calculate(intel_fdi_link_freq(dev) * 10000,
> + &pipe_config->fdi_m_n);
> }
>
> /** Returns the currently programmed mode of the given pipe. */
> @@ -8873,9 +8874,6 @@ check_crtc_state(struct drm_device *dev)
> encoder->get_config(encoder, &pipe_config);
> }
>
> - if (dev_priv->display.get_clock)
> - dev_priv->display.get_clock(crtc, &pipe_config);
> -
> WARN(crtc->active != active,
> "crtc active state doesn't match with hw state "
> "(expected %i, found %i)\n", crtc->active, active);
> @@ -8950,6 +8948,18 @@ intel_modeset_check_state(struct drm_device *dev)
> check_shared_dpll_state(dev);
> }
>
> +void ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config,
> + int dotclock)
> +{
> + /*
> + * FDI already provided one idea for the dotclock.
> + * Yell if the encoder disagrees.
> + */
> + WARN(!intel_fuzzy_clock_check(pipe_config->adjusted_mode.clock, dotclock),
> + "FDI dotclock and encoder dotclock mismatch, fdi: %i, encoder: %i\n",
> + pipe_config->adjusted_mode.clock, dotclock);
> +}
> +
> static int __intel_set_mode(struct drm_crtc *crtc,
> struct drm_display_mode *mode,
> int x, int y, struct drm_framebuffer *fb)
> @@ -9901,7 +9911,6 @@ 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;
> @@ -9909,7 +9918,6 @@ 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;
> @@ -9917,7 +9925,6 @@ 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;
> @@ -10536,15 +10543,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> pipe);
> }
>
> - list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> - base.head) {
> - if (!crtc->active)
> - continue;
> - if (dev_priv->display.get_clock)
> - dev_priv->display.get_clock(crtc,
> - &crtc->config);
> - }
> -
> list_for_each_entry(connector, &dev->mode_config.connector_list,
> base.head) {
> if (connector->get_hw_state(connector)) {
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 9ba697c..a7fa6fd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1417,6 +1417,7 @@ 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;
>
> if ((port == PORT_A) || !HAS_PCH_CPT(dev)) {
> tmp = I915_READ(intel_dp->output_reg);
> @@ -1448,12 +1449,20 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
>
> intel_dp_get_m_n(crtc, pipe_config);
>
> - if (dp_to_dig_port(intel_dp)->port == PORT_A) {
> + if (port == PORT_A) {
> if ((I915_READ(DP_A) & DP_PLL_FREQ_MASK) == DP_PLL_FREQ_160MHZ)
> pipe_config->port_clock = 162000;
> else
> 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->adjusted_mode.clock = dotclock;
> }
>
> static bool is_edp_psr(struct intel_dp *intel_dp)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6b97ac1..338222c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -807,5 +807,7 @@ extern void intel_dp_get_m_n(struct intel_crtc *crtc,
> struct intel_crtc_config *pipe_config);
> extern int intel_dotclock_calculate(int link_freq,
> const struct intel_link_m_n *m_n);
> +extern void ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config,
> + int dotclock);
>
> #endif /* __INTEL_DRV_H__ */
> diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> index 55cec38..ff86c36 100644
> --- a/drivers/gpu/drm/i915/intel_dvo.c
> +++ b/drivers/gpu/drm/i915/intel_dvo.c
> @@ -153,6 +153,8 @@ static void intel_dvo_get_config(struct intel_encoder *encoder,
> flags |= DRM_MODE_FLAG_NVSYNC;
>
> pipe_config->adjusted_mode.flags |= flags;
> +
> + pipe_config->adjusted_mode.clock = pipe_config->port_clock;
> }
>
> 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 70c716e..17b2d7e 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -713,6 +713,7 @@ static void intel_hdmi_get_config(struct intel_encoder *encoder,
> struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> u32 tmp, flags = 0;
> + int dotclock;
>
> tmp = I915_READ(intel_hdmi->hdmi_reg);
>
> @@ -727,6 +728,16 @@ static void intel_hdmi_get_config(struct intel_encoder *encoder,
> flags |= DRM_MODE_FLAG_NVSYNC;
>
> pipe_config->adjusted_mode.flags |= flags;
> +
> + if ((tmp & SDVO_COLOR_FORMAT_MASK) == HDMI_COLOR_FORMAT_12bpc)
> + dotclock = pipe_config->port_clock * 2 / 3;
> + else
> + dotclock = pipe_config->port_clock;
> +
> + if (HAS_PCH_SPLIT(dev_priv->dev))
> + ironlake_check_encoder_dotclock(pipe_config, dotclock);
> +
> + pipe_config->adjusted_mode.clock = dotclock;
> }
>
> 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 831a5c0..05e5485 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -92,6 +92,7 @@ static void intel_lvds_get_config(struct intel_encoder *encoder,
> struct drm_device *dev = encoder->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> u32 lvds_reg, tmp, flags = 0;
> + int dotclock;
>
> if (HAS_PCH_SPLIT(dev))
> lvds_reg = PCH_LVDS;
> @@ -116,6 +117,13 @@ 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->adjusted_mode.clock = dotclock;
> }
>
> /* 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 ebfd513..91aea9e 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1325,6 +1325,7 @@ static void intel_sdvo_get_config(struct intel_encoder *encoder,
> struct intel_sdvo *intel_sdvo = to_sdvo(encoder);
> struct intel_sdvo_dtd dtd;
> int encoder_pixel_multiplier = 0;
> + int dotclock;
> u32 flags = 0, sdvox;
> u8 val;
> bool ret;
> @@ -1363,6 +1364,13 @@ static void intel_sdvo_get_config(struct intel_encoder *encoder,
> >> SDVO_PORT_MULTIPLY_SHIFT) + 1;
> }
>
> + dotclock = pipe_config->port_clock / pipe_config->pixel_multiplier;
> +
> + if (HAS_PCH_SPLIT(dev))
> + ironlake_check_encoder_dotclock(pipe_config, dotclock);
> +
> + pipe_config->adjusted_mode.clock = dotclock;
> +
> /* Cross check the port pixel multiplier with the sdvo encoder state. */
> if (intel_sdvo_get_value(intel_sdvo, SDVO_CMD_GET_CLOCK_RATE_MULT,
> &val, 1)) {
> --
> 1.8.1.5
>
--
Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list