[PATCH V2 2/3] OMAPDSS: DISPC: Handle synclost errors in OMAP3

[PATCH V2 2/3] OMAPDSS: DISPC: Handle synclost errors in OMAP3

[Chandrabhanu Mahapatra]

In OMAP3 DISPC video overlays suffer from some undocumented horizontal position
and timing related limitations leading to SYNCLOST errors. Whenever the image
window is moved towards the right of the screen SYNCLOST errors become
frequent. Checks have been implemented to see that DISPC driver rejects
configuration exceeding above limitations.

This code was successfully tested on OMAP3. This code is written based on code
written by Ville Syrj채l채 <ville.syrjala@nokia.com> in Linux OMAP kernel. Ville
Syrj채l채 <ville.syrjala@nokia.com> had added checks for video overlay horizontal
timing and DISPC horizontal blanking length limitations.

Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
---
 drivers/video/omap2/dss/dispc.c |   97 +++++++++++++++++++++++++++++----------
 1 files changed, 72 insertions(+), 25 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 5a1963e..d8a1672 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -1622,6 +1622,57 @@ static void calc_dma_rotation_offset(u8 rotation, bool mirror,
 	}
 }
 
+static int check_horiz_timing(enum omap_channel channel, u16 pos_x,
+		u16 width, u16 height, u16 out_width, u16 out_height)
+{
+	int DS = DIV_ROUND_UP(height, out_height);
+	struct omap_dss_device *dssdev = dispc_mgr_get_device(channel);
+	struct omap_video_timings t = dssdev->panel.timings;
+	unsigned long nonactive, lclk, pclk;
+	static const u8 limits[3] = { 8, 10, 20 };
+	u64 val, blank;
+	int i;
+
+	nonactive = t.x_res + t.hfp + t.hsw + t.hbp - out_width;
+	pclk = dispc_mgr_pclk_rate(channel);
+	lclk = dispc_mgr_lclk_rate(channel);
+
+	i = 0;
+	if (out_height < height)
+		i++;
+	if (out_width < width)
+		i++;
+	blank = div_u64((u64)(t.hbp + t.hsw + t.hfp) * lclk, pclk);
+	DSSDBG("blanking period + ppl = %llu (limit = %u)\n", blank, limits[i]);
+	if (blank <= limits[i])
+		return -EINVAL;
+
+	/*
+	 * Pixel data should be prepared before visible display point starts.
+	 * So, atleast DS-2 lines must have already been fetched by DISPC
+	 * during nonactive - pos_x period.
+	 */
+	val = div_u64((u64)(nonactive - pos_x) * lclk, pclk);
+	DSSDBG("(nonactive - pos_x) * pcd = %llu,"
+		" max(0, DS - 2) * width = %d\n",
+		val, max(0, DS - 2) * width);
+	if (val < max(0, DS - 2) * width)
+		return -EINVAL;
+
+	/*
+	 * All lines need to be refilled during the nonactive period of which
+	 * only one line can be loaded during the active period. So, atleast
+	 * DS - 1 lines should be loaded during nonactive period.
+	 */
+	val =  div_u64((u64)nonactive * lclk, pclk);
+	DSSDBG("nonactive * pcd  = %llu, max(0, DS - 1) * width = %d\n",
+		val, max(0, DS - 1) * width);
+	if (val < max(0, DS - 1) * width)
+		return -EINVAL;
+
+	return 0;
+}
+
 static unsigned long calc_fclk_five_taps(enum omap_channel channel, u16 width,
 		u16 height, u16 out_width, u16 out_height,
 		enum omap_color_mode color_mode)
@@ -1702,7 +1753,7 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
 		enum omap_channel channel, u16 width, u16 height,
 		u16 out_width, u16 out_height,
 		enum omap_color_mode color_mode, bool *five_taps,
-		int *x_predecim, int *y_predecim)
+		int *x_predecim, int *y_predecim, u16 pos_x)
 {
 	struct omap_overlay *ovl = omap_dss_get_overlay(plane);
 	const int maxdownscale = dss_feat_get_param_max(FEAT_PARAM_DOWNSCALE);
@@ -1778,6 +1829,9 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
 			fclk = calc_fclk_five_taps(channel, in_width, in_height,
 				out_width, out_height, color_mode);
 
+			error = check_horiz_timing(channel, pos_x, in_width,
+				in_height, out_width, out_height);
+
 			if (in_width > maxsinglelinewidth)
 				if (in_height > out_height &&
 					in_height < out_height * 2)
@@ -1785,7 +1839,7 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
 			if (!*five_taps)
 				fclk = calc_fclk(channel, in_width, in_height,
 					out_width, out_height);
-			error = (in_width > maxsinglelinewidth * 2 ||
+			error = (error || in_width > maxsinglelinewidth * 2 ||
 				(in_width > maxsinglelinewidth && *five_taps) ||
 				!fclk || fclk > dispc_fclk_rate());
 			if (error) {
@@ -1801,6 +1855,12 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
 		} while (decim_x <= *x_predecim && decim_y <= *y_predecim
 			&& error);
 
+		if (check_horiz_timing(channel, pos_x, width, height,
+			out_width, out_height)){
+				DSSERR("horizontal timing too tight\n");
+				return -EINVAL;
+		}
+
 		if (in_width > (maxsinglelinewidth * 2)) {
 			DSSERR("Cannot setup scaling");
 			DSSERR("width exceeds maximum width possible");
@@ -1901,7 +1961,7 @@ int dispc_ovl_setup(enum omap_plane plane, struct omap_overlay_info *oi,
 
 	r = dispc_ovl_calc_scaling(plane, channel, in_width, in_height,
 			out_width, out_height, oi->color_mode, &five_taps,
-			&x_predecim, &y_predecim);
+			&x_predecim, &y_predecim, oi->pos_x);
 	if (r)
 		return r;
 
@@ -2472,32 +2532,19 @@ unsigned long dispc_fclk_rate(void)
 
 unsigned long dispc_mgr_lclk_rate(enum omap_channel channel)
 {
-	struct platform_device *dsidev;
-	int lcd;
-	unsigned long r;
-	u32 l;
+	unsigned long r = dispc_fclk_rate();
 
-	l = dispc_read_reg(DISPC_DIVISORo(channel));
+	if (dispc_mgr_is_lcd(channel)) {
+		u32 l;
+		int lcd;
 
-	lcd = FLD_GET(l, 23, 16);
+		l = dispc_read_reg(DISPC_DIVISORo(channel));
+		lcd = FLD_GET(l, 23, 16);
 
-	switch (dss_get_lcd_clk_source(channel)) {
-	case OMAP_DSS_CLK_SRC_FCK:
-		r = clk_get_rate(dispc.dss_clk);
-		break;
-	case OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC:
-		dsidev = dsi_get_dsidev_from_id(0);
-		r = dsi_get_pll_hsdiv_dispc_rate(dsidev);
-		break;
-	case OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC:
-		dsidev = dsi_get_dsidev_from_id(1);
-		r = dsi_get_pll_hsdiv_dispc_rate(dsidev);
-		break;
-	default:
-		BUG();
+		return r / lcd;
+	} else {
+		return r;
 	}
-
-	return r / lcd;
 }
 
 unsigned long dispc_mgr_pclk_rate(enum omap_channel channel)
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/3] OMAPDSS: DISPC: Handle synclost errors in OMAP3

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 9900 bytes --]

Hi,

On Wed, 2012-03-21 at 15:21 +0530, Chandrabhanu Mahapatra wrote:
> In OMAP3 DISPC video overlays suffer from some undocumented horizontal position
> and timing related limitations leading to SYNCLOST errors. Whenever the image
> window is moved towards the right of the screen SYNCLOST errors become
> frequent. Checks have been implemented to see that DISPC driver rejects
> configuration exceeding above limitations.
> 
> This code was successfully tested on OMAP3. This code is written based on code
> written by Ville Syrjälä <ville.syrjala@nokia.com> in Linux OMAP kernel. Ville
> Syrjälä <ville.syrjala@nokia.com> had added checks for video overlay horizontal
> timing and DISPC horizontal blanking length limitations.
> 
> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
> ---
>  drivers/video/omap2/dss/dispc.c |   97 +++++++++++++++++++++++++++++----------
>  1 files changed, 72 insertions(+), 25 deletions(-)

The patch seems to be using dos line endings... Where did they come
from? The two others are fine. I think git am removed the dos line
endings, so it's fine.

 Tomi


ERROR: DOS line endings
#72: FILE: drivers/video/omap2/dss/dispc.c:1625:
+static int check_horiz_timing(enum omap_channel channel, u16 pos_x,^M$

ERROR: DOS line endings
#73: FILE: drivers/video/omap2/dss/dispc.c:1626:
+^I^Iu16 width, u16 height, u16 out_width, u16 out_height)^M$

ERROR: DOS line endings
#74: FILE: drivers/video/omap2/dss/dispc.c:1627:
+{^M$

ERROR: DOS line endings
#75: FILE: drivers/video/omap2/dss/dispc.c:1628:
+^Iint DS = DIV_ROUND_UP(height, out_height);^M$

ERROR: DOS line endings
#76: FILE: drivers/video/omap2/dss/dispc.c:1629:
+^Istruct omap_dss_device *dssdev = dispc_mgr_get_device(channel);^M$

ERROR: DOS line endings
#77: FILE: drivers/video/omap2/dss/dispc.c:1630:
+^Istruct omap_video_timings t = dssdev->panel.timings;^M$

ERROR: DOS line endings
#78: FILE: drivers/video/omap2/dss/dispc.c:1631:
+^Iunsigned long nonactive, lclk, pclk;^M$

ERROR: DOS line endings
#79: FILE: drivers/video/omap2/dss/dispc.c:1632:
+^Istatic const u8 limits[3] = { 8, 10, 20 };^M$

ERROR: DOS line endings
#80: FILE: drivers/video/omap2/dss/dispc.c:1633:
+^Iu64 val, blank;^M$

ERROR: DOS line endings
#81: FILE: drivers/video/omap2/dss/dispc.c:1634:
+^Iint i;^M$

ERROR: DOS line endings
#82: FILE: drivers/video/omap2/dss/dispc.c:1635:
+^M$

ERROR: DOS line endings
#83: FILE: drivers/video/omap2/dss/dispc.c:1636:
+^Inonactive = t.x_res + t.hfp + t.hsw + t.hbp - out_width;^M$

ERROR: DOS line endings
#84: FILE: drivers/video/omap2/dss/dispc.c:1637:
+^Ipclk = dispc_mgr_pclk_rate(channel);^M$

ERROR: DOS line endings
#85: FILE: drivers/video/omap2/dss/dispc.c:1638:
+^Ilclk = dispc_mgr_lclk_rate(channel);^M$

ERROR: DOS line endings
#86: FILE: drivers/video/omap2/dss/dispc.c:1639:
+^M$

ERROR: DOS line endings
#87: FILE: drivers/video/omap2/dss/dispc.c:1640:
+^Ii = 0;^M$

ERROR: DOS line endings
#88: FILE: drivers/video/omap2/dss/dispc.c:1641:
+^Iif (out_height < height)^M$

ERROR: DOS line endings
#89: FILE: drivers/video/omap2/dss/dispc.c:1642:
+^I^Ii++;^M$

ERROR: DOS line endings
#90: FILE: drivers/video/omap2/dss/dispc.c:1643:
+^Iif (out_width < width)^M$

ERROR: DOS line endings
#91: FILE: drivers/video/omap2/dss/dispc.c:1644:
+^I^Ii++;^M$

ERROR: DOS line endings
#92: FILE: drivers/video/omap2/dss/dispc.c:1645:
+^Iblank = div_u64((u64)(t.hbp + t.hsw + t.hfp) * lclk, pclk);^M$

ERROR: DOS line endings
#93: FILE: drivers/video/omap2/dss/dispc.c:1646:
+^IDSSDBG("blanking period + ppl = %llu (limit = %u)\n", blank,
limits[i]);^M$

WARNING: line over 80 characters
#93: FILE: drivers/video/omap2/dss/dispc.c:1646:
+	DSSDBG("blanking period + ppl = %llu (limit = %u)\n", blank,
limits[i]);

ERROR: DOS line endings
#94: FILE: drivers/video/omap2/dss/dispc.c:1647:
+^Iif (blank <= limits[i])^M$

ERROR: DOS line endings
#95: FILE: drivers/video/omap2/dss/dispc.c:1648:
+^I^Ireturn -EINVAL;^M$

ERROR: DOS line endings
#96: FILE: drivers/video/omap2/dss/dispc.c:1649:
+^M$

ERROR: trailing whitespace
#97: FILE: drivers/video/omap2/dss/dispc.c:1650:
+^I/*^M$

ERROR: trailing whitespace
#98: FILE: drivers/video/omap2/dss/dispc.c:1651:
+^I * Pixel data should be prepared before visible display point
starts.^M$

ERROR: trailing whitespace
#99: FILE: drivers/video/omap2/dss/dispc.c:1652:
+^I * So, atleast DS-2 lines must have already been fetched by DISPC^M$

ERROR: trailing whitespace
#100: FILE: drivers/video/omap2/dss/dispc.c:1653:
+^I * during nonactive - pos_x period.^M$

ERROR: DOS line endings
#101: FILE: drivers/video/omap2/dss/dispc.c:1654:
+^I */^M$

ERROR: DOS line endings
#102: FILE: drivers/video/omap2/dss/dispc.c:1655:
+^Ival = div_u64((u64)(nonactive - pos_x) * lclk, pclk);^M$

ERROR: DOS line endings
#103: FILE: drivers/video/omap2/dss/dispc.c:1656:
+^IDSSDBG("(nonactive - pos_x) * pcd = %llu,"^M$

ERROR: DOS line endings
#104: FILE: drivers/video/omap2/dss/dispc.c:1657:
+^I^I" max(0, DS - 2) * width = %d\n",^M$

ERROR: DOS line endings
#105: FILE: drivers/video/omap2/dss/dispc.c:1658:
+^I^Ival, max(0, DS - 2) * width);^M$

ERROR: DOS line endings
#106: FILE: drivers/video/omap2/dss/dispc.c:1659:
+^Iif (val < max(0, DS - 2) * width)^M$

ERROR: DOS line endings
#107: FILE: drivers/video/omap2/dss/dispc.c:1660:
+^I^Ireturn -EINVAL;^M$

ERROR: DOS line endings
#108: FILE: drivers/video/omap2/dss/dispc.c:1661:
+^M$

ERROR: trailing whitespace
#109: FILE: drivers/video/omap2/dss/dispc.c:1662:
+^I/*^M$

ERROR: trailing whitespace
#110: FILE: drivers/video/omap2/dss/dispc.c:1663:
+^I * All lines need to be refilled during the nonactive period of
which^M$

ERROR: trailing whitespace
#111: FILE: drivers/video/omap2/dss/dispc.c:1664:
+^I * only one line can be loaded during the active period. So,
atleast^M$

ERROR: trailing whitespace
#112: FILE: drivers/video/omap2/dss/dispc.c:1665:
+^I * DS - 1 lines should be loaded during nonactive period.^M$

ERROR: DOS line endings
#113: FILE: drivers/video/omap2/dss/dispc.c:1666:
+^I */^M$

ERROR: DOS line endings
#114: FILE: drivers/video/omap2/dss/dispc.c:1667:
+^Ival =  div_u64((u64)nonactive * lclk, pclk);^M$

ERROR: DOS line endings
#115: FILE: drivers/video/omap2/dss/dispc.c:1668:
+^IDSSDBG("nonactive * pcd  = %llu, max(0, DS - 1) * width = %d\n",^M$

ERROR: DOS line endings
#116: FILE: drivers/video/omap2/dss/dispc.c:1669:
+^I^Ival, max(0, DS - 1) * width);^M$

ERROR: DOS line endings
#117: FILE: drivers/video/omap2/dss/dispc.c:1670:
+^Iif (val < max(0, DS - 1) * width)^M$

ERROR: DOS line endings
#118: FILE: drivers/video/omap2/dss/dispc.c:1671:
+^I^Ireturn -EINVAL;^M$

ERROR: DOS line endings
#119: FILE: drivers/video/omap2/dss/dispc.c:1672:
+^M$

ERROR: DOS line endings
#120: FILE: drivers/video/omap2/dss/dispc.c:1673:
+^Ireturn 0;^M$

ERROR: DOS line endings
#121: FILE: drivers/video/omap2/dss/dispc.c:1674:
+}^M$

ERROR: DOS line endings
#122: FILE: drivers/video/omap2/dss/dispc.c:1675:
+^M$

ERROR: DOS line endings
#131: FILE: drivers/video/omap2/dss/dispc.c:1756:
+^I^Iint *x_predecim, int *y_predecim, u16 pos_x)^M$

ERROR: DOS line endings
#139: FILE: drivers/video/omap2/dss/dispc.c:1832:
+^I^I^Ierror = check_horiz_timing(channel, pos_x, in_width,^M$

ERROR: DOS line endings
#140: FILE: drivers/video/omap2/dss/dispc.c:1833:
+^I^I^I^Iin_height, out_width, out_height);^M$

ERROR: DOS line endings
#141: FILE: drivers/video/omap2/dss/dispc.c:1834:
+^M$

ERROR: DOS line endings
#150: FILE: drivers/video/omap2/dss/dispc.c:1842:
+^I^I^Ierror = (error || in_width > maxsinglelinewidth * 2 ||^M$

ERROR: DOS line endings
#158: FILE: drivers/video/omap2/dss/dispc.c:1858:
+^I^Iif (check_horiz_timing(channel, pos_x, width, height,^M$

ERROR: DOS line endings
#159: FILE: drivers/video/omap2/dss/dispc.c:1859:
+^I^I^Iout_width, out_height)){^M$

ERROR: DOS line endings
#160: FILE: drivers/video/omap2/dss/dispc.c:1860:
+^I^I^I^IDSSERR("horizontal timing too tight\n");^M$

ERROR: DOS line endings
#161: FILE: drivers/video/omap2/dss/dispc.c:1861:
+^I^I^I^Ireturn -EINVAL;^M$

ERROR: DOS line endings
#162: FILE: drivers/video/omap2/dss/dispc.c:1862:
+^I^I}^M$

ERROR: DOS line endings
#163: FILE: drivers/video/omap2/dss/dispc.c:1863:
+^M$

ERROR: DOS line endings
#172: FILE: drivers/video/omap2/dss/dispc.c:1964:
+^I^I^I&x_predecim, &y_predecim, oi->pos_x);^M$

ERROR: DOS line endings
#184: FILE: drivers/video/omap2/dss/dispc.c:2535:
+^Iunsigned long r = dispc_fclk_rate();^M$

ERROR: DOS line endings
#187: FILE: drivers/video/omap2/dss/dispc.c:2537:
+^Iif (dispc_mgr_is_lcd(channel)) {^M$

ERROR: DOS line endings
#188: FILE: drivers/video/omap2/dss/dispc.c:2538:
+^I^Iu32 l;^M$

ERROR: DOS line endings
#189: FILE: drivers/video/omap2/dss/dispc.c:2539:
+^I^Iint lcd;^M$

ERROR: DOS line endings
#192: FILE: drivers/video/omap2/dss/dispc.c:2541:
+^I^Il = dispc_read_reg(DISPC_DIVISORo(channel));^M$

ERROR: DOS line endings
#193: FILE: drivers/video/omap2/dss/dispc.c:2542:
+^I^Ilcd = FLD_GET(l, 23, 16);^M$

ERROR: DOS line endings
#209: FILE: drivers/video/omap2/dss/dispc.c:2544:
+^I^Ireturn r / lcd;^M$

ERROR: DOS line endings
#210: FILE: drivers/video/omap2/dss/dispc.c:2545:
+^I} else {^M$

ERROR: DOS line endings
#211: FILE: drivers/video/omap2/dss/dispc.c:2546:
+^I^Ireturn r;^M$

total: 72 errors, 1 warnings, 143 lines checked

NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch
or
      scripts/cleanfile

123.mbox has style problems, please review.

If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2 2/3] OMAPDSS: DISPC: Handle synclost errors in OMAP3

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 6816 bytes --]

On Wed, 2012-03-21 at 15:21 +0530, Chandrabhanu Mahapatra wrote:
> In OMAP3 DISPC video overlays suffer from some undocumented horizontal position
> and timing related limitations leading to SYNCLOST errors. Whenever the image
> window is moved towards the right of the screen SYNCLOST errors become
> frequent. Checks have been implemented to see that DISPC driver rejects
> configuration exceeding above limitations.
> 
> This code was successfully tested on OMAP3. This code is written based on code
> written by Ville Syrjälä <ville.syrjala@nokia.com> in Linux OMAP kernel. Ville
> Syrjälä <ville.syrjala@nokia.com> had added checks for video overlay horizontal
> timing and DISPC horizontal blanking length limitations.
> 
> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
> ---
>  drivers/video/omap2/dss/dispc.c |   97 +++++++++++++++++++++++++++++----------
>  1 files changed, 72 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
> index 5a1963e..d8a1672 100644
> --- a/drivers/video/omap2/dss/dispc.c
> +++ b/drivers/video/omap2/dss/dispc.c
> @@ -1622,6 +1622,57 @@ static void calc_dma_rotation_offset(u8 rotation, bool mirror,
>  	}
>  }
>  
> +static int check_horiz_timing(enum omap_channel channel, u16 pos_x,
> +		u16 width, u16 height, u16 out_width, u16 out_height)

I think the function could be named check_horiz_timing_omap3 or
something. It's kinda hard to realize that this is an omap3 work-around.
Perhaps a short comment above the function would be in order.

> +{
> +	int DS = DIV_ROUND_UP(height, out_height);
> +	struct omap_dss_device *dssdev = dispc_mgr_get_device(channel);
> +	struct omap_video_timings t = dssdev->panel.timings;
> +	unsigned long nonactive, lclk, pclk;
> +	static const u8 limits[3] = { 8, 10, 20 };
> +	u64 val, blank;
> +	int i;
> +
> +	nonactive = t.x_res + t.hfp + t.hsw + t.hbp - out_width;
> +	pclk = dispc_mgr_pclk_rate(channel);
> +	lclk = dispc_mgr_lclk_rate(channel);
> +
> +	i = 0;
> +	if (out_height < height)
> +		i++;
> +	if (out_width < width)
> +		i++;
> +	blank = div_u64((u64)(t.hbp + t.hsw + t.hfp) * lclk, pclk);
> +	DSSDBG("blanking period + ppl = %llu (limit = %u)\n", blank, limits[i]);
> +	if (blank <= limits[i])
> +		return -EINVAL;
> +
> +	/*
> +	 * Pixel data should be prepared before visible display point starts.
> +	 * So, atleast DS-2 lines must have already been fetched by DISPC
> +	 * during nonactive - pos_x period.
> +	 */
> +	val = div_u64((u64)(nonactive - pos_x) * lclk, pclk);
> +	DSSDBG("(nonactive - pos_x) * pcd = %llu,"
> +		" max(0, DS - 2) * width = %d\n",
> +		val, max(0, DS - 2) * width);
> +	if (val < max(0, DS - 2) * width)
> +		return -EINVAL;
> +
> +	/*
> +	 * All lines need to be refilled during the nonactive period of which
> +	 * only one line can be loaded during the active period. So, atleast
> +	 * DS - 1 lines should be loaded during nonactive period.
> +	 */
> +	val =  div_u64((u64)nonactive * lclk, pclk);
> +	DSSDBG("nonactive * pcd  = %llu, max(0, DS - 1) * width = %d\n",
> +		val, max(0, DS - 1) * width);
> +	if (val < max(0, DS - 1) * width)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static unsigned long calc_fclk_five_taps(enum omap_channel channel, u16 width,
>  		u16 height, u16 out_width, u16 out_height,
>  		enum omap_color_mode color_mode)
> @@ -1702,7 +1753,7 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
>  		enum omap_channel channel, u16 width, u16 height,
>  		u16 out_width, u16 out_height,
>  		enum omap_color_mode color_mode, bool *five_taps,
> -		int *x_predecim, int *y_predecim)
> +		int *x_predecim, int *y_predecim, u16 pos_x)
>  {
>  	struct omap_overlay *ovl = omap_dss_get_overlay(plane);
>  	const int maxdownscale = dss_feat_get_param_max(FEAT_PARAM_DOWNSCALE);
> @@ -1778,6 +1829,9 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
>  			fclk = calc_fclk_five_taps(channel, in_width, in_height,
>  				out_width, out_height, color_mode);
>  
> +			error = check_horiz_timing(channel, pos_x, in_width,
> +				in_height, out_width, out_height);
> +
>  			if (in_width > maxsinglelinewidth)
>  				if (in_height > out_height &&
>  					in_height < out_height * 2)
> @@ -1785,7 +1839,7 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
>  			if (!*five_taps)
>  				fclk = calc_fclk(channel, in_width, in_height,
>  					out_width, out_height);
> -			error = (in_width > maxsinglelinewidth * 2 ||
> +			error = (error || in_width > maxsinglelinewidth * 2 ||
>  				(in_width > maxsinglelinewidth && *five_taps) ||
>  				!fclk || fclk > dispc_fclk_rate());
>  			if (error) {
> @@ -1801,6 +1855,12 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
>  		} while (decim_x <= *x_predecim && decim_y <= *y_predecim
>  			&& error);
>  
> +		if (check_horiz_timing(channel, pos_x, width, height,
> +			out_width, out_height)){
> +				DSSERR("horizontal timing too tight\n");
> +				return -EINVAL;
> +		}
> +
>  		if (in_width > (maxsinglelinewidth * 2)) {
>  			DSSERR("Cannot setup scaling");
>  			DSSERR("width exceeds maximum width possible");
> @@ -1901,7 +1961,7 @@ int dispc_ovl_setup(enum omap_plane plane, struct omap_overlay_info *oi,
>  
>  	r = dispc_ovl_calc_scaling(plane, channel, in_width, in_height,
>  			out_width, out_height, oi->color_mode, &five_taps,
> -			&x_predecim, &y_predecim);
> +			&x_predecim, &y_predecim, oi->pos_x);
>  	if (r)
>  		return r;
>  
> @@ -2472,32 +2532,19 @@ unsigned long dispc_fclk_rate(void)
>  
>  unsigned long dispc_mgr_lclk_rate(enum omap_channel channel)
>  {
> -	struct platform_device *dsidev;
> -	int lcd;
> -	unsigned long r;
> -	u32 l;
> +	unsigned long r = dispc_fclk_rate();
>  
> -	l = dispc_read_reg(DISPC_DIVISORo(channel));
> +	if (dispc_mgr_is_lcd(channel)) {
> +		u32 l;
> +		int lcd;
>  
> -	lcd = FLD_GET(l, 23, 16);
> +		l = dispc_read_reg(DISPC_DIVISORo(channel));
> +		lcd = FLD_GET(l, 23, 16);
>  
> -	switch (dss_get_lcd_clk_source(channel)) {
> -	case OMAP_DSS_CLK_SRC_FCK:
> -		r = clk_get_rate(dispc.dss_clk);
> -		break;
> -	case OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC:
> -		dsidev = dsi_get_dsidev_from_id(0);
> -		r = dsi_get_pll_hsdiv_dispc_rate(dsidev);
> -		break;
> -	case OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC:
> -		dsidev = dsi_get_dsidev_from_id(1);
> -		r = dsi_get_pll_hsdiv_dispc_rate(dsidev);
> -		break;
> -	default:
> -		BUG();
> +		return r / lcd;
> +	} else {
> +		return r;
>  	}
> -
> -	return r / lcd;
>  }

I don't the change to dispc_mgr_lclk_rate has anything to do with this
patch (fixing omap3 sync lost error). Shouldn't it be a separate fix?

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2 2/3] OMAPDSS: DISPC: Handle synclost errors in OMAP3

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1573 bytes --]

On Tue, 2012-03-27 at 16:31 +0530, Mahapatra, Chandrabhanu wrote:
> On Tue, Mar 27, 2012 at 4:13 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > Hi,
> >
> > On Wed, 2012-03-21 at 15:21 +0530, Chandrabhanu Mahapatra wrote:
> >> In OMAP3 DISPC video overlays suffer from some undocumented horizontal position
> >> and timing related limitations leading to SYNCLOST errors. Whenever the image
> >> window is moved towards the right of the screen SYNCLOST errors become
> >> frequent. Checks have been implemented to see that DISPC driver rejects
> >> configuration exceeding above limitations.
> >>
> >> This code was successfully tested on OMAP3. This code is written based on code
> >> written by Ville Syrjälä <ville.syrjala@nokia.com> in Linux OMAP kernel. Ville
> >> Syrjälä <ville.syrjala@nokia.com> had added checks for video overlay horizontal
> >> timing and DISPC horizontal blanking length limitations.
> >>
> >> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
> >> ---
> >>  drivers/video/omap2/dss/dispc.c |   97 +++++++++++++++++++++++++++++----------
> >>  1 files changed, 72 insertions(+), 25 deletions(-)
> >
> > The patch seems to be using dos line endings... Where did they come
> > from? The two others are fine. I think git am removed the dos line
> > endings, so it's fine.
> >
> 
> How did you find these errors? scripts/checkpatch.pl still returns me nothing.

Well I just saved the email and ran checkpatch. Could be a problem in
gmail or my end too, but I've never seen it happen before.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2 2/3] OMAPDSS: DISPC: Handle synclost errors in OMAP3

[Mahapatra, Chandrabhanu]

On Tue, Mar 27, 2012 at 4:13 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi,
>
> On Wed, 2012-03-21 at 15:21 +0530, Chandrabhanu Mahapatra wrote:
>> In OMAP3 DISPC video overlays suffer from some undocumented horizontal position
>> and timing related limitations leading to SYNCLOST errors. Whenever the image
>> window is moved towards the right of the screen SYNCLOST errors become
>> frequent. Checks have been implemented to see that DISPC driver rejects
>> configuration exceeding above limitations.
>>
>> This code was successfully tested on OMAP3. This code is written based on code
>> written by Ville Syrjälä <ville.syrjala@nokia.com> in Linux OMAP kernel. Ville
>> Syrjälä <ville.syrjala@nokia.com> had added checks for video overlay horizontal
>> timing and DISPC horizontal blanking length limitations.
>>
>> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
>> ---
>>  drivers/video/omap2/dss/dispc.c |   97 +++++++++++++++++++++++++++++----------
>>  1 files changed, 72 insertions(+), 25 deletions(-)
>
> The patch seems to be using dos line endings... Where did they come
> from? The two others are fine. I think git am removed the dos line
> endings, so it's fine.
>

How did you find these errors? scripts/checkpatch.pl still returns me nothing.

>  Tomi
>
>
> ERROR: DOS line endings
> #72: FILE: drivers/video/omap2/dss/dispc.c:1625:
> +static int check_horiz_timing(enum omap_channel channel, u16 pos_x,^M$
>
> ERROR: DOS line endings
> #73: FILE: drivers/video/omap2/dss/dispc.c:1626:
> +^I^Iu16 width, u16 height, u16 out_width, u16 out_height)^M$
>
> ERROR: DOS line endings
> #74: FILE: drivers/video/omap2/dss/dispc.c:1627:
> +{^M$
>
> ERROR: DOS line endings
> #75: FILE: drivers/video/omap2/dss/dispc.c:1628:
> +^Iint DS = DIV_ROUND_UP(height, out_height);^M$
>
> ERROR: DOS line endings
> #76: FILE: drivers/video/omap2/dss/dispc.c:1629:
> +^Istruct omap_dss_device *dssdev = dispc_mgr_get_device(channel);^M$
>
> ERROR: DOS line endings
> #77: FILE: drivers/video/omap2/dss/dispc.c:1630:
> +^Istruct omap_video_timings t = dssdev->panel.timings;^M$
>
> ERROR: DOS line endings
> #78: FILE: drivers/video/omap2/dss/dispc.c:1631:
> +^Iunsigned long nonactive, lclk, pclk;^M$
>
> ERROR: DOS line endings
> #79: FILE: drivers/video/omap2/dss/dispc.c:1632:
> +^Istatic const u8 limits[3] = { 8, 10, 20 };^M$
>
> ERROR: DOS line endings
> #80: FILE: drivers/video/omap2/dss/dispc.c:1633:
> +^Iu64 val, blank;^M$
>
> ERROR: DOS line endings
> #81: FILE: drivers/video/omap2/dss/dispc.c:1634:
> +^Iint i;^M$
>
> ERROR: DOS line endings
> #82: FILE: drivers/video/omap2/dss/dispc.c:1635:
> +^M$
>
> ERROR: DOS line endings
> #83: FILE: drivers/video/omap2/dss/dispc.c:1636:
> +^Inonactive = t.x_res + t.hfp + t.hsw + t.hbp - out_width;^M$
>
> ERROR: DOS line endings
> #84: FILE: drivers/video/omap2/dss/dispc.c:1637:
> +^Ipclk = dispc_mgr_pclk_rate(channel);^M$
>
> ERROR: DOS line endings
> #85: FILE: drivers/video/omap2/dss/dispc.c:1638:
> +^Ilclk = dispc_mgr_lclk_rate(channel);^M$
>
> ERROR: DOS line endings
> #86: FILE: drivers/video/omap2/dss/dispc.c:1639:
> +^M$
>
> ERROR: DOS line endings
> #87: FILE: drivers/video/omap2/dss/dispc.c:1640:
> +^Ii = 0;^M$
>
> ERROR: DOS line endings
> #88: FILE: drivers/video/omap2/dss/dispc.c:1641:
> +^Iif (out_height < height)^M$
>
> ERROR: DOS line endings
> #89: FILE: drivers/video/omap2/dss/dispc.c:1642:
> +^I^Ii++;^M$
>
> ERROR: DOS line endings
> #90: FILE: drivers/video/omap2/dss/dispc.c:1643:
> +^Iif (out_width < width)^M$
>
> ERROR: DOS line endings
> #91: FILE: drivers/video/omap2/dss/dispc.c:1644:
> +^I^Ii++;^M$
>
> ERROR: DOS line endings
> #92: FILE: drivers/video/omap2/dss/dispc.c:1645:
> +^Iblank = div_u64((u64)(t.hbp + t.hsw + t.hfp) * lclk, pclk);^M$
>
> ERROR: DOS line endings
> #93: FILE: drivers/video/omap2/dss/dispc.c:1646:
> +^IDSSDBG("blanking period + ppl = %llu (limit = %u)\n", blank,
> limits[i]);^M$
>
> WARNING: line over 80 characters
> #93: FILE: drivers/video/omap2/dss/dispc.c:1646:
> +       DSSDBG("blanking period + ppl = %llu (limit = %u)\n", blank,
> limits[i]);
>
> ERROR: DOS line endings
> #94: FILE: drivers/video/omap2/dss/dispc.c:1647:
> +^Iif (blank <= limits[i])^M$
>
> ERROR: DOS line endings
> #95: FILE: drivers/video/omap2/dss/dispc.c:1648:
> +^I^Ireturn -EINVAL;^M$
>
> ERROR: DOS line endings
> #96: FILE: drivers/video/omap2/dss/dispc.c:1649:
> +^M$
>
> ERROR: trailing whitespace
> #97: FILE: drivers/video/omap2/dss/dispc.c:1650:
> +^I/*^M$
>
> ERROR: trailing whitespace
> #98: FILE: drivers/video/omap2/dss/dispc.c:1651:
> +^I * Pixel data should be prepared before visible display point
> starts.^M$
>
> ERROR: trailing whitespace
> #99: FILE: drivers/video/omap2/dss/dispc.c:1652:
> +^I * So, atleast DS-2 lines must have already been fetched by DISPC^M$
>
> ERROR: trailing whitespace
> #100: FILE: drivers/video/omap2/dss/dispc.c:1653:
> +^I * during nonactive - pos_x period.^M$
>
> ERROR: DOS line endings
> #101: FILE: drivers/video/omap2/dss/dispc.c:1654:
> +^I */^M$
>
> ERROR: DOS line endings
> #102: FILE: drivers/video/omap2/dss/dispc.c:1655:
> +^Ival = div_u64((u64)(nonactive - pos_x) * lclk, pclk);^M$
>
> ERROR: DOS line endings
> #103: FILE: drivers/video/omap2/dss/dispc.c:1656:
> +^IDSSDBG("(nonactive - pos_x) * pcd = %llu,"^M$
>
> ERROR: DOS line endings
> #104: FILE: drivers/video/omap2/dss/dispc.c:1657:
> +^I^I" max(0, DS - 2) * width = %d\n",^M$
>
> ERROR: DOS line endings
> #105: FILE: drivers/video/omap2/dss/dispc.c:1658:
> +^I^Ival, max(0, DS - 2) * width);^M$
>
> ERROR: DOS line endings
> #106: FILE: drivers/video/omap2/dss/dispc.c:1659:
> +^Iif (val < max(0, DS - 2) * width)^M$
>
> ERROR: DOS line endings
> #107: FILE: drivers/video/omap2/dss/dispc.c:1660:
> +^I^Ireturn -EINVAL;^M$
>
> ERROR: DOS line endings
> #108: FILE: drivers/video/omap2/dss/dispc.c:1661:
> +^M$
>
> ERROR: trailing whitespace
> #109: FILE: drivers/video/omap2/dss/dispc.c:1662:
> +^I/*^M$
>
> ERROR: trailing whitespace
> #110: FILE: drivers/video/omap2/dss/dispc.c:1663:
> +^I * All lines need to be refilled during the nonactive period of
> which^M$
>
> ERROR: trailing whitespace
> #111: FILE: drivers/video/omap2/dss/dispc.c:1664:
> +^I * only one line can be loaded during the active period. So,
> atleast^M$
>
> ERROR: trailing whitespace
> #112: FILE: drivers/video/omap2/dss/dispc.c:1665:
> +^I * DS - 1 lines should be loaded during nonactive period.^M$
>
> ERROR: DOS line endings
> #113: FILE: drivers/video/omap2/dss/dispc.c:1666:
> +^I */^M$
>
> ERROR: DOS line endings
> #114: FILE: drivers/video/omap2/dss/dispc.c:1667:
> +^Ival =  div_u64((u64)nonactive * lclk, pclk);^M$
>
> ERROR: DOS line endings
> #115: FILE: drivers/video/omap2/dss/dispc.c:1668:
> +^IDSSDBG("nonactive * pcd  = %llu, max(0, DS - 1) * width = %d\n",^M$
>
> ERROR: DOS line endings
> #116: FILE: drivers/video/omap2/dss/dispc.c:1669:
> +^I^Ival, max(0, DS - 1) * width);^M$
>
> ERROR: DOS line endings
> #117: FILE: drivers/video/omap2/dss/dispc.c:1670:
> +^Iif (val < max(0, DS - 1) * width)^M$
>
> ERROR: DOS line endings
> #118: FILE: drivers/video/omap2/dss/dispc.c:1671:
> +^I^Ireturn -EINVAL;^M$
>
> ERROR: DOS line endings
> #119: FILE: drivers/video/omap2/dss/dispc.c:1672:
> +^M$
>
> ERROR: DOS line endings
> #120: FILE: drivers/video/omap2/dss/dispc.c:1673:
> +^Ireturn 0;^M$
>
> ERROR: DOS line endings
> #121: FILE: drivers/video/omap2/dss/dispc.c:1674:
> +}^M$
>
> ERROR: DOS line endings
> #122: FILE: drivers/video/omap2/dss/dispc.c:1675:
> +^M$
>
> ERROR: DOS line endings
> #131: FILE: drivers/video/omap2/dss/dispc.c:1756:
> +^I^Iint *x_predecim, int *y_predecim, u16 pos_x)^M$
>
> ERROR: DOS line endings
> #139: FILE: drivers/video/omap2/dss/dispc.c:1832:
> +^I^I^Ierror = check_horiz_timing(channel, pos_x, in_width,^M$
>
> ERROR: DOS line endings
> #140: FILE: drivers/video/omap2/dss/dispc.c:1833:
> +^I^I^I^Iin_height, out_width, out_height);^M$
>
> ERROR: DOS line endings
> #141: FILE: drivers/video/omap2/dss/dispc.c:1834:
> +^M$
>
> ERROR: DOS line endings
> #150: FILE: drivers/video/omap2/dss/dispc.c:1842:
> +^I^I^Ierror = (error || in_width > maxsinglelinewidth * 2 ||^M$
>
> ERROR: DOS line endings
> #158: FILE: drivers/video/omap2/dss/dispc.c:1858:
> +^I^Iif (check_horiz_timing(channel, pos_x, width, height,^M$
>
> ERROR: DOS line endings
> #159: FILE: drivers/video/omap2/dss/dispc.c:1859:
> +^I^I^Iout_width, out_height)){^M$
>
> ERROR: DOS line endings
> #160: FILE: drivers/video/omap2/dss/dispc.c:1860:
> +^I^I^I^IDSSERR("horizontal timing too tight\n");^M$
>
> ERROR: DOS line endings
> #161: FILE: drivers/video/omap2/dss/dispc.c:1861:
> +^I^I^I^Ireturn -EINVAL;^M$
>
> ERROR: DOS line endings
> #162: FILE: drivers/video/omap2/dss/dispc.c:1862:
> +^I^I}^M$
>
> ERROR: DOS line endings
> #163: FILE: drivers/video/omap2/dss/dispc.c:1863:
> +^M$
>
> ERROR: DOS line endings
> #172: FILE: drivers/video/omap2/dss/dispc.c:1964:
> +^I^I^I&x_predecim, &y_predecim, oi->pos_x);^M$
>
> ERROR: DOS line endings
> #184: FILE: drivers/video/omap2/dss/dispc.c:2535:
> +^Iunsigned long r = dispc_fclk_rate();^M$
>
> ERROR: DOS line endings
> #187: FILE: drivers/video/omap2/dss/dispc.c:2537:
> +^Iif (dispc_mgr_is_lcd(channel)) {^M$
>
> ERROR: DOS line endings
> #188: FILE: drivers/video/omap2/dss/dispc.c:2538:
> +^I^Iu32 l;^M$
>
> ERROR: DOS line endings
> #189: FILE: drivers/video/omap2/dss/dispc.c:2539:
> +^I^Iint lcd;^M$
>
> ERROR: DOS line endings
> #192: FILE: drivers/video/omap2/dss/dispc.c:2541:
> +^I^Il = dispc_read_reg(DISPC_DIVISORo(channel));^M$
>
> ERROR: DOS line endings
> #193: FILE: drivers/video/omap2/dss/dispc.c:2542:
> +^I^Ilcd = FLD_GET(l, 23, 16);^M$
>
> ERROR: DOS line endings
> #209: FILE: drivers/video/omap2/dss/dispc.c:2544:
> +^I^Ireturn r / lcd;^M$
>
> ERROR: DOS line endings
> #210: FILE: drivers/video/omap2/dss/dispc.c:2545:
> +^I} else {^M$
>
> ERROR: DOS line endings
> #211: FILE: drivers/video/omap2/dss/dispc.c:2546:
> +^I^Ireturn r;^M$
>
> total: 72 errors, 1 warnings, 143 lines checked
>
> NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch
> or
>      scripts/cleanfile
>
> 123.mbox has style problems, please review.
>
> If any of these errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>



-- 
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.

Re: [PATCH V2 2/3] OMAPDSS: DISPC: Handle synclost errors in OMAP3

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2859 bytes --]

On Tue, 2012-03-27 at 16:37 +0530, Mahapatra, Chandrabhanu wrote:
> On Tue, Mar 27, 2012 at 4:27 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Wed, 2012-03-21 at 15:21 +0530, Chandrabhanu Mahapatra wrote:
> >> In OMAP3 DISPC video overlays suffer from some undocumented horizontal position
> >> and timing related limitations leading to SYNCLOST errors. Whenever the image
> >> window is moved towards the right of the screen SYNCLOST errors become
> >> frequent. Checks have been implemented to see that DISPC driver rejects
> >> configuration exceeding above limitations.
> >>
> >> This code was successfully tested on OMAP3. This code is written based on code
> >> written by Ville Syrjälä <ville.syrjala@nokia.com> in Linux OMAP kernel. Ville
> >> Syrjälä <ville.syrjala@nokia.com> had added checks for video overlay horizontal
> >> timing and DISPC horizontal blanking length limitations.
> >>
> >> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
> >> ---
> >>  drivers/video/omap2/dss/dispc.c |   97 +++++++++++++++++++++++++++++----------
> >>  1 files changed, 72 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
> >> index 5a1963e..d8a1672 100644
> >> --- a/drivers/video/omap2/dss/dispc.c
> >> +++ b/drivers/video/omap2/dss/dispc.c
> >> @@ -1622,6 +1622,57 @@ static void calc_dma_rotation_offset(u8 rotation, bool mirror,
> >>       }
> >>  }
> >>
> >> +static int check_horiz_timing(enum omap_channel channel, u16 pos_x,
> >> +             u16 width, u16 height, u16 out_width, u16 out_height)
> >
> > I think the function could be named check_horiz_timing_omap3 or
> > something. It's kinda hard to realize that this is an omap3 work-around.
> > Perhaps a short comment above the function would be in order.
> >
> 
> Yes, you are right! Do you mean to include some comments within the
> code definition itself? The patch description includes all that this
> function does.

I mean just a short comment above the function that mentions that this
function is used to avoid the synclosts on omap3, because yadda yadda,
so that the reader will immediately realize that this is doing some
special handling. 

> > I don't the change to dispc_mgr_lclk_rate has anything to do with this
> > patch (fixing omap3 sync lost error). Shouldn't it be a separate fix?
> >
> >  Tomi
> >
> I am thinking to to move this fix to the third patch in the series
> "OMAPDSS: DISPC: Correct DISPC functional clock usage" and make it
> separate patch from the series and if possible move it ahead of the
> predecimation patch series. What do you say?

This patch is using dispc_mgr_lclk_rate. Does it depend on the change?
If so, it needs to be fixed before this patch. But yes, separating it
sounds fine. 

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2 2/3] OMAPDSS: DISPC: Handle synclost errors in OMAP3

[Mahapatra, Chandrabhanu]

On Tue, Mar 27, 2012 at 4:27 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Wed, 2012-03-21 at 15:21 +0530, Chandrabhanu Mahapatra wrote:
>> In OMAP3 DISPC video overlays suffer from some undocumented horizontal position
>> and timing related limitations leading to SYNCLOST errors. Whenever the image
>> window is moved towards the right of the screen SYNCLOST errors become
>> frequent. Checks have been implemented to see that DISPC driver rejects
>> configuration exceeding above limitations.
>>
>> This code was successfully tested on OMAP3. This code is written based on code
>> written by Ville Syrjälä <ville.syrjala@nokia.com> in Linux OMAP kernel. Ville
>> Syrjälä <ville.syrjala@nokia.com> had added checks for video overlay horizontal
>> timing and DISPC horizontal blanking length limitations.
>>
>> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
>> ---
>>  drivers/video/omap2/dss/dispc.c |   97 +++++++++++++++++++++++++++++----------
>>  1 files changed, 72 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
>> index 5a1963e..d8a1672 100644
>> --- a/drivers/video/omap2/dss/dispc.c
>> +++ b/drivers/video/omap2/dss/dispc.c
>> @@ -1622,6 +1622,57 @@ static void calc_dma_rotation_offset(u8 rotation, bool mirror,
>>       }
>>  }
>>
>> +static int check_horiz_timing(enum omap_channel channel, u16 pos_x,
>> +             u16 width, u16 height, u16 out_width, u16 out_height)
>
> I think the function could be named check_horiz_timing_omap3 or
> something. It's kinda hard to realize that this is an omap3 work-around.
> Perhaps a short comment above the function would be in order.
>

Yes, you are right! Do you mean to include some comments within the
code definition itself? The patch description includes all that this
function does.

>> +{
>> +     int DS = DIV_ROUND_UP(height, out_height);
>> +     struct omap_dss_device *dssdev = dispc_mgr_get_device(channel);
>> +     struct omap_video_timings t = dssdev->panel.timings;
>> +     unsigned long nonactive, lclk, pclk;
>> +     static const u8 limits[3] = { 8, 10, 20 };
>> +     u64 val, blank;
>> +     int i;
>> +
>> +     nonactive = t.x_res + t.hfp + t.hsw + t.hbp - out_width;
>> +     pclk = dispc_mgr_pclk_rate(channel);
>> +     lclk = dispc_mgr_lclk_rate(channel);
>> +
>> +     i = 0;
>> +     if (out_height < height)
>> +             i++;
>> +     if (out_width < width)
>> +             i++;
>> +     blank = div_u64((u64)(t.hbp + t.hsw + t.hfp) * lclk, pclk);
>> +     DSSDBG("blanking period + ppl = %llu (limit = %u)\n", blank, limits[i]);
>> +     if (blank <= limits[i])
>> +             return -EINVAL;
>> +
>> +     /*
>> +      * Pixel data should be prepared before visible display point starts.
>> +      * So, atleast DS-2 lines must have already been fetched by DISPC
>> +      * during nonactive - pos_x period.
>> +      */
>> +     val = div_u64((u64)(nonactive - pos_x) * lclk, pclk);
>> +     DSSDBG("(nonactive - pos_x) * pcd = %llu,"
>> +             " max(0, DS - 2) * width = %d\n",
>> +             val, max(0, DS - 2) * width);
>> +     if (val < max(0, DS - 2) * width)
>> +             return -EINVAL;
>> +
>> +     /*
>> +      * All lines need to be refilled during the nonactive period of which
>> +      * only one line can be loaded during the active period. So, atleast
>> +      * DS - 1 lines should be loaded during nonactive period.
>> +      */
>> +     val =  div_u64((u64)nonactive * lclk, pclk);
>> +     DSSDBG("nonactive * pcd  = %llu, max(0, DS - 1) * width = %d\n",
>> +             val, max(0, DS - 1) * width);
>> +     if (val < max(0, DS - 1) * width)
>> +             return -EINVAL;
>> +
>> +     return 0;
>> +}
>> +
>>  static unsigned long calc_fclk_five_taps(enum omap_channel channel, u16 width,
>>               u16 height, u16 out_width, u16 out_height,
>>               enum omap_color_mode color_mode)
>> @@ -1702,7 +1753,7 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
>>               enum omap_channel channel, u16 width, u16 height,
>>               u16 out_width, u16 out_height,
>>               enum omap_color_mode color_mode, bool *five_taps,
>> -             int *x_predecim, int *y_predecim)
>> +             int *x_predecim, int *y_predecim, u16 pos_x)
>>  {
>>       struct omap_overlay *ovl = omap_dss_get_overlay(plane);
>>       const int maxdownscale = dss_feat_get_param_max(FEAT_PARAM_DOWNSCALE);
>> @@ -1778,6 +1829,9 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
>>                       fclk = calc_fclk_five_taps(channel, in_width, in_height,
>>                               out_width, out_height, color_mode);
>>
>> +                     error = check_horiz_timing(channel, pos_x, in_width,
>> +                             in_height, out_width, out_height);
>> +
>>                       if (in_width > maxsinglelinewidth)
>>                               if (in_height > out_height &&
>>                                       in_height < out_height * 2)
>> @@ -1785,7 +1839,7 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
>>                       if (!*five_taps)
>>                               fclk = calc_fclk(channel, in_width, in_height,
>>                                       out_width, out_height);
>> -                     error = (in_width > maxsinglelinewidth * 2 ||
>> +                     error = (error || in_width > maxsinglelinewidth * 2 ||
>>                               (in_width > maxsinglelinewidth && *five_taps) ||
>>                               !fclk || fclk > dispc_fclk_rate());
>>                       if (error) {
>> @@ -1801,6 +1855,12 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
>>               } while (decim_x <= *x_predecim && decim_y <= *y_predecim
>>                       && error);
>>
>> +             if (check_horiz_timing(channel, pos_x, width, height,
>> +                     out_width, out_height)){
>> +                             DSSERR("horizontal timing too tight\n");
>> +                             return -EINVAL;
>> +             }
>> +
>>               if (in_width > (maxsinglelinewidth * 2)) {
>>                       DSSERR("Cannot setup scaling");
>>                       DSSERR("width exceeds maximum width possible");
>> @@ -1901,7 +1961,7 @@ int dispc_ovl_setup(enum omap_plane plane, struct omap_overlay_info *oi,
>>
>>       r = dispc_ovl_calc_scaling(plane, channel, in_width, in_height,
>>                       out_width, out_height, oi->color_mode, &five_taps,
>> -                     &x_predecim, &y_predecim);
>> +                     &x_predecim, &y_predecim, oi->pos_x);
>>       if (r)
>>               return r;
>>
>> @@ -2472,32 +2532,19 @@ unsigned long dispc_fclk_rate(void)
>>
>>  unsigned long dispc_mgr_lclk_rate(enum omap_channel channel)
>>  {
>> -     struct platform_device *dsidev;
>> -     int lcd;
>> -     unsigned long r;
>> -     u32 l;
>> +     unsigned long r = dispc_fclk_rate();
>>
>> -     l = dispc_read_reg(DISPC_DIVISORo(channel));
>> +     if (dispc_mgr_is_lcd(channel)) {
>> +             u32 l;
>> +             int lcd;
>>
>> -     lcd = FLD_GET(l, 23, 16);
>> +             l = dispc_read_reg(DISPC_DIVISORo(channel));
>> +             lcd = FLD_GET(l, 23, 16);
>>
>> -     switch (dss_get_lcd_clk_source(channel)) {
>> -     case OMAP_DSS_CLK_SRC_FCK:
>> -             r = clk_get_rate(dispc.dss_clk);
>> -             break;
>> -     case OMAP_DSS_CLK_SRC_DSI_PLL_HSDIV_DISPC:
>> -             dsidev = dsi_get_dsidev_from_id(0);
>> -             r = dsi_get_pll_hsdiv_dispc_rate(dsidev);
>> -             break;
>> -     case OMAP_DSS_CLK_SRC_DSI2_PLL_HSDIV_DISPC:
>> -             dsidev = dsi_get_dsidev_from_id(1);
>> -             r = dsi_get_pll_hsdiv_dispc_rate(dsidev);
>> -             break;
>> -     default:
>> -             BUG();
>> +             return r / lcd;
>> +     } else {
>> +             return r;
>>       }
>> -
>> -     return r / lcd;
>>  }
>
> I don't the change to dispc_mgr_lclk_rate has anything to do with this
> patch (fixing omap3 sync lost error). Shouldn't it be a separate fix?
>
>  Tomi
>
I am thinking to to move this fix to the third patch in the series
"OMAPDSS: DISPC: Correct DISPC functional clock usage" and make it
separate patch from the series and if possible move it ahead of the
predecimation patch series. What do you say?


-- 
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.

Re: [PATCH V2 2/3] OMAPDSS: DISPC: Handle synclost errors in OMAP3

[Mahapatra, Chandrabhanu]

On Tue, Mar 27, 2012 at 4:40 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Tue, 2012-03-27 at 16:31 +0530, Mahapatra, Chandrabhanu wrote:
>> On Tue, Mar 27, 2012 at 4:13 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> > Hi,
>> >
>> > On Wed, 2012-03-21 at 15:21 +0530, Chandrabhanu Mahapatra wrote:
>> >> In OMAP3 DISPC video overlays suffer from some undocumented horizontal position
>> >> and timing related limitations leading to SYNCLOST errors. Whenever the image
>> >> window is moved towards the right of the screen SYNCLOST errors become
>> >> frequent. Checks have been implemented to see that DISPC driver rejects
>> >> configuration exceeding above limitations.
>> >>
>> >> This code was successfully tested on OMAP3. This code is written based on code
>> >> written by Ville Syrjälä <ville.syrjala@nokia.com> in Linux OMAP kernel. Ville
>> >> Syrjälä <ville.syrjala@nokia.com> had added checks for video overlay horizontal
>> >> timing and DISPC horizontal blanking length limitations.
>> >>
>> >> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
>> >> ---
>> >>  drivers/video/omap2/dss/dispc.c |   97 +++++++++++++++++++++++++++++----------
>> >>  1 files changed, 72 insertions(+), 25 deletions(-)
>> >
>> > The patch seems to be using dos line endings... Where did they come
>> > from? The two others are fine. I think git am removed the dos line
>> > endings, so it's fine.
>> >
>>
>> How did you find these errors? scripts/checkpatch.pl still returns me nothing.
>
> Well I just saved the email and ran checkpatch. Could be a problem in
> gmail or my end too, but I've never seen it happen before.
>
>  Tomi
>
Save it as text file and then run checkpatch it wont give any errors.
By default it tries to save it as a eml file. Just check it.


-- 
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.

Re: [PATCH V2 2/3] OMAPDSS: DISPC: Handle synclost errors in OMAP3

[Mahapatra, Chandrabhanu]

On Tue, Mar 27, 2012 at 4:47 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Tue, 2012-03-27 at 16:37 +0530, Mahapatra, Chandrabhanu wrote:
>> On Tue, Mar 27, 2012 at 4:27 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> > On Wed, 2012-03-21 at 15:21 +0530, Chandrabhanu Mahapatra wrote:
>> >> In OMAP3 DISPC video overlays suffer from some undocumented horizontal position
>> >> and timing related limitations leading to SYNCLOST errors. Whenever the image
>> >> window is moved towards the right of the screen SYNCLOST errors become
>> >> frequent. Checks have been implemented to see that DISPC driver rejects
>> >> configuration exceeding above limitations.
>> >>
>> >> This code was successfully tested on OMAP3. This code is written based on code
>> >> written by Ville Syrjälä <ville.syrjala@nokia.com> in Linux OMAP kernel. Ville
>> >> Syrjälä <ville.syrjala@nokia.com> had added checks for video overlay horizontal
>> >> timing and DISPC horizontal blanking length limitations.
>> >>
>> >> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
>> >> ---
>> >>  drivers/video/omap2/dss/dispc.c |   97 +++++++++++++++++++++++++++++----------
>> >>  1 files changed, 72 insertions(+), 25 deletions(-)
>> >>
>> >> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
>> >> index 5a1963e..d8a1672 100644
>> >> --- a/drivers/video/omap2/dss/dispc.c
>> >> +++ b/drivers/video/omap2/dss/dispc.c
>> >> @@ -1622,6 +1622,57 @@ static void calc_dma_rotation_offset(u8 rotation, bool mirror,
>> >>       }
>> >>  }
>> >>
>> >> +static int check_horiz_timing(enum omap_channel channel, u16 pos_x,
>> >> +             u16 width, u16 height, u16 out_width, u16 out_height)
>> >
>> > I think the function could be named check_horiz_timing_omap3 or
>> > something. It's kinda hard to realize that this is an omap3 work-around.
>> > Perhaps a short comment above the function would be in order.
>> >
>>
>> Yes, you are right! Do you mean to include some comments within the
>> code definition itself? The patch description includes all that this
>> function does.
>
> I mean just a short comment above the function that mentions that this
> function is used to avoid the synclosts on omap3, because yadda yadda,
> so that the reader will immediately realize that this is doing some
> special handling.
>

Ok.

>> > I don't the change to dispc_mgr_lclk_rate has anything to do with this
>> > patch (fixing omap3 sync lost error). Shouldn't it be a separate fix?
>> >
>> >  Tomi
>> >
>> I am thinking to to move this fix to the third patch in the series
>> "OMAPDSS: DISPC: Correct DISPC functional clock usage" and make it
>> separate patch from the series and if possible move it ahead of the
>> predecimation patch series. What do you say?
>
> This patch is using dispc_mgr_lclk_rate. Does it depend on the change?
> If so, it needs to be fixed before this patch. But yes, separating it
> sounds fine.
>
>  Tomi
>
To a certain extent it does. Moving it to the third patch will
introduce certain code changes as
if (dispc_mgr_is_lcd(channel)) while initialising dispc_core_clk.
Anyways that can be cleaned up in the third patch.

-- 
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.