[PATCH 1/2] drm/i915: Remove the redundant check for a fixed_panel_mode

[PATCH 1/2] drm/i915: Remove the redundant check for a fixed_panel_mode

[Chris Wilson]

We already checked just a couple of lines above that we have found a
fixed_panel_mode for the LVDS, so remove the surplus check.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lvds.c |   32 +++++++++++++++-----------------
 1 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 0eab8df..6ef9388 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -200,27 +200,25 @@ static bool intel_lvds_mode_fixup(struct drm_encoder *encoder,
 	if (dev_priv->panel_fixed_mode == NULL)
 		return true;
 	/*
-	 * If we have timings from the BIOS for the panel, put them in
+	 * We have timings from the BIOS for the panel, put them in
 	 * to the adjusted mode.  The CRTC will be set up for this mode,
 	 * with the panel scaling set up to source from the H/VDisplay
 	 * of the original mode.
 	 */
-	if (dev_priv->panel_fixed_mode != NULL) {
-		adjusted_mode->hdisplay = dev_priv->panel_fixed_mode->hdisplay;
-		adjusted_mode->hsync_start =
-			dev_priv->panel_fixed_mode->hsync_start;
-		adjusted_mode->hsync_end =
-			dev_priv->panel_fixed_mode->hsync_end;
-		adjusted_mode->htotal = dev_priv->panel_fixed_mode->htotal;
-		adjusted_mode->vdisplay = dev_priv->panel_fixed_mode->vdisplay;
-		adjusted_mode->vsync_start =
-			dev_priv->panel_fixed_mode->vsync_start;
-		adjusted_mode->vsync_end =
-			dev_priv->panel_fixed_mode->vsync_end;
-		adjusted_mode->vtotal = dev_priv->panel_fixed_mode->vtotal;
-		adjusted_mode->clock = dev_priv->panel_fixed_mode->clock;
-		drm_mode_set_crtcinfo(adjusted_mode, CRTC_INTERLACE_HALVE_V);
-	}
+	adjusted_mode->hdisplay = dev_priv->panel_fixed_mode->hdisplay;
+	adjusted_mode->hsync_start =
+		dev_priv->panel_fixed_mode->hsync_start;
+	adjusted_mode->hsync_end =
+		dev_priv->panel_fixed_mode->hsync_end;
+	adjusted_mode->htotal = dev_priv->panel_fixed_mode->htotal;
+	adjusted_mode->vdisplay = dev_priv->panel_fixed_mode->vdisplay;
+	adjusted_mode->vsync_start =
+		dev_priv->panel_fixed_mode->vsync_start;
+	adjusted_mode->vsync_end =
+		dev_priv->panel_fixed_mode->vsync_end;
+	adjusted_mode->vtotal = dev_priv->panel_fixed_mode->vtotal;
+	adjusted_mode->clock = dev_priv->panel_fixed_mode->clock;
+	drm_mode_set_crtcinfo(adjusted_mode, CRTC_INTERLACE_HALVE_V);
 
 	/* Make sure pre-965s set dither correctly */
 	if (!IS_I965G(dev)) {
-- 
1.7.1

[PATCH 2/2] drm/i915: Refactor panel fitting on the LVDS.

[Chris Wilson]

Move the common routines into separate functions to not only increase
readability, but also throwaway surplus code.

In doing so, we review the calculation of the aspect preserving scaling
and avoid the use of fixed-point until we need to calculate the accurate
scale factor.

1 files changed, 128 insertions(+), 194 deletions(-)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_lvds.c |  322 +++++++++++++++----------------------
 1 files changed, 128 insertions(+), 194 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 6ef9388..eab6a3a 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -156,31 +156,92 @@ static int intel_lvds_mode_valid(struct drm_connector *connector,
 	return MODE_OK;
 }
 
+static void
+centre_horizontally(struct drm_display_mode *mode,
+		    int width)
+{
+	u32 border, sync_pos, blank_width, sync_width;
+
+	sync_width = mode->crtc_hsync_end - mode->crtc_hsync_start;
+	blank_width = mode->crtc_hblank_end - mode->crtc_hblank_start;
+
+	border = (mode->hdisplay - width) / 2;
+
+	if (mode->hdisplay & 1) /* odd resolutions */
+		border++;
+
+	/* keep the border be even */
+	if (border & 1)
+		border++;
+
+	mode->crtc_hdisplay = width;
+	mode->crtc_hblank_start = width + border;
+	/* keep the hblank width constant */
+	mode->crtc_hblank_end =
+		mode->crtc_hblank_start + blank_width;
+
+	/* get the hsync start pos relative to hblank start */
+	sync_pos = (blank_width - sync_width) / 2;
+	/* keep the hsync_pos be even */
+	if (sync_pos & 1)
+		sync_pos++;
+
+	/* keep hsync width constant */
+	mode->crtc_hsync_start = mode->crtc_hblank_start + sync_pos;
+	mode->crtc_hsync_end = mode->crtc_hsync_start + sync_width;
+}
+
+static void
+centre_vertically(struct drm_display_mode *mode,
+		  int height)
+{
+	u32 border, sync_pos, blank_width, sync_width;
+
+	sync_width = mode->crtc_vsync_end - mode->crtc_vsync_start;
+	blank_width = mode->crtc_vblank_end - mode->crtc_vblank_start;
+
+	border = (mode->vdisplay - height) / 2;
+
+	if (mode->vdisplay & 1)
+		border++;
+
+	mode->crtc_vdisplay = height;
+	mode->crtc_vblank_start = height + border;
+	/* keep the vblank width constant */
+	mode->crtc_vblank_end = mode->crtc_vblank_start + blank_width;
+
+	/* get the vsync start pos relative to vblank start */
+	sync_pos = (blank_width - sync_width) / 2;
+
+	/* keep the vsync width constant */
+	mode->crtc_vsync_start = mode->crtc_vblank_start + sync_pos;
+	mode->crtc_vsync_end = mode->crtc_vsync_start + sync_width;
+}
+
+static inline u32 panel_fitter_scaling(u32 source, u32 target)
+{
+	/*
+	 * Floating point operation is not supported. So the FACTOR
+	 * is defined, which can avoid the floating point computation
+	 * when calculating the panel ratio.
+	 */
+#define ACCURACY 12
+#define FACTOR (1 << ACCURACY)
+	u32 ratio = source * FACTOR / target;
+	return (FACTOR * (ratio + FACTOR/2)) / FACTOR;
+}
+
 static bool intel_lvds_mode_fixup(struct drm_encoder *encoder,
 				  struct drm_display_mode *mode,
 				  struct drm_display_mode *adjusted_mode)
 {
-	/*
-	 * float point operation is not supported . So the PANEL_RATIO_FACTOR
-	 * is defined, which can avoid the float point computation when
-	 * calculating the panel ratio.
-	 */
-#define PANEL_RATIO_FACTOR 8192
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
 	struct drm_encoder *tmp_encoder;
 	struct intel_encoder *intel_encoder = enc_to_intel_encoder(encoder);
 	struct intel_lvds_priv *lvds_priv = intel_encoder->dev_priv;
-	u32 pfit_control = 0, pfit_pgm_ratios = 0;
-	int left_border = 0, right_border = 0, top_border = 0;
-	int bottom_border = 0;
-	bool border = 0;
-	int panel_ratio, desired_ratio, vert_scale, horiz_scale;
-	int horiz_ratio, vert_ratio;
-	u32 hsync_width, vsync_width;
-	u32 hblank_width, vblank_width;
-	u32 hsync_pos, vsync_pos;
+	u32 pfit_control = 0, pfit_pgm_ratios = 0, border = 0;
 
 	/* Should never happen!! */
 	if (!IS_I965G(dev) && intel_crtc->pipe == 0) {
@@ -228,11 +289,8 @@ static bool intel_lvds_mode_fixup(struct drm_encoder *encoder,
 
 	/* Native modes don't need fitting */
 	if (adjusted_mode->hdisplay == mode->hdisplay &&
-			adjusted_mode->vdisplay == mode->vdisplay) {
-		pfit_pgm_ratios = 0;
-		border = 0;
+	    adjusted_mode->vdisplay == mode->vdisplay)
 		goto out;
-	}
 
 	/* full screen scale for now */
 	if (HAS_PCH_SPLIT(dev))
@@ -240,25 +298,9 @@ static bool intel_lvds_mode_fixup(struct drm_encoder *encoder,
 
 	/* 965+ wants fuzzy fitting */
 	if (IS_I965G(dev))
-		pfit_control |= (intel_crtc->pipe << PFIT_PIPE_SHIFT) |
-					PFIT_FILTER_FUZZY;
-
-	hsync_width = adjusted_mode->crtc_hsync_end -
-					adjusted_mode->crtc_hsync_start;
-	vsync_width = adjusted_mode->crtc_vsync_end -
-					adjusted_mode->crtc_vsync_start;
-	hblank_width = adjusted_mode->crtc_hblank_end -
-					adjusted_mode->crtc_hblank_start;
-	vblank_width = adjusted_mode->crtc_vblank_end -
-					adjusted_mode->crtc_vblank_start;
-	/*
-	 * Deal with panel fitting options. Figure out how to stretch the
-	 * image based on its aspect ratio & the current panel fitting mode.
-	 */
-	panel_ratio = adjusted_mode->hdisplay * PANEL_RATIO_FACTOR /
-				adjusted_mode->vdisplay;
-	desired_ratio = mode->hdisplay * PANEL_RATIO_FACTOR /
-				mode->vdisplay;
+		pfit_control |= ((intel_crtc->pipe << PFIT_PIPE_SHIFT) |
+				 PFIT_FILTER_FUZZY);
+
 	/*
 	 * Enable automatic panel scaling for non-native modes so that they fill
 	 * the screen.  Should be enabled before the pipe is enabled, according
@@ -276,170 +318,67 @@ static bool intel_lvds_mode_fixup(struct drm_encoder *encoder,
 		 * For centered modes, we have to calculate border widths &
 		 * heights and modify the values programmed into the CRTC.
 		 */
-		left_border = (adjusted_mode->hdisplay - mode->hdisplay) / 2;
-		right_border = left_border;
-		if (mode->hdisplay & 1)
-			right_border++;
-		top_border = (adjusted_mode->vdisplay - mode->vdisplay) / 2;
-		bottom_border = top_border;
-		if (mode->vdisplay & 1)
-			bottom_border++;
-		/* Set active & border values */
-		adjusted_mode->crtc_hdisplay = mode->hdisplay;
-		/* Keep the boder be even */
-		if (right_border & 1)
-			right_border++;
-		/* use the border directly instead of border minuse one */
-		adjusted_mode->crtc_hblank_start = mode->hdisplay +
-						right_border;
-		/* keep the blank width constant */
-		adjusted_mode->crtc_hblank_end =
-			adjusted_mode->crtc_hblank_start + hblank_width;
-		/* get the hsync pos relative to hblank start */
-		hsync_pos = (hblank_width - hsync_width) / 2;
-		/* keep the hsync pos be even */
-		if (hsync_pos & 1)
-			hsync_pos++;
-		adjusted_mode->crtc_hsync_start =
-				adjusted_mode->crtc_hblank_start + hsync_pos;
-		/* keep the hsync width constant */
-		adjusted_mode->crtc_hsync_end =
-				adjusted_mode->crtc_hsync_start + hsync_width;
-		adjusted_mode->crtc_vdisplay = mode->vdisplay;
-		/* use the border instead of border minus one */
-		adjusted_mode->crtc_vblank_start = mode->vdisplay +
-						bottom_border;
-		/* keep the vblank width constant */
-		adjusted_mode->crtc_vblank_end =
-				adjusted_mode->crtc_vblank_start + vblank_width;
-		/* get the vsync start postion relative to vblank start */
-		vsync_pos = (vblank_width - vsync_width) / 2;
-		adjusted_mode->crtc_vsync_start =
-				adjusted_mode->crtc_vblank_start + vsync_pos;
-		/* keep the vsync width constant */
-		adjusted_mode->crtc_vsync_end =
-				adjusted_mode->crtc_vsync_start + vsync_width;
-		border = 1;
+		centre_horizontally(adjusted_mode, mode->hdisplay);
+		centre_vertically(adjusted_mode, mode->vdisplay);
+		border = LVDS_BORDER_ENABLE;
 		break;
+
 	case DRM_MODE_SCALE_ASPECT:
-		/* Scale but preserve the spect ratio */
-		pfit_control |= PFIT_ENABLE;
+		/* Scale but preserve the aspect ratio */
 		if (IS_I965G(dev)) {
+			u32 scaled_width = adjusted_mode->hdisplay * mode->vdisplay;
+			u32 scaled_height = mode->hdisplay * adjusted_mode->vdisplay;
+
+			pfit_control |= PFIT_ENABLE;
 			/* 965+ is easy, it does everything in hw */
-			if (panel_ratio > desired_ratio)
+			if (scaled_width > scaled_height)
 				pfit_control |= PFIT_SCALING_PILLAR;
-			else if (panel_ratio < desired_ratio)
+			else if (scaled_width < scaled_height)
 				pfit_control |= PFIT_SCALING_LETTER;
 			else
 				pfit_control |= PFIT_SCALING_AUTO;
 		} else {
+			u32 scaled_width = adjusted_mode->hdisplay * mode->vdisplay;
+			u32 scaled_height = mode->hdisplay * adjusted_mode->vdisplay;
+
 			/*
 			 * For earlier chips we have to calculate the scaling
 			 * ratio by hand and program it into the
 			 * PFIT_PGM_RATIO register
 			 */
-			u32 horiz_bits, vert_bits, bits = 12;
-			horiz_ratio = mode->hdisplay * PANEL_RATIO_FACTOR/
-						adjusted_mode->hdisplay;
-			vert_ratio = mode->vdisplay * PANEL_RATIO_FACTOR/
-						adjusted_mode->vdisplay;
-			horiz_scale = adjusted_mode->hdisplay *
-					PANEL_RATIO_FACTOR / mode->hdisplay;
-			vert_scale = adjusted_mode->vdisplay *
-					PANEL_RATIO_FACTOR / mode->vdisplay;
-
-			/* retain aspect ratio */
-			if (panel_ratio > desired_ratio) { /* Pillar */
-				u32 scaled_width;
-				scaled_width = mode->hdisplay * vert_scale /
-						PANEL_RATIO_FACTOR;
-				horiz_ratio = vert_ratio;
-				pfit_control |= (VERT_AUTO_SCALE |
-						 VERT_INTERP_BILINEAR |
-						 HORIZ_INTERP_BILINEAR);
-				/* Pillar will have left/right borders */
-				left_border = (adjusted_mode->hdisplay -
-						scaled_width) / 2;
-				right_border = left_border;
-				if (mode->hdisplay & 1) /* odd resolutions */
-					right_border++;
-				/* keep the border be even */
-				if (right_border & 1)
-					right_border++;
-				adjusted_mode->crtc_hdisplay = scaled_width;
-				/* use border instead of border minus one */
-				adjusted_mode->crtc_hblank_start =
-					scaled_width + right_border;
-				/* keep the hblank width constant */
-				adjusted_mode->crtc_hblank_end =
-					adjusted_mode->crtc_hblank_start +
-							hblank_width;
-				/*
-				 * get the hsync start pos relative to
-				 * hblank start
-				 */
-				hsync_pos = (hblank_width - hsync_width) / 2;
-				/* keep the hsync_pos be even */
-				if (hsync_pos & 1)
-					hsync_pos++;
-				adjusted_mode->crtc_hsync_start =
-					adjusted_mode->crtc_hblank_start +
-							hsync_pos;
-				/* keept hsync width constant */
-				adjusted_mode->crtc_hsync_end =
-					adjusted_mode->crtc_hsync_start +
-							hsync_width;
-				border = 1;
-			} else if (panel_ratio < desired_ratio) { /* letter */
-				u32 scaled_height = mode->vdisplay *
-					horiz_scale / PANEL_RATIO_FACTOR;
-				vert_ratio = horiz_ratio;
-				pfit_control |= (HORIZ_AUTO_SCALE |
-						 VERT_INTERP_BILINEAR |
-						 HORIZ_INTERP_BILINEAR);
-				/* Letterbox will have top/bottom border */
-				top_border = (adjusted_mode->vdisplay -
-					scaled_height) / 2;
-				bottom_border = top_border;
-				if (mode->vdisplay & 1)
-					bottom_border++;
-				adjusted_mode->crtc_vdisplay = scaled_height;
-				/* use border instead of border minus one */
-				adjusted_mode->crtc_vblank_start =
-					scaled_height + bottom_border;
-				/* keep the vblank width constant */
-				adjusted_mode->crtc_vblank_end =
-					adjusted_mode->crtc_vblank_start +
-							vblank_width;
-				/*
-				 * get the vsync start pos relative to
-				 * vblank start
-				 */
-				vsync_pos = (vblank_width - vsync_width) / 2;
-				adjusted_mode->crtc_vsync_start =
-					adjusted_mode->crtc_vblank_start +
-							vsync_pos;
-				/* keep the vsync width constant */
-				adjusted_mode->crtc_vsync_end =
-					adjusted_mode->crtc_vsync_start +
-							vsync_width;
-				border = 1;
-			} else {
-			/* Aspects match, Let hw scale both directions */
-				pfit_control |= (VERT_AUTO_SCALE |
-						 HORIZ_AUTO_SCALE |
+
+			if (scaled_width > scaled_height) { /* Pillar */
+				centre_horizontally(adjusted_mode, scaled_height / mode->vdisplay);
+
+				border = LVDS_BORDER_ENABLE;
+				pfit_control |= VERT_AUTO_SCALE;
+				if (mode->vdisplay != adjusted_mode->vdisplay) {
+					u32 bits = panel_fitter_scaling(mode->vdisplay, adjusted_mode->vdisplay);
+					pfit_pgm_ratios |= (bits << PFIT_HORIZ_SCALE_SHIFT |
+							    bits << PFIT_VERT_SCALE_SHIFT);
+					pfit_control |= (PFIT_ENABLE |
+							 VERT_INTERP_BILINEAR |
+							 HORIZ_INTERP_BILINEAR);
+				}
+			} else if (scaled_width < scaled_height) { /* letter */
+				centre_vertically(adjusted_mode, scaled_width / mode->hdisplay);
+
+				border = LVDS_BORDER_ENABLE;
+				pfit_control |= HORIZ_AUTO_SCALE;
+				if (mode->hdisplay != adjusted_mode->hdisplay) {
+					u32 bits = panel_fitter_scaling(mode->hdisplay, adjusted_mode->hdisplay);
+					pfit_pgm_ratios |= (bits << PFIT_HORIZ_SCALE_SHIFT |
+							    bits << PFIT_VERT_SCALE_SHIFT);
+					pfit_control |= (PFIT_ENABLE |
+							 VERT_INTERP_BILINEAR |
+							 HORIZ_INTERP_BILINEAR);
+				}
+			} else
+				/* Aspects match, Let hw scale both directions */
+				pfit_control |= (PFIT_ENABLE |
+						 VERT_AUTO_SCALE | HORIZ_AUTO_SCALE |
 						 VERT_INTERP_BILINEAR |
 						 HORIZ_INTERP_BILINEAR);
-			}
-			horiz_bits = (1 << bits) * horiz_ratio /
-					PANEL_RATIO_FACTOR;
-			vert_bits = (1 << bits) * vert_ratio /
-					PANEL_RATIO_FACTOR;
-			pfit_pgm_ratios =
-				((vert_bits << PFIT_VERT_SCALE_SHIFT) &
-						PFIT_VERT_SCALE_MASK) |
-				((horiz_bits << PFIT_HORIZ_SCALE_SHIFT) &
-						PFIT_HORIZ_SCALE_MASK);
 		}
 		break;
 
@@ -456,6 +395,7 @@ static bool intel_lvds_mode_fixup(struct drm_encoder *encoder,
 					 VERT_INTERP_BILINEAR |
 					 HORIZ_INTERP_BILINEAR);
 		break;
+
 	default:
 		break;
 	}
@@ -463,14 +403,8 @@ static bool intel_lvds_mode_fixup(struct drm_encoder *encoder,
 out:
 	lvds_priv->pfit_control = pfit_control;
 	lvds_priv->pfit_pgm_ratios = pfit_pgm_ratios;
-	/*
-	 * When there exists the border, it means that the LVDS_BORDR
-	 * should be enabled.
-	 */
-	if (border)
-		dev_priv->lvds_border_bits |= LVDS_BORDER_ENABLE;
-	else
-		dev_priv->lvds_border_bits &= ~(LVDS_BORDER_ENABLE);
+	dev_priv->lvds_border_bits = border;
+
 	/*
 	 * XXX: It would be nice to support lower refresh rates on the
 	 * panels to reduce power consumption, and perhaps match the
-- 
1.7.1

Re: [PATCH 2/2] drm/i915: Refactor panel fitting on the LVDS.

[Chris Wilson]

On Sat, 17 Jul 2010 13:38:44 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> +static inline u32 panel_fitter_scaling(u32 source, u32 target)
> +{
> +	/*
> +	 * Floating point operation is not supported. So the FACTOR
> +	 * is defined, which can avoid the floating point computation
> +	 * when calculating the panel ratio.
> +	 */
> +#define ACCURACY 12
> +#define FACTOR (1 << ACCURACY)
> +	u32 ratio = source * FACTOR / target;
> +	return (FACTOR * (ratio + FACTOR/2)) / FACTOR;
> +}

Rounding went wrong here. Should be
  return (FACTOR * ratio + FACTOR/2) / FACTOR;

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Refactor panel fitting on the LVDS.

[Jesse Barnes]

On Sat, 17 Jul 2010 13:38:44 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Move the common routines into separate functions to not only increase
> readability, but also throwaway surplus code.
> 
> In doing so, we review the calculation of the aspect preserving
> scaling and avoid the use of fixed-point until we need to calculate
> the accurate scale factor.
> 
> 1 files changed, 128 insertions(+), 194 deletions(-)

Nice cleanups, that function was getting too big...  misc nitpicks
below.

> +	/* keep the border be even */
> +	if (border & 1)
> +		border++;

May as well fix the comment here or combine it with the above.  There
are a few other comments like this elsewhere too.

While you're in there it would also be cool to write a small CRTC mode
verification function that checks the htotal/vtotal/etc against the
constraints documented in the manual (e.g. sync before end, active
divisible by 2 in ganged mode, etc).  If you're feeling adventurous
anyway. :)

Other than that:

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/2] drm/i915: Refactor panel fitting on the LVDS.

[Chris Wilson]

On Sat, 17 Jul 2010 18:15:46 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> While you're in there it would also be cool to write a small CRTC mode
> verification function that checks the htotal/vtotal/etc against the
> constraints documented in the manual (e.g. sync before end, active
> divisible by 2 in ganged mode, etc).  If you're feeling adventurous
> anyway. :)

Sounds like a useful task, both for verification of the code and for
forcing me to read the documentation closely ;-)

/me adds to his todo list.

-- 
Chris Wilson, Intel Open Source Technology Centre