From: Daniel Vetter <daniel@ffwll.ch>
To: ville.syrjala@linux.intel.com
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 09/11] drm/i915: Fix port_clock and adjusted_mode.clock readout all over
Date: Sun, 8 Sep 2013 14:37:32 +0200 [thread overview]
Message-ID: <20130908123732.GA27291@phenom.ffwll.local> (raw)
In-Reply-To: <1378499348-4281-10-git-send-email-ville.syrjala@linux.intel.com>
On Fri, Sep 06, 2013 at 11:29:06PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@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.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 -
> drivers/gpu/drm/i915/intel_crt.c | 8 ++++++
> drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++++++++++++------------
> 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_sdvo.c | 8 ++++++
> 8 files changed, 72 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 769c138..09fc308 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -369,7 +369,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/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index ea9022e..be99cf2 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 d89ea94..26f0269 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5047,6 +5047,8 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
> DPLL_PORTB_READY_MASK);
> }
>
> + i9xx_crtc_clock_get(crtc, pipe_config);
> +
> return true;
> }
>
> @@ -6007,6 +6009,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_crtc_clock_get(crtc, pipe_config);
> } else {
> pipe_config->pixel_multiplier = 1;
> }
> @@ -7408,7 +7412,12 @@ static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
> }
> }
>
> - 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,
> @@ -7436,6 +7445,9 @@ static void ironlake_crtc_clock_get(struct intel_crtc *crtc,
> 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.
> @@ -7446,6 +7458,12 @@ static void ironlake_crtc_clock_get(struct intel_crtc *crtc,
> */
> link_freq = intel_fdi_link_freq(dev) * 10000;
>
> + /*
> + * 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.
> + */
> pipe_config->adjusted_mode.clock =
> intel_dotclock_calculate(link_freq, &pipe_config->fdi_m_n);
> }
> @@ -8859,9 +8877,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);
> @@ -8936,6 +8951,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)
> @@ -9887,7 +9914,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;
> @@ -9895,7 +9921,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;
> @@ -9903,7 +9928,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;
> @@ -10517,15 +10541,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 be0449b..509a9e4 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1372,6 +1372,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);
> @@ -1403,12 +1404,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 4cf8898..6e1204f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -805,5 +805,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 406303b..92487ce 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 4148cc8..56e6191 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 (pipe_config->pipe_bpp == 12*3)
I think you need to check for the 12BPC bit in the hdmi/sdvo reg here, you
can't rely on the pipe bpc value. At least on fdi configs we might end up
with sending a differnt pipe bpp (mayb just 10bpc) accross the wire than
what the port will output (if we aim for the 12bpc mode).
-Daniel
> + 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_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 74042c6..7148fdd 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1319,6 +1319,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;
> @@ -1357,6 +1358,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
>
> _______________________________________________
> 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
next prev parent reply other threads:[~2013-09-08 12:37 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-06 20:28 [PATCH 00/11] drm/i915: adjusted_mode.clock vs. port_clock v3 ville.syrjala
2013-09-06 20:28 ` [PATCH 01/11] drm/i915: Don't factor in pixel multplier when deriving dotclock from link clock and M/N values ville.syrjala
2013-09-06 20:28 ` [PATCH v2 02/11] drm/i915: Make adjusted_mode.clock non-pixel multiplied ville.syrjala
2013-09-13 11:40 ` Jani Nikula
2013-09-06 20:29 ` [PATCH v2 03/11] drm/i915: Add support for pipe_bpp readout ville.syrjala
2013-09-13 11:59 ` Jani Nikula
2013-09-06 20:29 ` [PATCH 04/11] drm/i915: Add state readout and checking for has_dp_encoder and dp_m_n ville.syrjala
2013-09-10 14:02 ` [PATCH v2] " ville.syrjala
2013-09-13 12:11 ` Jani Nikula
2013-09-06 20:29 ` [PATCH 05/11] drm/i915: Make intel_fuzzy_clock_check() take in arbitrary clocks ville.syrjala
2013-09-13 12:55 ` Daniel Vetter
2013-09-06 20:29 ` [PATCH 06/11] drm/i915: Add intel_dotclock_calculate() ville.syrjala
2013-09-13 12:30 ` Jani Nikula
2013-09-13 12:43 ` Ville Syrjälä
2013-09-13 12:59 ` [PATCH v2] " ville.syrjala
2013-09-16 11:14 ` Jani Nikula
2013-09-16 20:41 ` Daniel Vetter
2013-09-17 8:16 ` Ville Syrjälä
2013-09-06 20:29 ` [PATCH 07/11] drm/i915: Make i9xx_crtc_clock_get() use dpll_hw_state ville.syrjala
2013-09-13 12:40 ` Jani Nikula
2013-09-13 13:12 ` Ville Syrjälä
2013-09-13 13:18 ` [PATCH v2] " ville.syrjala
2013-09-13 13:44 ` Jani Nikula
2013-09-06 20:29 ` [PATCH 08/11] drm/i915: Make i9xx_crtc_clock_get() work for PCH DPLLs ville.syrjala
2013-09-08 12:35 ` Daniel Vetter
2013-09-09 11:06 ` [PATCH v2] " ville.syrjala
2013-09-13 13:04 ` Jani Nikula
2013-09-13 13:06 ` Ville Syrjälä
2013-09-13 13:47 ` Jani Nikula
2013-09-13 13:54 ` Ville Syrjälä
2013-09-16 20:43 ` Daniel Vetter
2013-09-06 20:29 ` [PATCH 09/11] drm/i915: Fix port_clock and adjusted_mode.clock readout all over ville.syrjala
2013-09-08 12:37 ` Daniel Vetter [this message]
2013-09-09 10:35 ` [PATCH v2] " ville.syrjala
2013-09-09 11:34 ` [PATCH v3] " ville.syrjala
2013-09-13 13:00 ` [PATCH v4] " ville.syrjala
2013-09-16 11:16 ` Jani Nikula
2013-09-06 20:29 ` [PATCH v2 10/11] drm/i915: Add PIPE_CONF_CHECK_CLOCK_FUZZY() ville.syrjala
2013-09-06 20:29 ` [PATCH v2 11/11] drm/i915: Add fuzzy clock check for port_clock ville.syrjala
2013-09-16 21:16 ` Daniel Vetter
2013-09-08 12:38 ` [PATCH 00/11] drm/i915: adjusted_mode.clock vs. port_clock v3 Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130908123732.GA27291@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=intel-gfx@lists.freedesktop.org \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.