[PATCH 0/5] Rework ironlake_crtc_mode_set V2

[PATCH 0/5] Rework ironlake_crtc_mode_set V2

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Hi

Here are the missing patches that received review during the last week.

 - Patch 0001 is the follow up requested by Daniel on his reply to the previous
   patch 0001.
 - Patch 0002 is just a new version with the comments from Daniel applied.
 - Patch 0003 is new, but we can consider it as a sub-set of the previous patch
   0007 (extract pch_pll_set from ironlake_crtc_mode_set). This should keep the
   order things are executed, while still moving a significant amount of code
   away from ironlake_crtc_mode_set.
 - Patch 0004 is just like previous patch 0008, but rebased.
 - Patch 0005 is a new version of previous patch 0002, but now has the potential
   to do some damage on i9xx, so it is the last one. I don't have i9xx machines
   to test this, so I think we need to gather a few tested-by stamps on it. We
   can also give up breaking i9xx and apply the original version which only
   touches ironlake.

Thanks,
Paulo

Paulo Zanoni (5):
  drm/i915: don't recheck for invalid pipe bpp
  drm/i915: extract set_m_n from ironlake_crtc_mode_set
  drm/i915: extract compute_dpll from ironlake_crtc_mode_set
  drm/i915: remove unused variables from ironlake_crtc_mode_set
  drm/i915: extract intel_set_pipe_timings from crtc_mode_set

 drivers/gpu/drm/i915/intel_display.c |  335 ++++++++++++++++++----------------
 1 file changed, 180 insertions(+), 155 deletions(-)

-- 
1.7.10.4

[PATCH 1/5] drm/i915: don't recheck for invalid pipe bpp

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

As noticed by Daniel Vetter, intel_pipe_choose_bpp_dither should
already check for invalid bpp values and set a valid value, so remove
the recheck inside ironlake_crtc_mode_set and also replace a "default"
switch case inside ironlake_set_pipeconf with a BUG().

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 947c97d..eb8248b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4624,8 +4624,8 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
 		val |= PIPE_12BPC;
 		break;
 	default:
-		val |= PIPE_8BPC;
-		break;
+		/* Case prevented by intel_choose_pipe_bpp_dither. */
+		BUG();
 	}
 
 	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
@@ -4726,7 +4726,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	struct fdi_m_n m_n = {0};
 	u32 temp;
 	int target_clock, pixel_multiplier, lane, link_bw, factor;
-	unsigned int pipe_bpp;
 	bool dither;
 	bool is_cpu_edp = false, is_pch_edp = false;
 
@@ -4800,18 +4799,10 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		target_clock = adjusted_mode->clock;
 
 	/* determine panel color depth */
-	dither = intel_choose_pipe_bpp_dither(crtc, fb, &pipe_bpp, mode);
+	dither = intel_choose_pipe_bpp_dither(crtc, fb, &intel_crtc->bpp, mode);
 	if (is_lvds && dev_priv->lvds_dither)
 		dither = true;
 
-	if (pipe_bpp != 18 && pipe_bpp != 24 && pipe_bpp != 30 &&
-	    pipe_bpp != 36) {
-		WARN(1, "intel_choose_pipe_bpp returned invalid value %d\n",
-		     pipe_bpp);
-		pipe_bpp = 24;
-	}
-	intel_crtc->bpp = pipe_bpp;
-
 	if (!lane) {
 		/*
 		 * Account for spread spectrum to avoid
-- 
1.7.10.4

Re: [PATCH 1/5] drm/i915: don't recheck for invalid pipe bpp

[Rodrigo Vivi]

Feel free to use:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Thu, Sep 20, 2012 at 6:36 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> As noticed by Daniel Vetter, intel_pipe_choose_bpp_dither should
> already check for invalid bpp values and set a valid value, so remove
> the recheck inside ironlake_crtc_mode_set and also replace a "default"
> switch case inside ironlake_set_pipeconf with a BUG().
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 947c97d..eb8248b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4624,8 +4624,8 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
>                 val |= PIPE_12BPC;
>                 break;
>         default:
> -               val |= PIPE_8BPC;
> -               break;
> +               /* Case prevented by intel_choose_pipe_bpp_dither. */
> +               BUG();
>         }
>
>         val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
> @@ -4726,7 +4726,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>         struct fdi_m_n m_n = {0};
>         u32 temp;
>         int target_clock, pixel_multiplier, lane, link_bw, factor;
> -       unsigned int pipe_bpp;
>         bool dither;
>         bool is_cpu_edp = false, is_pch_edp = false;
>
> @@ -4800,18 +4799,10 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>                 target_clock = adjusted_mode->clock;
>
>         /* determine panel color depth */
> -       dither = intel_choose_pipe_bpp_dither(crtc, fb, &pipe_bpp, mode);
> +       dither = intel_choose_pipe_bpp_dither(crtc, fb, &intel_crtc->bpp, mode);
>         if (is_lvds && dev_priv->lvds_dither)
>                 dither = true;
>
> -       if (pipe_bpp != 18 && pipe_bpp != 24 && pipe_bpp != 30 &&
> -           pipe_bpp != 36) {
> -               WARN(1, "intel_choose_pipe_bpp returned invalid value %d\n",
> -                    pipe_bpp);
> -               pipe_bpp = 24;
> -       }
> -       intel_crtc->bpp = pipe_bpp;
> -
>         if (!lane) {
>                 /*
>                  * Account for spread spectrum to avoid
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[PATCH 2/5] drm/i915: extract set_m_n from ironlake_crtc_mode_set

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The set_m_n code was spread all over the mode_set function.

Version 2:
Don't set the DP M/N registers on ironlake_set_m_n. Daniel Vetter has
plans to add some encoder-specific callbacks. Also, on this version we
don't change the order we're writing the registers, making the code
change safer.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  131 ++++++++++++++++++++--------------
 1 file changed, 79 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index eb8248b..ebffd99 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4705,6 +4705,82 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
 	return true;
 }
 
+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);
+
+	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,
@@ -4721,11 +4797,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;
-	int ret;
-	struct fdi_m_n m_n = {0};
+	struct intel_encoder *encoder;
 	u32 temp;
-	int target_clock, pixel_multiplier, lane, link_bw, factor;
+	int ret, factor;
 	bool dither;
 	bool is_cpu_edp = false, is_pch_edp = false;
 
@@ -4755,7 +4829,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 				is_pch_edp = true;
 			else
 				is_cpu_edp = true;
-			edp_encoder = encoder;
 			break;
 		}
 
@@ -4772,54 +4845,11 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	/* Ensure that the cursor is valid for the new mode before changing... */
 	intel_crtc_update_cursor(crtc, true);
 
-	/* 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, &intel_crtc->bpp, mode);
 	if (is_lvds && dev_priv->lvds_dither)
 		dither = true;
 
-	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 |
@@ -5016,10 +5046,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	I915_WRITE(PIPESRC(pipe),
 		   ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
 
-	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);
+	ironlake_set_m_n(crtc, mode, adjusted_mode);
 
 	if (is_cpu_edp)
 		ironlake_set_pll_edp(crtc, adjusted_mode->clock);
-- 
1.7.10.4

Re: [PATCH 2/5] drm/i915: extract set_m_n from ironlake_crtc_mode_set

[Rodrigo Vivi]

Feel free to use:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Thu, Sep 20, 2012 at 6:36 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> The set_m_n code was spread all over the mode_set function.
>
> Version 2:
> Don't set the DP M/N registers on ironlake_set_m_n. Daniel Vetter has
> plans to add some encoder-specific callbacks. Also, on this version we
> don't change the order we're writing the registers, making the code
> change safer.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  131 ++++++++++++++++++++--------------
>  1 file changed, 79 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index eb8248b..ebffd99 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4705,6 +4705,82 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
>         return true;
>  }
>
> +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);
> +
> +       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,
> @@ -4721,11 +4797,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;
> -       int ret;
> -       struct fdi_m_n m_n = {0};
> +       struct intel_encoder *encoder;
>         u32 temp;
> -       int target_clock, pixel_multiplier, lane, link_bw, factor;
> +       int ret, factor;
>         bool dither;
>         bool is_cpu_edp = false, is_pch_edp = false;
>
> @@ -4755,7 +4829,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>                                 is_pch_edp = true;
>                         else
>                                 is_cpu_edp = true;
> -                       edp_encoder = encoder;
>                         break;
>                 }
>
> @@ -4772,54 +4845,11 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>         /* Ensure that the cursor is valid for the new mode before changing... */
>         intel_crtc_update_cursor(crtc, true);
>
> -       /* 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, &intel_crtc->bpp, mode);
>         if (is_lvds && dev_priv->lvds_dither)
>                 dither = true;
>
> -       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 |
> @@ -5016,10 +5046,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>         I915_WRITE(PIPESRC(pipe),
>                    ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
>
> -       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);
> +       ironlake_set_m_n(crtc, mode, adjusted_mode);
>
>         if (is_cpu_edp)
>                 ironlake_set_pll_edp(crtc, adjusted_mode->clock);
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[PATCH 3/5] drm/i915: extract compute_dpll from ironlake_crtc_mode_set

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Too many lines just to compute the value of a single variable, so
move this to its own function.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  165 +++++++++++++++++++++-------------
 1 file changed, 105 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ebffd99..5fb082f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4781,6 +4781,109 @@ static void ironlake_set_m_n(struct drm_crtc *crtc,
 	I915_WRITE(PIPE_LINK_N1(pipe), m_n.link_n);
 }
 
+static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
+				      struct drm_display_mode *adjusted_mode,
+				      intel_clock_t *clock, u32 fp)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_encoder *intel_encoder;
+	uint32_t dpll;
+	int factor, pixel_multiplier, num_connectors = 0;
+	bool is_lvds = false, is_sdvo = false, is_tv = false;
+	bool is_dp = false, is_cpu_edp = false;
+
+	for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
+		switch (intel_encoder->type) {
+		case INTEL_OUTPUT_LVDS:
+			is_lvds = true;
+			break;
+		case INTEL_OUTPUT_SDVO:
+		case INTEL_OUTPUT_HDMI:
+			is_sdvo = true;
+			if (intel_encoder->needs_tv_clock)
+				is_tv = true;
+			break;
+		case INTEL_OUTPUT_TVOUT:
+			is_tv = true;
+			break;
+		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;
+			break;
+		}
+
+		num_connectors++;
+	}
+
+	/* Enable autotuning of the PLL clock (if permissible) */
+	factor = 21;
+	if (is_lvds) {
+		if ((intel_panel_use_ssc(dev_priv) &&
+		     dev_priv->lvds_ssc_freq == 100) ||
+		    (I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP)
+			factor = 25;
+	} else if (is_sdvo && is_tv)
+		factor = 20;
+
+	if (clock->m < factor * clock->n)
+		fp |= FP_CB_TUNE;
+
+	dpll = 0;
+
+	if (is_lvds)
+		dpll |= DPLLB_MODE_LVDS;
+	else
+		dpll |= DPLLB_MODE_DAC_SERIAL;
+	if (is_sdvo) {
+		pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
+		if (pixel_multiplier > 1) {
+			dpll |= (pixel_multiplier - 1) << PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
+		}
+		dpll |= DPLL_DVO_HIGH_SPEED;
+	}
+	if (is_dp && !is_cpu_edp)
+		dpll |= DPLL_DVO_HIGH_SPEED;
+
+	/* compute bitmask from p1 value */
+	dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
+	/* also FPA1 */
+	dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
+
+	switch (clock->p2) {
+	case 5:
+		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5;
+		break;
+	case 7:
+		dpll |= DPLLB_LVDS_P2_CLOCK_DIV_7;
+		break;
+	case 10:
+		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_10;
+		break;
+	case 14:
+		dpll |= DPLLB_LVDS_P2_CLOCK_DIV_14;
+		break;
+	}
+
+	if (is_sdvo && is_tv)
+		dpll |= PLL_REF_INPUT_TVCLKINBC;
+	else if (is_tv)
+		/* XXX: just matching BIOS for now */
+		/*	dpll |= PLL_REF_INPUT_TVCLKINBC; */
+		dpll |= 3;
+	else if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2)
+		dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
+	else
+		dpll |= PLL_REF_INPUT_DREFCLK;
+
+	return dpll;
+}
+
 static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 				  struct drm_display_mode *mode,
 				  struct drm_display_mode *adjusted_mode,
@@ -4799,7 +4902,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	bool is_crt = false, is_lvds = false, is_tv = false, is_dp = false;
 	struct intel_encoder *encoder;
 	u32 temp;
-	int ret, factor;
+	int ret;
 	bool dither;
 	bool is_cpu_edp = false, is_pch_edp = false;
 
@@ -4855,65 +4958,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
 			reduced_clock.m2;
 
-	/* Enable autotuning of the PLL clock (if permissible) */
-	factor = 21;
-	if (is_lvds) {
-		if ((intel_panel_use_ssc(dev_priv) &&
-		     dev_priv->lvds_ssc_freq == 100) ||
-		    (I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP)
-			factor = 25;
-	} else if (is_sdvo && is_tv)
-		factor = 20;
-
-	if (clock.m < factor * clock.n)
-		fp |= FP_CB_TUNE;
-
-	dpll = 0;
-
-	if (is_lvds)
-		dpll |= DPLLB_MODE_LVDS;
-	else
-		dpll |= DPLLB_MODE_DAC_SERIAL;
-	if (is_sdvo) {
-		int pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
-		if (pixel_multiplier > 1) {
-			dpll |= (pixel_multiplier - 1) << PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
-		}
-		dpll |= DPLL_DVO_HIGH_SPEED;
-	}
-	if (is_dp && !is_cpu_edp)
-		dpll |= DPLL_DVO_HIGH_SPEED;
-
-	/* compute bitmask from p1 value */
-	dpll |= (1 << (clock.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
-	/* also FPA1 */
-	dpll |= (1 << (clock.p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
-
-	switch (clock.p2) {
-	case 5:
-		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5;
-		break;
-	case 7:
-		dpll |= DPLLB_LVDS_P2_CLOCK_DIV_7;
-		break;
-	case 10:
-		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_10;
-		break;
-	case 14:
-		dpll |= DPLLB_LVDS_P2_CLOCK_DIV_14;
-		break;
-	}
-
-	if (is_sdvo && is_tv)
-		dpll |= PLL_REF_INPUT_TVCLKINBC;
-	else if (is_tv)
-		/* XXX: just matching BIOS for now */
-		/*	dpll |= PLL_REF_INPUT_TVCLKINBC; */
-		dpll |= 3;
-	else if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2)
-		dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
-	else
-		dpll |= PLL_REF_INPUT_DREFCLK;
+	dpll = ironlake_compute_dpll(intel_crtc, adjusted_mode, &clock, fp);
 
 	DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
 	drm_mode_debug_printmodeline(mode);
-- 
1.7.10.4

Re: [PATCH 3/5] drm/i915: extract compute_dpll from ironlake_crtc_mode_set

[Rodrigo Vivi]

Feel free to use:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Thu, Sep 20, 2012 at 6:36 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Too many lines just to compute the value of a single variable, so
> move this to its own function.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  165 +++++++++++++++++++++-------------
>  1 file changed, 105 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ebffd99..5fb082f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4781,6 +4781,109 @@ static void ironlake_set_m_n(struct drm_crtc *crtc,
>         I915_WRITE(PIPE_LINK_N1(pipe), m_n.link_n);
>  }
>
> +static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
> +                                     struct drm_display_mode *adjusted_mode,
> +                                     intel_clock_t *clock, u32 fp)
> +{
> +       struct drm_crtc *crtc = &intel_crtc->base;
> +       struct drm_device *dev = crtc->dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct intel_encoder *intel_encoder;
> +       uint32_t dpll;
> +       int factor, pixel_multiplier, num_connectors = 0;
> +       bool is_lvds = false, is_sdvo = false, is_tv = false;
> +       bool is_dp = false, is_cpu_edp = false;
> +
> +       for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
> +               switch (intel_encoder->type) {
> +               case INTEL_OUTPUT_LVDS:
> +                       is_lvds = true;
> +                       break;
> +               case INTEL_OUTPUT_SDVO:
> +               case INTEL_OUTPUT_HDMI:
> +                       is_sdvo = true;
> +                       if (intel_encoder->needs_tv_clock)
> +                               is_tv = true;
> +                       break;
> +               case INTEL_OUTPUT_TVOUT:
> +                       is_tv = true;
> +                       break;
> +               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;
> +                       break;
> +               }
> +
> +               num_connectors++;
> +       }
> +
> +       /* Enable autotuning of the PLL clock (if permissible) */
> +       factor = 21;
> +       if (is_lvds) {
> +               if ((intel_panel_use_ssc(dev_priv) &&
> +                    dev_priv->lvds_ssc_freq == 100) ||
> +                   (I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP)
> +                       factor = 25;
> +       } else if (is_sdvo && is_tv)
> +               factor = 20;
> +
> +       if (clock->m < factor * clock->n)
> +               fp |= FP_CB_TUNE;
> +
> +       dpll = 0;
> +
> +       if (is_lvds)
> +               dpll |= DPLLB_MODE_LVDS;
> +       else
> +               dpll |= DPLLB_MODE_DAC_SERIAL;
> +       if (is_sdvo) {
> +               pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
> +               if (pixel_multiplier > 1) {
> +                       dpll |= (pixel_multiplier - 1) << PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
> +               }
> +               dpll |= DPLL_DVO_HIGH_SPEED;
> +       }
> +       if (is_dp && !is_cpu_edp)
> +               dpll |= DPLL_DVO_HIGH_SPEED;
> +
> +       /* compute bitmask from p1 value */
> +       dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> +       /* also FPA1 */
> +       dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
> +
> +       switch (clock->p2) {
> +       case 5:
> +               dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5;
> +               break;
> +       case 7:
> +               dpll |= DPLLB_LVDS_P2_CLOCK_DIV_7;
> +               break;
> +       case 10:
> +               dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_10;
> +               break;
> +       case 14:
> +               dpll |= DPLLB_LVDS_P2_CLOCK_DIV_14;
> +               break;
> +       }
> +
> +       if (is_sdvo && is_tv)
> +               dpll |= PLL_REF_INPUT_TVCLKINBC;
> +       else if (is_tv)
> +               /* XXX: just matching BIOS for now */
> +               /*      dpll |= PLL_REF_INPUT_TVCLKINBC; */
> +               dpll |= 3;
> +       else if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2)
> +               dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
> +       else
> +               dpll |= PLL_REF_INPUT_DREFCLK;
> +
> +       return dpll;
> +}
> +
>  static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>                                   struct drm_display_mode *mode,
>                                   struct drm_display_mode *adjusted_mode,
> @@ -4799,7 +4902,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>         bool is_crt = false, is_lvds = false, is_tv = false, is_dp = false;
>         struct intel_encoder *encoder;
>         u32 temp;
> -       int ret, factor;
> +       int ret;
>         bool dither;
>         bool is_cpu_edp = false, is_pch_edp = false;
>
> @@ -4855,65 +4958,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>                 fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
>                         reduced_clock.m2;
>
> -       /* Enable autotuning of the PLL clock (if permissible) */
> -       factor = 21;
> -       if (is_lvds) {
> -               if ((intel_panel_use_ssc(dev_priv) &&
> -                    dev_priv->lvds_ssc_freq == 100) ||
> -                   (I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP)
> -                       factor = 25;
> -       } else if (is_sdvo && is_tv)
> -               factor = 20;
> -
> -       if (clock.m < factor * clock.n)
> -               fp |= FP_CB_TUNE;
> -
> -       dpll = 0;
> -
> -       if (is_lvds)
> -               dpll |= DPLLB_MODE_LVDS;
> -       else
> -               dpll |= DPLLB_MODE_DAC_SERIAL;
> -       if (is_sdvo) {
> -               int pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
> -               if (pixel_multiplier > 1) {
> -                       dpll |= (pixel_multiplier - 1) << PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
> -               }
> -               dpll |= DPLL_DVO_HIGH_SPEED;
> -       }
> -       if (is_dp && !is_cpu_edp)
> -               dpll |= DPLL_DVO_HIGH_SPEED;
> -
> -       /* compute bitmask from p1 value */
> -       dpll |= (1 << (clock.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> -       /* also FPA1 */
> -       dpll |= (1 << (clock.p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
> -
> -       switch (clock.p2) {
> -       case 5:
> -               dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5;
> -               break;
> -       case 7:
> -               dpll |= DPLLB_LVDS_P2_CLOCK_DIV_7;
> -               break;
> -       case 10:
> -               dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_10;
> -               break;
> -       case 14:
> -               dpll |= DPLLB_LVDS_P2_CLOCK_DIV_14;
> -               break;
> -       }
> -
> -       if (is_sdvo && is_tv)
> -               dpll |= PLL_REF_INPUT_TVCLKINBC;
> -       else if (is_tv)
> -               /* XXX: just matching BIOS for now */
> -               /*      dpll |= PLL_REF_INPUT_TVCLKINBC; */
> -               dpll |= 3;
> -       else if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2)
> -               dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
> -       else
> -               dpll |= PLL_REF_INPUT_DREFCLK;
> +       dpll = ironlake_compute_dpll(intel_crtc, adjusted_mode, &clock, fp);
>
>         DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
>         drm_mode_debug_printmodeline(mode);
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[PATCH 4/5] drm/i915: remove unused variables from ironlake_crtc_mode_set

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The last patches moved a lot of code from ironlake_crtc_mode_set to
sub-functions, so these variables became useless. You could get
warnings by enabling -Wunused-but-set-variable.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5fb082f..c402774 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4898,39 +4898,24 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	int num_connectors = 0;
 	intel_clock_t clock, reduced_clock;
 	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;
+	bool ok, has_reduced_clock = false;
+	bool is_lvds = false, is_dp = false, is_cpu_edp = false;
 	struct intel_encoder *encoder;
 	u32 temp;
 	int ret;
 	bool dither;
-	bool is_cpu_edp = false, is_pch_edp = false;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
 		case INTEL_OUTPUT_LVDS:
 			is_lvds = true;
 			break;
-		case INTEL_OUTPUT_SDVO:
-		case INTEL_OUTPUT_HDMI:
-			is_sdvo = true;
-			if (encoder->needs_tv_clock)
-				is_tv = true;
-			break;
-		case INTEL_OUTPUT_TVOUT:
-			is_tv = true;
-			break;
-		case INTEL_OUTPUT_ANALOG:
-			is_crt = true;
-			break;
 		case INTEL_OUTPUT_DISPLAYPORT:
 			is_dp = true;
 			break;
 		case INTEL_OUTPUT_EDP:
 			is_dp = true;
-			if (intel_encoder_is_pch_edp(&encoder->base))
-				is_pch_edp = true;
-			else
+			if (!intel_encoder_is_pch_edp(&encoder->base))
 				is_cpu_edp = true;
 			break;
 		}
-- 
1.7.10.4

Re: [PATCH 4/5] drm/i915: remove unused variables from ironlake_crtc_mode_set

[Rodrigo Vivi]

Feel free to use
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Thu, Sep 20, 2012 at 6:36 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> The last patches moved a lot of code from ironlake_crtc_mode_set to
> sub-functions, so these variables became useless. You could get
> warnings by enabling -Wunused-but-set-variable.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   21 +++------------------
>  1 file changed, 3 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5fb082f..c402774 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4898,39 +4898,24 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>         int num_connectors = 0;
>         intel_clock_t clock, reduced_clock;
>         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;
> +       bool ok, has_reduced_clock = false;
> +       bool is_lvds = false, is_dp = false, is_cpu_edp = false;
>         struct intel_encoder *encoder;
>         u32 temp;
>         int ret;
>         bool dither;
> -       bool is_cpu_edp = false, is_pch_edp = false;
>
>         for_each_encoder_on_crtc(dev, crtc, encoder) {
>                 switch (encoder->type) {
>                 case INTEL_OUTPUT_LVDS:
>                         is_lvds = true;
>                         break;
> -               case INTEL_OUTPUT_SDVO:
> -               case INTEL_OUTPUT_HDMI:
> -                       is_sdvo = true;
> -                       if (encoder->needs_tv_clock)
> -                               is_tv = true;
> -                       break;
> -               case INTEL_OUTPUT_TVOUT:
> -                       is_tv = true;
> -                       break;
> -               case INTEL_OUTPUT_ANALOG:
> -                       is_crt = true;
> -                       break;
>                 case INTEL_OUTPUT_DISPLAYPORT:
>                         is_dp = true;
>                         break;
>                 case INTEL_OUTPUT_EDP:
>                         is_dp = true;
> -                       if (intel_encoder_is_pch_edp(&encoder->base))
> -                               is_pch_edp = true;
> -                       else
> +                       if (!intel_encoder_is_pch_edp(&encoder->base))
>                                 is_cpu_edp = true;
>                         break;
>                 }
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 4/5] drm/i915: remove unused variables from ironlake_crtc_mode_set

[Daniel Vetter]

On Mon, Sep 24, 2012 at 08:36:27PM -0300, Rodrigo Vivi wrote:
> Feel free to use
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

Merged up to this patch to dinq, thanks for the review&patches.
-Daniel
> 
> On Thu, Sep 20, 2012 at 6:36 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > The last patches moved a lot of code from ironlake_crtc_mode_set to
> > sub-functions, so these variables became useless. You could get
> > warnings by enabling -Wunused-but-set-variable.
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   21 +++------------------
> >  1 file changed, 3 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 5fb082f..c402774 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4898,39 +4898,24 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> >         int num_connectors = 0;
> >         intel_clock_t clock, reduced_clock;
> >         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;
> > +       bool ok, has_reduced_clock = false;
> > +       bool is_lvds = false, is_dp = false, is_cpu_edp = false;
> >         struct intel_encoder *encoder;
> >         u32 temp;
> >         int ret;
> >         bool dither;
> > -       bool is_cpu_edp = false, is_pch_edp = false;
> >
> >         for_each_encoder_on_crtc(dev, crtc, encoder) {
> >                 switch (encoder->type) {
> >                 case INTEL_OUTPUT_LVDS:
> >                         is_lvds = true;
> >                         break;
> > -               case INTEL_OUTPUT_SDVO:
> > -               case INTEL_OUTPUT_HDMI:
> > -                       is_sdvo = true;
> > -                       if (encoder->needs_tv_clock)
> > -                               is_tv = true;
> > -                       break;
> > -               case INTEL_OUTPUT_TVOUT:
> > -                       is_tv = true;
> > -                       break;
> > -               case INTEL_OUTPUT_ANALOG:
> > -                       is_crt = true;
> > -                       break;
> >                 case INTEL_OUTPUT_DISPLAYPORT:
> >                         is_dp = true;
> >                         break;
> >                 case INTEL_OUTPUT_EDP:
> >                         is_dp = true;
> > -                       if (intel_encoder_is_pch_edp(&encoder->base))
> > -                               is_pch_edp = true;
> > -                       else
> > +                       if (!intel_encoder_is_pch_edp(&encoder->base))
> >                                 is_cpu_edp = true;
> >                         break;
> >                 }
> > --
> > 1.7.10.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
> _______________________________________________
> 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

[PATCH 5/5] drm/i915: extract intel_set_pipe_timings from crtc_mode_set

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Version 2: call intel_set_pipe_timings from both i9xx_crtc_mode_set
and ironlake_crtc_mode_set, instead of just ironlake, as requested by
Daniel Vetter.

The problem caused by calling this function from i9xx_crtc_mode_set too
is that now on i9xx we write to PIPESRC before writing to DSPSIZE and
DSPPOS. I could not find any evidence in our documentation that this
won't work, and the docs actually say the pipe registers should be set
before the plane registers.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  127 ++++++++++++++--------------------
 1 file changed, 52 insertions(+), 75 deletions(-)


Untested on i9xx. Needs a few tested-by stamps.


diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c402774..50b58a5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4250,6 +4250,55 @@ static void i8xx_update_pll(struct drm_crtc *crtc,
 	I915_WRITE(DPLL(pipe), dpll);
 }
 
+static void intel_set_pipe_timings(struct intel_crtc *intel_crtc,
+				   struct drm_display_mode *mode,
+				   struct drm_display_mode *adjusted_mode)
+{
+	struct drm_device *dev = intel_crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum pipe pipe = intel_crtc->pipe;
+	uint32_t vsyncshift;
+
+	if (!IS_GEN2(dev) && adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
+		/* the chip adds 2 halflines automatically */
+		adjusted_mode->crtc_vtotal -= 1;
+		adjusted_mode->crtc_vblank_end -= 1;
+		vsyncshift = adjusted_mode->crtc_hsync_start
+			     - adjusted_mode->crtc_htotal / 2;
+	} else {
+		vsyncshift = 0;
+	}
+
+	if (INTEL_INFO(dev)->gen > 3)
+		I915_WRITE(VSYNCSHIFT(pipe), vsyncshift);
+
+	I915_WRITE(HTOTAL(pipe),
+		   (adjusted_mode->crtc_hdisplay - 1) |
+		   ((adjusted_mode->crtc_htotal - 1) << 16));
+	I915_WRITE(HBLANK(pipe),
+		   (adjusted_mode->crtc_hblank_start - 1) |
+		   ((adjusted_mode->crtc_hblank_end - 1) << 16));
+	I915_WRITE(HSYNC(pipe),
+		   (adjusted_mode->crtc_hsync_start - 1) |
+		   ((adjusted_mode->crtc_hsync_end - 1) << 16));
+
+	I915_WRITE(VTOTAL(pipe),
+		   (adjusted_mode->crtc_vdisplay - 1) |
+		   ((adjusted_mode->crtc_vtotal - 1) << 16));
+	I915_WRITE(VBLANK(pipe),
+		   (adjusted_mode->crtc_vblank_start - 1) |
+		   ((adjusted_mode->crtc_vblank_end - 1) << 16));
+	I915_WRITE(VSYNC(pipe),
+		   (adjusted_mode->crtc_vsync_start - 1) |
+		   ((adjusted_mode->crtc_vsync_end - 1) << 16));
+
+	/* pipesrc controls the size that is scaled from, which should
+	 * always be the user's requested size.
+	 */
+	I915_WRITE(PIPESRC(pipe),
+		   ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
+}
+
 static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 			      struct drm_display_mode *mode,
 			      struct drm_display_mode *adjusted_mode,
@@ -4263,7 +4312,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	int plane = intel_crtc->plane;
 	int refclk, num_connectors = 0;
 	intel_clock_t clock, reduced_clock;
-	u32 dspcntr, pipeconf, vsyncshift;
+	u32 dspcntr, pipeconf;
 	bool ok, has_reduced_clock = false, is_sdvo = false;
 	bool is_lvds = false, is_tv = false, is_dp = false;
 	struct intel_encoder *encoder;
@@ -4388,42 +4437,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 		}
 	}
 
-	pipeconf &= ~PIPECONF_INTERLACE_MASK;
-	if (!IS_GEN2(dev) &&
-	    adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
-		pipeconf |= PIPECONF_INTERLACE_W_FIELD_INDICATION;
-		/* the chip adds 2 halflines automatically */
-		adjusted_mode->crtc_vtotal -= 1;
-		adjusted_mode->crtc_vblank_end -= 1;
-		vsyncshift = adjusted_mode->crtc_hsync_start
-			     - adjusted_mode->crtc_htotal/2;
-	} else {
-		pipeconf |= PIPECONF_PROGRESSIVE;
-		vsyncshift = 0;
-	}
-
-	if (!IS_GEN3(dev))
-		I915_WRITE(VSYNCSHIFT(pipe), vsyncshift);
-
-	I915_WRITE(HTOTAL(pipe),
-		   (adjusted_mode->crtc_hdisplay - 1) |
-		   ((adjusted_mode->crtc_htotal - 1) << 16));
-	I915_WRITE(HBLANK(pipe),
-		   (adjusted_mode->crtc_hblank_start - 1) |
-		   ((adjusted_mode->crtc_hblank_end - 1) << 16));
-	I915_WRITE(HSYNC(pipe),
-		   (adjusted_mode->crtc_hsync_start - 1) |
-		   ((adjusted_mode->crtc_hsync_end - 1) << 16));
-
-	I915_WRITE(VTOTAL(pipe),
-		   (adjusted_mode->crtc_vdisplay - 1) |
-		   ((adjusted_mode->crtc_vtotal - 1) << 16));
-	I915_WRITE(VBLANK(pipe),
-		   (adjusted_mode->crtc_vblank_start - 1) |
-		   ((adjusted_mode->crtc_vblank_end - 1) << 16));
-	I915_WRITE(VSYNC(pipe),
-		   (adjusted_mode->crtc_vsync_start - 1) |
-		   ((adjusted_mode->crtc_vsync_end - 1) << 16));
+	intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
 
 	/* pipesrc and dspsize control the size that is scaled from,
 	 * which should always be the user's requested size.
@@ -4432,8 +4446,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 		   ((mode->vdisplay - 1) << 16) |
 		   (mode->hdisplay - 1));
 	I915_WRITE(DSPPOS(plane), 0);
-	I915_WRITE(PIPESRC(pipe),
-		   ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
 
 	I915_WRITE(PIPECONF(pipe), pipeconf);
 	POSTING_READ(PIPECONF(pipe));
@@ -5039,42 +5051,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		}
 	}
 
-	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
-		/* the chip adds 2 halflines automatically */
-		adjusted_mode->crtc_vtotal -= 1;
-		adjusted_mode->crtc_vblank_end -= 1;
-		I915_WRITE(VSYNCSHIFT(pipe),
-			   adjusted_mode->crtc_hsync_start
-			   - adjusted_mode->crtc_htotal/2);
-	} else {
-		I915_WRITE(VSYNCSHIFT(pipe), 0);
-	}
-
-	I915_WRITE(HTOTAL(pipe),
-		   (adjusted_mode->crtc_hdisplay - 1) |
-		   ((adjusted_mode->crtc_htotal - 1) << 16));
-	I915_WRITE(HBLANK(pipe),
-		   (adjusted_mode->crtc_hblank_start - 1) |
-		   ((adjusted_mode->crtc_hblank_end - 1) << 16));
-	I915_WRITE(HSYNC(pipe),
-		   (adjusted_mode->crtc_hsync_start - 1) |
-		   ((adjusted_mode->crtc_hsync_end - 1) << 16));
-
-	I915_WRITE(VTOTAL(pipe),
-		   (adjusted_mode->crtc_vdisplay - 1) |
-		   ((adjusted_mode->crtc_vtotal - 1) << 16));
-	I915_WRITE(VBLANK(pipe),
-		   (adjusted_mode->crtc_vblank_start - 1) |
-		   ((adjusted_mode->crtc_vblank_end - 1) << 16));
-	I915_WRITE(VSYNC(pipe),
-		   (adjusted_mode->crtc_vsync_start - 1) |
-		   ((adjusted_mode->crtc_vsync_end - 1) << 16));
-
-	/* pipesrc controls the size that is scaled from, which should
-	 * always be the user's requested size.
-	 */
-	I915_WRITE(PIPESRC(pipe),
-		   ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
+	intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
 
 	ironlake_set_m_n(crtc, mode, adjusted_mode);
 
-- 
1.7.10.4

Re: [PATCH 5/5] drm/i915: extract intel_set_pipe_timings from crtc_mode_set

[Rodrigo Vivi]

what about
pipeconf |= PIPECONF_INTERLACE_W_FIELD_INDICATION;?

isn't this useful anymore? in this case I think it would be better
split this patch in 2: 1 to remove it and another one to organize the
functions.

On Thu, Sep 20, 2012 at 6:36 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Version 2: call intel_set_pipe_timings from both i9xx_crtc_mode_set
> and ironlake_crtc_mode_set, instead of just ironlake, as requested by
> Daniel Vetter.
>
> The problem caused by calling this function from i9xx_crtc_mode_set too
> is that now on i9xx we write to PIPESRC before writing to DSPSIZE and
> DSPPOS. I could not find any evidence in our documentation that this
> won't work, and the docs actually say the pipe registers should be set
> before the plane registers.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  127 ++++++++++++++--------------------
>  1 file changed, 52 insertions(+), 75 deletions(-)
>
>
> Untested on i9xx. Needs a few tested-by stamps.
>
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c402774..50b58a5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4250,6 +4250,55 @@ static void i8xx_update_pll(struct drm_crtc *crtc,
>         I915_WRITE(DPLL(pipe), dpll);
>  }
>
> +static void intel_set_pipe_timings(struct intel_crtc *intel_crtc,
> +                                  struct drm_display_mode *mode,
> +                                  struct drm_display_mode *adjusted_mode)
> +{
> +       struct drm_device *dev = intel_crtc->base.dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       enum pipe pipe = intel_crtc->pipe;
> +       uint32_t vsyncshift;
> +
> +       if (!IS_GEN2(dev) && adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
> +               /* the chip adds 2 halflines automatically */
> +               adjusted_mode->crtc_vtotal -= 1;
> +               adjusted_mode->crtc_vblank_end -= 1;
> +               vsyncshift = adjusted_mode->crtc_hsync_start
> +                            - adjusted_mode->crtc_htotal / 2;
> +       } else {
> +               vsyncshift = 0;
> +       }
> +
> +       if (INTEL_INFO(dev)->gen > 3)
> +               I915_WRITE(VSYNCSHIFT(pipe), vsyncshift);
> +
> +       I915_WRITE(HTOTAL(pipe),
> +                  (adjusted_mode->crtc_hdisplay - 1) |
> +                  ((adjusted_mode->crtc_htotal - 1) << 16));
> +       I915_WRITE(HBLANK(pipe),
> +                  (adjusted_mode->crtc_hblank_start - 1) |
> +                  ((adjusted_mode->crtc_hblank_end - 1) << 16));
> +       I915_WRITE(HSYNC(pipe),
> +                  (adjusted_mode->crtc_hsync_start - 1) |
> +                  ((adjusted_mode->crtc_hsync_end - 1) << 16));
> +
> +       I915_WRITE(VTOTAL(pipe),
> +                  (adjusted_mode->crtc_vdisplay - 1) |
> +                  ((adjusted_mode->crtc_vtotal - 1) << 16));
> +       I915_WRITE(VBLANK(pipe),
> +                  (adjusted_mode->crtc_vblank_start - 1) |
> +                  ((adjusted_mode->crtc_vblank_end - 1) << 16));
> +       I915_WRITE(VSYNC(pipe),
> +                  (adjusted_mode->crtc_vsync_start - 1) |
> +                  ((adjusted_mode->crtc_vsync_end - 1) << 16));
> +
> +       /* pipesrc controls the size that is scaled from, which should
> +        * always be the user's requested size.
> +        */
> +       I915_WRITE(PIPESRC(pipe),
> +                  ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
> +}
> +
>  static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>                               struct drm_display_mode *mode,
>                               struct drm_display_mode *adjusted_mode,
> @@ -4263,7 +4312,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>         int plane = intel_crtc->plane;
>         int refclk, num_connectors = 0;
>         intel_clock_t clock, reduced_clock;
> -       u32 dspcntr, pipeconf, vsyncshift;
> +       u32 dspcntr, pipeconf;
>         bool ok, has_reduced_clock = false, is_sdvo = false;
>         bool is_lvds = false, is_tv = false, is_dp = false;
>         struct intel_encoder *encoder;
> @@ -4388,42 +4437,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>                 }
>         }
>
> -       pipeconf &= ~PIPECONF_INTERLACE_MASK;
> -       if (!IS_GEN2(dev) &&
> -           adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
> -               pipeconf |= PIPECONF_INTERLACE_W_FIELD_INDICATION;
> -               /* the chip adds 2 halflines automatically */
> -               adjusted_mode->crtc_vtotal -= 1;
> -               adjusted_mode->crtc_vblank_end -= 1;
> -               vsyncshift = adjusted_mode->crtc_hsync_start
> -                            - adjusted_mode->crtc_htotal/2;
> -       } else {
> -               pipeconf |= PIPECONF_PROGRESSIVE;
> -               vsyncshift = 0;
> -       }
> -
> -       if (!IS_GEN3(dev))
> -               I915_WRITE(VSYNCSHIFT(pipe), vsyncshift);
> -
> -       I915_WRITE(HTOTAL(pipe),
> -                  (adjusted_mode->crtc_hdisplay - 1) |
> -                  ((adjusted_mode->crtc_htotal - 1) << 16));
> -       I915_WRITE(HBLANK(pipe),
> -                  (adjusted_mode->crtc_hblank_start - 1) |
> -                  ((adjusted_mode->crtc_hblank_end - 1) << 16));
> -       I915_WRITE(HSYNC(pipe),
> -                  (adjusted_mode->crtc_hsync_start - 1) |
> -                  ((adjusted_mode->crtc_hsync_end - 1) << 16));
> -
> -       I915_WRITE(VTOTAL(pipe),
> -                  (adjusted_mode->crtc_vdisplay - 1) |
> -                  ((adjusted_mode->crtc_vtotal - 1) << 16));
> -       I915_WRITE(VBLANK(pipe),
> -                  (adjusted_mode->crtc_vblank_start - 1) |
> -                  ((adjusted_mode->crtc_vblank_end - 1) << 16));
> -       I915_WRITE(VSYNC(pipe),
> -                  (adjusted_mode->crtc_vsync_start - 1) |
> -                  ((adjusted_mode->crtc_vsync_end - 1) << 16));
> +       intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
>
>         /* pipesrc and dspsize control the size that is scaled from,
>          * which should always be the user's requested size.
> @@ -4432,8 +4446,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>                    ((mode->vdisplay - 1) << 16) |
>                    (mode->hdisplay - 1));
>         I915_WRITE(DSPPOS(plane), 0);
> -       I915_WRITE(PIPESRC(pipe),
> -                  ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
>
>         I915_WRITE(PIPECONF(pipe), pipeconf);
>         POSTING_READ(PIPECONF(pipe));
> @@ -5039,42 +5051,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>                 }
>         }
>
> -       if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
> -               /* the chip adds 2 halflines automatically */
> -               adjusted_mode->crtc_vtotal -= 1;
> -               adjusted_mode->crtc_vblank_end -= 1;
> -               I915_WRITE(VSYNCSHIFT(pipe),
> -                          adjusted_mode->crtc_hsync_start
> -                          - adjusted_mode->crtc_htotal/2);
> -       } else {
> -               I915_WRITE(VSYNCSHIFT(pipe), 0);
> -       }
> -
> -       I915_WRITE(HTOTAL(pipe),
> -                  (adjusted_mode->crtc_hdisplay - 1) |
> -                  ((adjusted_mode->crtc_htotal - 1) << 16));
> -       I915_WRITE(HBLANK(pipe),
> -                  (adjusted_mode->crtc_hblank_start - 1) |
> -                  ((adjusted_mode->crtc_hblank_end - 1) << 16));
> -       I915_WRITE(HSYNC(pipe),
> -                  (adjusted_mode->crtc_hsync_start - 1) |
> -                  ((adjusted_mode->crtc_hsync_end - 1) << 16));
> -
> -       I915_WRITE(VTOTAL(pipe),
> -                  (adjusted_mode->crtc_vdisplay - 1) |
> -                  ((adjusted_mode->crtc_vtotal - 1) << 16));
> -       I915_WRITE(VBLANK(pipe),
> -                  (adjusted_mode->crtc_vblank_start - 1) |
> -                  ((adjusted_mode->crtc_vblank_end - 1) << 16));
> -       I915_WRITE(VSYNC(pipe),
> -                  (adjusted_mode->crtc_vsync_start - 1) |
> -                  ((adjusted_mode->crtc_vsync_end - 1) << 16));
> -
> -       /* pipesrc controls the size that is scaled from, which should
> -        * always be the user's requested size.
> -        */
> -       I915_WRITE(PIPESRC(pipe),
> -                  ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
> +       intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
>
>         ironlake_set_m_n(crtc, mode, adjusted_mode);
>
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

[PATCH] drm/i915: extract intel_set_pipe_timings from crtc_mode_set

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Version 2: call intel_set_pipe_timings from both i9xx_crtc_mode_set
and ironlake_crtc_mode_set, instead of just ironlake, as requested by
Daniel Vetter.

The problem caused by calling this function from i9xx_crtc_mode_set
too is that now on i9xx we write to PIPESRC before writing to DSPSIZE
and DSPPOS. I could not find any evidence in our documentation that
this won't work, and the docs actually say the pipe registers should
be set before the plane registers.

Version 3: don't remove pipeconf bits on i9xx_crtc_mode_set.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 124 +++++++++++++++--------------------
 1 file changed, 54 insertions(+), 70 deletions(-)


Still untested on i9xx.


diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ed0dcea..6cf0d00 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4290,6 +4290,55 @@ static void i8xx_update_pll(struct drm_crtc *crtc,
 	I915_WRITE(DPLL(pipe), dpll);
 }
 
+static void intel_set_pipe_timings(struct intel_crtc *intel_crtc,
+				   struct drm_display_mode *mode,
+				   struct drm_display_mode *adjusted_mode)
+{
+	struct drm_device *dev = intel_crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum pipe pipe = intel_crtc->pipe;
+	uint32_t vsyncshift;
+
+	if (!IS_GEN2(dev) && adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
+		/* the chip adds 2 halflines automatically */
+		adjusted_mode->crtc_vtotal -= 1;
+		adjusted_mode->crtc_vblank_end -= 1;
+		vsyncshift = adjusted_mode->crtc_hsync_start
+			     - adjusted_mode->crtc_htotal / 2;
+	} else {
+		vsyncshift = 0;
+	}
+
+	if (INTEL_INFO(dev)->gen > 3)
+		I915_WRITE(VSYNCSHIFT(pipe), vsyncshift);
+
+	I915_WRITE(HTOTAL(pipe),
+		   (adjusted_mode->crtc_hdisplay - 1) |
+		   ((adjusted_mode->crtc_htotal - 1) << 16));
+	I915_WRITE(HBLANK(pipe),
+		   (adjusted_mode->crtc_hblank_start - 1) |
+		   ((adjusted_mode->crtc_hblank_end - 1) << 16));
+	I915_WRITE(HSYNC(pipe),
+		   (adjusted_mode->crtc_hsync_start - 1) |
+		   ((adjusted_mode->crtc_hsync_end - 1) << 16));
+
+	I915_WRITE(VTOTAL(pipe),
+		   (adjusted_mode->crtc_vdisplay - 1) |
+		   ((adjusted_mode->crtc_vtotal - 1) << 16));
+	I915_WRITE(VBLANK(pipe),
+		   (adjusted_mode->crtc_vblank_start - 1) |
+		   ((adjusted_mode->crtc_vblank_end - 1) << 16));
+	I915_WRITE(VSYNC(pipe),
+		   (adjusted_mode->crtc_vsync_start - 1) |
+		   ((adjusted_mode->crtc_vsync_end - 1) << 16));
+
+	/* pipesrc controls the size that is scaled from, which should
+	 * always be the user's requested size.
+	 */
+	I915_WRITE(PIPESRC(pipe),
+		   ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
+}
+
 static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 			      struct drm_display_mode *mode,
 			      struct drm_display_mode *adjusted_mode,
@@ -4303,7 +4352,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	int plane = intel_crtc->plane;
 	int refclk, num_connectors = 0;
 	intel_clock_t clock, reduced_clock;
-	u32 dspcntr, pipeconf, vsyncshift;
+	u32 dspcntr, pipeconf;
 	bool ok, has_reduced_clock = false, is_sdvo = false;
 	bool is_lvds = false, is_tv = false, is_dp = false;
 	struct intel_encoder *encoder;
@@ -4438,40 +4487,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 
 	pipeconf &= ~PIPECONF_INTERLACE_MASK;
 	if (!IS_GEN2(dev) &&
-	    adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
+	    adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
 		pipeconf |= PIPECONF_INTERLACE_W_FIELD_INDICATION;
-		/* the chip adds 2 halflines automatically */
-		adjusted_mode->crtc_vtotal -= 1;
-		adjusted_mode->crtc_vblank_end -= 1;
-		vsyncshift = adjusted_mode->crtc_hsync_start
-			     - adjusted_mode->crtc_htotal/2;
-	} else {
+	else
 		pipeconf |= PIPECONF_PROGRESSIVE;
-		vsyncshift = 0;
-	}
-
-	if (!IS_GEN3(dev))
-		I915_WRITE(VSYNCSHIFT(pipe), vsyncshift);
 
-	I915_WRITE(HTOTAL(pipe),
-		   (adjusted_mode->crtc_hdisplay - 1) |
-		   ((adjusted_mode->crtc_htotal - 1) << 16));
-	I915_WRITE(HBLANK(pipe),
-		   (adjusted_mode->crtc_hblank_start - 1) |
-		   ((adjusted_mode->crtc_hblank_end - 1) << 16));
-	I915_WRITE(HSYNC(pipe),
-		   (adjusted_mode->crtc_hsync_start - 1) |
-		   ((adjusted_mode->crtc_hsync_end - 1) << 16));
-
-	I915_WRITE(VTOTAL(pipe),
-		   (adjusted_mode->crtc_vdisplay - 1) |
-		   ((adjusted_mode->crtc_vtotal - 1) << 16));
-	I915_WRITE(VBLANK(pipe),
-		   (adjusted_mode->crtc_vblank_start - 1) |
-		   ((adjusted_mode->crtc_vblank_end - 1) << 16));
-	I915_WRITE(VSYNC(pipe),
-		   (adjusted_mode->crtc_vsync_start - 1) |
-		   ((adjusted_mode->crtc_vsync_end - 1) << 16));
+	intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
 
 	/* pipesrc and dspsize control the size that is scaled from,
 	 * which should always be the user's requested size.
@@ -4480,8 +4501,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 		   ((mode->vdisplay - 1) << 16) |
 		   (mode->hdisplay - 1));
 	I915_WRITE(DSPPOS(plane), 0);
-	I915_WRITE(PIPESRC(pipe),
-		   ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
 
 	I915_WRITE(PIPECONF(pipe), pipeconf);
 	POSTING_READ(PIPECONF(pipe));
@@ -5087,42 +5106,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		}
 	}
 
-	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
-		/* the chip adds 2 halflines automatically */
-		adjusted_mode->crtc_vtotal -= 1;
-		adjusted_mode->crtc_vblank_end -= 1;
-		I915_WRITE(VSYNCSHIFT(pipe),
-			   adjusted_mode->crtc_hsync_start
-			   - adjusted_mode->crtc_htotal/2);
-	} else {
-		I915_WRITE(VSYNCSHIFT(pipe), 0);
-	}
-
-	I915_WRITE(HTOTAL(pipe),
-		   (adjusted_mode->crtc_hdisplay - 1) |
-		   ((adjusted_mode->crtc_htotal - 1) << 16));
-	I915_WRITE(HBLANK(pipe),
-		   (adjusted_mode->crtc_hblank_start - 1) |
-		   ((adjusted_mode->crtc_hblank_end - 1) << 16));
-	I915_WRITE(HSYNC(pipe),
-		   (adjusted_mode->crtc_hsync_start - 1) |
-		   ((adjusted_mode->crtc_hsync_end - 1) << 16));
-
-	I915_WRITE(VTOTAL(pipe),
-		   (adjusted_mode->crtc_vdisplay - 1) |
-		   ((adjusted_mode->crtc_vtotal - 1) << 16));
-	I915_WRITE(VBLANK(pipe),
-		   (adjusted_mode->crtc_vblank_start - 1) |
-		   ((adjusted_mode->crtc_vblank_end - 1) << 16));
-	I915_WRITE(VSYNC(pipe),
-		   (adjusted_mode->crtc_vsync_start - 1) |
-		   ((adjusted_mode->crtc_vsync_end - 1) << 16));
-
-	/* pipesrc controls the size that is scaled from, which should
-	 * always be the user's requested size.
-	 */
-	I915_WRITE(PIPESRC(pipe),
-		   ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
+	intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
 
 	ironlake_set_m_n(crtc, mode, adjusted_mode);
 
-- 
1.7.11.4

Re: [PATCH] drm/i915: extract intel_set_pipe_timings from crtc_mode_set

[Rodrigo Vivi]

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Mon, Oct 1, 2012 at 6:10 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Version 2: call intel_set_pipe_timings from both i9xx_crtc_mode_set
> and ironlake_crtc_mode_set, instead of just ironlake, as requested by
> Daniel Vetter.
>
> The problem caused by calling this function from i9xx_crtc_mode_set
> too is that now on i9xx we write to PIPESRC before writing to DSPSIZE
> and DSPPOS. I could not find any evidence in our documentation that
> this won't work, and the docs actually say the pipe registers should
> be set before the plane registers.
>
> Version 3: don't remove pipeconf bits on i9xx_crtc_mode_set.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 124 +++++++++++++++--------------------
>  1 file changed, 54 insertions(+), 70 deletions(-)
>
>
> Still untested on i9xx.
>
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ed0dcea..6cf0d00 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4290,6 +4290,55 @@ static void i8xx_update_pll(struct drm_crtc *crtc,
>         I915_WRITE(DPLL(pipe), dpll);
>  }
>
> +static void intel_set_pipe_timings(struct intel_crtc *intel_crtc,
> +                                  struct drm_display_mode *mode,
> +                                  struct drm_display_mode *adjusted_mode)
> +{
> +       struct drm_device *dev = intel_crtc->base.dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       enum pipe pipe = intel_crtc->pipe;
> +       uint32_t vsyncshift;
> +
> +       if (!IS_GEN2(dev) && adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
> +               /* the chip adds 2 halflines automatically */
> +               adjusted_mode->crtc_vtotal -= 1;
> +               adjusted_mode->crtc_vblank_end -= 1;
> +               vsyncshift = adjusted_mode->crtc_hsync_start
> +                            - adjusted_mode->crtc_htotal / 2;
> +       } else {
> +               vsyncshift = 0;
> +       }
> +
> +       if (INTEL_INFO(dev)->gen > 3)
> +               I915_WRITE(VSYNCSHIFT(pipe), vsyncshift);
> +
> +       I915_WRITE(HTOTAL(pipe),
> +                  (adjusted_mode->crtc_hdisplay - 1) |
> +                  ((adjusted_mode->crtc_htotal - 1) << 16));
> +       I915_WRITE(HBLANK(pipe),
> +                  (adjusted_mode->crtc_hblank_start - 1) |
> +                  ((adjusted_mode->crtc_hblank_end - 1) << 16));
> +       I915_WRITE(HSYNC(pipe),
> +                  (adjusted_mode->crtc_hsync_start - 1) |
> +                  ((adjusted_mode->crtc_hsync_end - 1) << 16));
> +
> +       I915_WRITE(VTOTAL(pipe),
> +                  (adjusted_mode->crtc_vdisplay - 1) |
> +                  ((adjusted_mode->crtc_vtotal - 1) << 16));
> +       I915_WRITE(VBLANK(pipe),
> +                  (adjusted_mode->crtc_vblank_start - 1) |
> +                  ((adjusted_mode->crtc_vblank_end - 1) << 16));
> +       I915_WRITE(VSYNC(pipe),
> +                  (adjusted_mode->crtc_vsync_start - 1) |
> +                  ((adjusted_mode->crtc_vsync_end - 1) << 16));
> +
> +       /* pipesrc controls the size that is scaled from, which should
> +        * always be the user's requested size.
> +        */
> +       I915_WRITE(PIPESRC(pipe),
> +                  ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
> +}
> +
>  static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>                               struct drm_display_mode *mode,
>                               struct drm_display_mode *adjusted_mode,
> @@ -4303,7 +4352,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>         int plane = intel_crtc->plane;
>         int refclk, num_connectors = 0;
>         intel_clock_t clock, reduced_clock;
> -       u32 dspcntr, pipeconf, vsyncshift;
> +       u32 dspcntr, pipeconf;
>         bool ok, has_reduced_clock = false, is_sdvo = false;
>         bool is_lvds = false, is_tv = false, is_dp = false;
>         struct intel_encoder *encoder;
> @@ -4438,40 +4487,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>
>         pipeconf &= ~PIPECONF_INTERLACE_MASK;
>         if (!IS_GEN2(dev) &&
> -           adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
> +           adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
>                 pipeconf |= PIPECONF_INTERLACE_W_FIELD_INDICATION;
> -               /* the chip adds 2 halflines automatically */
> -               adjusted_mode->crtc_vtotal -= 1;
> -               adjusted_mode->crtc_vblank_end -= 1;
> -               vsyncshift = adjusted_mode->crtc_hsync_start
> -                            - adjusted_mode->crtc_htotal/2;
> -       } else {
> +       else
>                 pipeconf |= PIPECONF_PROGRESSIVE;
> -               vsyncshift = 0;
> -       }
> -
> -       if (!IS_GEN3(dev))
> -               I915_WRITE(VSYNCSHIFT(pipe), vsyncshift);
>
> -       I915_WRITE(HTOTAL(pipe),
> -                  (adjusted_mode->crtc_hdisplay - 1) |
> -                  ((adjusted_mode->crtc_htotal - 1) << 16));
> -       I915_WRITE(HBLANK(pipe),
> -                  (adjusted_mode->crtc_hblank_start - 1) |
> -                  ((adjusted_mode->crtc_hblank_end - 1) << 16));
> -       I915_WRITE(HSYNC(pipe),
> -                  (adjusted_mode->crtc_hsync_start - 1) |
> -                  ((adjusted_mode->crtc_hsync_end - 1) << 16));
> -
> -       I915_WRITE(VTOTAL(pipe),
> -                  (adjusted_mode->crtc_vdisplay - 1) |
> -                  ((adjusted_mode->crtc_vtotal - 1) << 16));
> -       I915_WRITE(VBLANK(pipe),
> -                  (adjusted_mode->crtc_vblank_start - 1) |
> -                  ((adjusted_mode->crtc_vblank_end - 1) << 16));
> -       I915_WRITE(VSYNC(pipe),
> -                  (adjusted_mode->crtc_vsync_start - 1) |
> -                  ((adjusted_mode->crtc_vsync_end - 1) << 16));
> +       intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
>
>         /* pipesrc and dspsize control the size that is scaled from,
>          * which should always be the user's requested size.
> @@ -4480,8 +4501,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>                    ((mode->vdisplay - 1) << 16) |
>                    (mode->hdisplay - 1));
>         I915_WRITE(DSPPOS(plane), 0);
> -       I915_WRITE(PIPESRC(pipe),
> -                  ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
>
>         I915_WRITE(PIPECONF(pipe), pipeconf);
>         POSTING_READ(PIPECONF(pipe));
> @@ -5087,42 +5106,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>                 }
>         }
>
> -       if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
> -               /* the chip adds 2 halflines automatically */
> -               adjusted_mode->crtc_vtotal -= 1;
> -               adjusted_mode->crtc_vblank_end -= 1;
> -               I915_WRITE(VSYNCSHIFT(pipe),
> -                          adjusted_mode->crtc_hsync_start
> -                          - adjusted_mode->crtc_htotal/2);
> -       } else {
> -               I915_WRITE(VSYNCSHIFT(pipe), 0);
> -       }
> -
> -       I915_WRITE(HTOTAL(pipe),
> -                  (adjusted_mode->crtc_hdisplay - 1) |
> -                  ((adjusted_mode->crtc_htotal - 1) << 16));
> -       I915_WRITE(HBLANK(pipe),
> -                  (adjusted_mode->crtc_hblank_start - 1) |
> -                  ((adjusted_mode->crtc_hblank_end - 1) << 16));
> -       I915_WRITE(HSYNC(pipe),
> -                  (adjusted_mode->crtc_hsync_start - 1) |
> -                  ((adjusted_mode->crtc_hsync_end - 1) << 16));
> -
> -       I915_WRITE(VTOTAL(pipe),
> -                  (adjusted_mode->crtc_vdisplay - 1) |
> -                  ((adjusted_mode->crtc_vtotal - 1) << 16));
> -       I915_WRITE(VBLANK(pipe),
> -                  (adjusted_mode->crtc_vblank_start - 1) |
> -                  ((adjusted_mode->crtc_vblank_end - 1) << 16));
> -       I915_WRITE(VSYNC(pipe),
> -                  (adjusted_mode->crtc_vsync_start - 1) |
> -                  ((adjusted_mode->crtc_vsync_end - 1) << 16));
> -
> -       /* pipesrc controls the size that is scaled from, which should
> -        * always be the user's requested size.
> -        */
> -       I915_WRITE(PIPESRC(pipe),
> -                  ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
> +       intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
>
>         ironlake_set_m_n(crtc, mode, adjusted_mode);
>
> --
> 1.7.11.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br