On Wed, Sep 12, 2012 at 10:06:33AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>
> The set_m_n code was spread all over the mode_set function.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 152 ++++++++++++++++++++--------------
> 1 file changed, 89 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b657416..8d3b9d6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4735,6 +4735,92 @@ static void ironlake_set_pipe_timings(struct intel_crtc *intel_crtc,
> ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
> }
>
> +static void ironlake_set_m_n(struct drm_crtc *crtc,
> + struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode)
> +{
> + struct drm_device *dev = crtc->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + enum pipe pipe = intel_crtc->pipe;
> + struct intel_encoder *intel_encoder, *edp_encoder = NULL;
> + struct fdi_m_n m_n = {0};
> + int target_clock, pixel_multiplier, lane, link_bw;
> + bool is_dp = false, is_cpu_edp = false;
> +
> + for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
> + switch (intel_encoder->type) {
> + case INTEL_OUTPUT_DISPLAYPORT:
> + is_dp = true;
> + break;
> + case INTEL_OUTPUT_EDP:
> + is_dp = true;
> + if (!intel_encoder_is_pch_edp(&intel_encoder->base))
> + is_cpu_edp = true;
> + edp_encoder = intel_encoder;
> + break;
> + }
> + }
> +
> + /* FDI link */
> + pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
> + lane = 0;
> + /* CPU eDP doesn't require FDI link, so just set DP M/N
> + according to current link config */
> + if (is_cpu_edp) {
> + intel_edp_link_config(edp_encoder, &lane, &link_bw);
> + } else {
> + /* FDI is a binary signal running at ~2.7GHz, encoding
> + * each output octet as 10 bits. The actual frequency
> + * is stored as a divider into a 100MHz clock, and the
> + * mode pixel clock is stored in units of 1KHz.
> + * 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;
> + }
> +
> + /* [e]DP over FDI requires target mode clock instead of link clock. */
> + if (edp_encoder)
> + target_clock = intel_edp_target_clock(edp_encoder, mode);
> + else if (is_dp)
> + target_clock = mode->clock;
> + else
> + target_clock = adjusted_mode->clock;
> +
> + if (!lane) {
> + /*
> + * Account for spread spectrum to avoid
> + * oversubscribing the link. Max center spread
> + * is 2.5%; use 5% for safety's sake.
> + */
> + u32 bps = target_clock * intel_crtc->bpp * 21 / 20;
> + lane = bps / (link_bw * 8) + 1;
> + }
> +
> + intel_crtc->fdi_lanes = lane;
> +
> + if (pixel_multiplier > 1)
> + link_bw *= pixel_multiplier;
> +
> + ironlake_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw,
> + &m_n);
> +
> + if (is_dp && !is_cpu_edp) {
> + intel_dp_set_m_n(crtc, mode, adjusted_mode);
> + } else {
> + /* For non-DP output, clear any trans DP clock recovery setting.*/
> + I915_WRITE(TRANSDATA_M1(pipe), 0);
> + I915_WRITE(TRANSDATA_N1(pipe), 0);
> + I915_WRITE(TRANSDPLINK_M1(pipe), 0);
> + I915_WRITE(TRANSDPLINK_N1(pipe), 0);
> + }
The pch DP transcoder m/n values are imo encoder-specific state - I'm
toying around with ideas to shovel this into encoder callbacks, similarly
to the lvds port enabling. Hence ...
[pls scroll down.]
> + I915_WRITE(PIPE_DATA_M1(pipe), TU_SIZE(m_n.tu) | m_n.gmch_m);
> + I915_WRITE(PIPE_DATA_N1(pipe), m_n.gmch_n);
> + I915_WRITE(PIPE_LINK_M1(pipe), m_n.link_m);
> + I915_WRITE(PIPE_LINK_N1(pipe), m_n.link_n);
> +}
> +
> static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> struct drm_display_mode *mode,
> struct drm_display_mode *adjusted_mode,
> @@ -4751,11 +4837,9 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> u32 dpll, fp = 0, fp2 = 0;
> bool ok, has_reduced_clock = false, is_sdvo = false;
> bool is_crt = false, is_lvds = false, is_tv = false, is_dp = false;
> - struct intel_encoder *encoder, *edp_encoder = NULL;
> + struct intel_encoder *encoder;
> const intel_limit_t *limit;
> - int ret;
> - struct fdi_m_n m_n = {0};
> - int target_clock, pixel_multiplier, lane, link_bw, factor;
> + int ret, factor;
> unsigned int pipe_bpp;
> bool dither;
> bool is_cpu_edp = false, is_pch_edp = false;
> @@ -4786,7 +4870,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> is_pch_edp = true;
> else
> is_cpu_edp = true;
> - edp_encoder = encoder;
> break;
> }
>
> @@ -4828,33 +4911,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> if (is_sdvo && is_tv)
> i9xx_adjust_sdvo_tv_clock(adjusted_mode, &clock);
>
> -
> - /* FDI link */
> - pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
> - lane = 0;
> - /* CPU eDP doesn't require FDI link, so just set DP M/N
> - according to current link config */
> - if (is_cpu_edp) {
> - intel_edp_link_config(edp_encoder, &lane, &link_bw);
> - } else {
> - /* FDI is a binary signal running at ~2.7GHz, encoding
> - * each output octet as 10 bits. The actual frequency
> - * is stored as a divider into a 100MHz clock, and the
> - * mode pixel clock is stored in units of 1KHz.
> - * 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;
> - }
> -
> - /* [e]DP over FDI requires target mode clock instead of link clock. */
> - if (edp_encoder)
> - target_clock = intel_edp_target_clock(edp_encoder, mode);
> - else if (is_dp)
> - target_clock = mode->clock;
> - else
> - target_clock = adjusted_mode->clock;
> -
> /* determine panel color depth */
> dither = intel_choose_pipe_bpp_dither(crtc, fb, &pipe_bpp, mode);
> if (is_lvds && dev_priv->lvds_dither)
> @@ -4868,23 +4924,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> }
> intel_crtc->bpp = pipe_bpp;
>
> - if (!lane) {
> - /*
> - * Account for spread spectrum to avoid
> - * oversubscribing the link. Max center spread
> - * is 2.5%; use 5% for safety's sake.
> - */
> - u32 bps = target_clock * intel_crtc->bpp * 21 / 20;
> - lane = bps / (link_bw * 8) + 1;
> - }
> -
> - intel_crtc->fdi_lanes = lane;
> -
> - if (pixel_multiplier > 1)
> - link_bw *= pixel_multiplier;
> - ironlake_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw,
> - &m_n);
> -
> fp = clock.n << 16 | clock.m1 << 8 | clock.m2;
> if (has_reduced_clock)
> fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
> @@ -4977,15 +5016,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> if (is_lvds)
> ironlake_lvds_port_enable(intel_crtc, adjusted_mode, clock.p2);
>
> - if (is_dp && !is_cpu_edp) {
> - intel_dp_set_m_n(crtc, mode, adjusted_mode);
> - } else {
> - /* For non-DP output, clear any trans DP clock recovery setting.*/
> - I915_WRITE(TRANSDATA_M1(pipe), 0);
> - I915_WRITE(TRANSDATA_N1(pipe), 0);
> - I915_WRITE(TRANSDPLINK_M1(pipe), 0);
> - I915_WRITE(TRANSDPLINK_N1(pipe), 0);
> - }
> + ironlake_set_m_n(crtc, mode, adjusted_mode);
... I think we should leave this here for now ...
>
> if (intel_crtc->pch_pll) {
> I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
> @@ -5014,11 +5045,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>
> ironlake_set_pipe_timings(intel_crtc, mode, adjusted_mode);
>
> - I915_WRITE(PIPE_DATA_M1(pipe), TU_SIZE(m_n.tu) | m_n.gmch_m);
> - I915_WRITE(PIPE_DATA_N1(pipe), m_n.gmch_n);
> - I915_WRITE(PIPE_LINK_M1(pipe), m_n.link_m);
> - I915_WRITE(PIPE_LINK_N1(pipe), m_n.link_n);
... and instead move the new ironlake_set_m_n call to this place - that
way we don't change anything in the register write sequence, too. And
somehow changing the register write sequence bites us every time we try :(
Also, I'm hunting a strange dp dpms issue on ilk/gm45 currently and I
suspect (without any evidence). So I prefer we're extra careful around dp
state for now.
Otherwise this looks nice.
-Daniel
> -
> if (is_cpu_edp)
> ironlake_set_pll_edp(crtc, adjusted_mode->clock);
>
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at 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
