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

[PATCH 2/2] 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 |   67 +++++++++++++++++++++++++++++++++++++--
 1 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 5a1963e..ebfa613 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -1622,6 +1622,58 @@ 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;
+	int pcd = REG_GET(DISPC_DIVISORo(channel), 7, 0);
+	unsigned long nonactive, val, blank;
+	static const u8 limits[3] = { 8, 10, 20 };
+	int i;
+
+	nonactive = t.x_res + t.hfp + t.hsw + t.hbp - out_width;
+
+	/*
+	 * Atleast DS-2 lines must have already been fetched
+	 * before the display active video period starts.
+	 */
+	val = (nonactive - pos_x) * pcd;
+	DSSDBG("(nonactive - pos_x) * pcd = %lu,"
+		" max(0, DS - 2) * width = %d\n",
+		val, max(0, DS - 2) * width);
+	if (val < max(0, DS - 2) * width)
+		return -EINVAL;
+
+	/*
+	 * Only one line can be fetched during the overlay active
+	 * period, the rest have to be fetched during the inactive
+	 * period.
+	 */
+	val = nonactive * pcd;
+	DSSDBG("nonactive * pcd  = %lu, max(0, DS - 1) * width = %d\n",
+		val, max(0, DS - 1) * width);
+	if (val < max(0, DS - 1) * width)
+		return -EINVAL;
+
+	/*
+	 * Atleast Ceil(DS) lines should have been loaded during
+	 * PPL (screen width) + blanking period.
+	 */
+	i = 0;
+	if (out_height < height)
+		i++;
+	if (out_width < width)
+		i++;
+	blank = (t.hbp + t.hsw + t.hfp) * pcd;
+	DSSDBG("blanking period + ppl = %lu (limit = %u)\n", blank, limits[i]);
+	if (blank <= limits[i])
+		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 +1754,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 +1830,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 +1840,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 +1856,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 +1962,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;
 
-- 
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 2/2] OMAPDSS: DISPC: Handle synclost errors in OMAP3

[Ville Syrjälä]

On Thu, Mar 15, 2012 at 05:18:52PM +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 |   67 +++++++++++++++++++++++++++++++++++++--
>  1 files changed, 64 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
> index 5a1963e..ebfa613 100644
> --- a/drivers/video/omap2/dss/dispc.c
> +++ b/drivers/video/omap2/dss/dispc.c
> @@ -1622,6 +1622,58 @@ 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;
> +	int pcd = REG_GET(DISPC_DIVISORo(channel), 7, 0);

pcd doesn't exist for TV out, which is the reason why you see
'lclk/pclk' ratio used instead in the harmattan kernel.

Another thing which still seems to be wrong in the upstream code is the
use of fclk in the scaling checks. It should be checking lclk instead.

> +	unsigned long nonactive, val, blank;
> +	static const u8 limits[3] = { 8, 10, 20 };
> +	int i;
> +
> +	nonactive = t.x_res + t.hfp + t.hsw + t.hbp - out_width;
> +
> +	/*
> +	 * Atleast DS-2 lines must have already been fetched
> +	 * before the display active video period starts.
> +	 */
> +	val = (nonactive - pos_x) * pcd;
> +	DSSDBG("(nonactive - pos_x) * pcd = %lu,"
> +		" max(0, DS - 2) * width = %d\n",
> +		val, max(0, DS - 2) * width);
> +	if (val < max(0, DS - 2) * width)
> +		return -EINVAL;
> +
> +	/*
> +	 * Only one line can be fetched during the overlay active
> +	 * period, the rest have to be fetched during the inactive
> +	 * period.
> +	 */
> +	val = nonactive * pcd;
> +	DSSDBG("nonactive * pcd  = %lu, max(0, DS - 1) * width = %d\n",
> +		val, max(0, DS - 1) * width);
> +	if (val < max(0, DS - 1) * width)
> +		return -EINVAL;
> +
> +	/*
> +	 * Atleast Ceil(DS) lines should have been loaded during
> +	 * PPL (screen width) + blanking period.
> +	 */
> +	i = 0;
> +	if (out_height < height)
> +		i++;
> +	if (out_width < width)
> +		i++;
> +	blank = (t.hbp + t.hsw + t.hfp) * pcd;
> +	DSSDBG("blanking period + ppl = %lu (limit = %u)\n", blank, limits[i]);
> +	if (blank <= limits[i])
> +		return -EINVAL;

The comment and code here are totally out of sync. IIRC the hblank
length check came directly from the TRM. I have no idea if it's correct
for more recent OMAPs, or if it was just copy pasted from the
old TRM when the TRM got adjusted for more recent chips. That is why I
stuck it into a separate function in harmattan.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

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

[Tomi Valkeinen]

On Fri, 2012-03-16 at 01:04 +0200, Ville Syrjälä wrote:
> On Thu, Mar 15, 2012 at 05:18:52PM +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 |   67 +++++++++++++++++++++++++++++++++++++--
> >  1 files changed, 64 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
> > index 5a1963e..ebfa613 100644
> > --- a/drivers/video/omap2/dss/dispc.c
> > +++ b/drivers/video/omap2/dss/dispc.c
> > @@ -1622,6 +1622,58 @@ 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;
> > +	int pcd = REG_GET(DISPC_DIVISORo(channel), 7, 0);
> 
> pcd doesn't exist for TV out, which is the reason why you see
> 'lclk/pclk' ratio used instead in the harmattan kernel.

I'm surprised you still remember these things =). And even more
surprised that you actually bothered to review it, thanks for that.

Chandrabhanu, do you have an omap3 board with composite/svideo tv-out?
It'd be good to test on that also, as, like Ville points out, the above
code can't work for tv-out. Hmm, then again, it shouldn't work for HDMI
on OMAP4 either. Did you test with HDMI?

Or is the above code called for TV-manager at all? If not, perhaps it
could be named *_lcd or something. Or perhaps have a BUG_ON(channel !lcd1 or lcd2);

 Tomi

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

[Mahapatra, Chandrabhanu]

On Fri, Mar 16, 2012 at 4:34 AM, Ville Syrjälä <syrjala@sci.fi> wrote:
> On Thu, Mar 15, 2012 at 05:18:52PM +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 |   67 +++++++++++++++++++++++++++++++++++++--
>>  1 files changed, 64 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
>> index 5a1963e..ebfa613 100644
>> --- a/drivers/video/omap2/dss/dispc.c
>> +++ b/drivers/video/omap2/dss/dispc.c
>> @@ -1622,6 +1622,58 @@ 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;
>> +     int pcd = REG_GET(DISPC_DIVISORo(channel), 7, 0);
>
> pcd doesn't exist for TV out, which is the reason why you see
> 'lclk/pclk' ratio used instead in the harmattan kernel.
>
Yes, you are right. It's a very important point which I had missed.

> Another thing which still seems to be wrong in the upstream code is the
> use of fclk in the scaling checks. It should be checking lclk instead.

Thanks for sighting this out. I will implement it in the future.

>> +     unsigned long nonactive, val, blank;
>> +     static const u8 limits[3] = { 8, 10, 20 };
>> +     int i;
>> +
>> +     nonactive = t.x_res + t.hfp + t.hsw + t.hbp - out_width;
>> +
>> +     /*
>> +      * Atleast DS-2 lines must have already been fetched
>> +      * before the display active video period starts.
>> +      */
>> +     val = (nonactive - pos_x) * pcd;
>> +     DSSDBG("(nonactive - pos_x) * pcd = %lu,"
>> +             " max(0, DS - 2) * width = %d\n",
>> +             val, max(0, DS - 2) * width);
>> +     if (val < max(0, DS - 2) * width)
>> +             return -EINVAL;
>> +
>> +     /*
>> +      * Only one line can be fetched during the overlay active
>> +      * period, the rest have to be fetched during the inactive
>> +      * period.
>> +      */
>> +     val = nonactive * pcd;
>> +     DSSDBG("nonactive * pcd  = %lu, max(0, DS - 1) * width = %d\n",
>> +             val, max(0, DS - 1) * width);
>> +     if (val < max(0, DS - 1) * width)
>> +             return -EINVAL;
>> +
>> +     /*
>> +      * Atleast Ceil(DS) lines should have been loaded during
>> +      * PPL (screen width) + blanking period.
>> +      */
>> +     i = 0;
>> +     if (out_height < height)
>> +             i++;
>> +     if (out_width < width)
>> +             i++;
>> +     blank = (t.hbp + t.hsw + t.hfp) * pcd;
>> +     DSSDBG("blanking period + ppl = %lu (limit = %u)\n", blank, limits[i]);
>> +     if (blank <= limits[i])
>> +             return -EINVAL;
>
> The comment and code here are totally out of sync. IIRC the hblank
> length check came directly from the TRM. I have no idea if it's correct
> for more recent OMAPs, or if it was just copy pasted from the
> old TRM when the TRM got adjusted for more recent chips. That is why I
> stuck it into a separate function in harmattan.
>

This patch is only for OMAP3 and this errata is present upto OMAP4 ES1
which we are currently not supporting.

> --
> Ville Syrjälä
> syrjala@sci.fi
> http://www.sci.fi/~syrjala/



-- 
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.

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

[Mahapatra, Chandrabhanu]

2012/3/16 Tomi Valkeinen <tomi.valkeinen@ti.com>:
> On Fri, 2012-03-16 at 01:04 +0200, Ville Syrjälä wrote:
>> On Thu, Mar 15, 2012 at 05:18:52PM +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 |   67 +++++++++++++++++++++++++++++++++++++--
>> >  1 files changed, 64 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
>> > index 5a1963e..ebfa613 100644
>> > --- a/drivers/video/omap2/dss/dispc.c
>> > +++ b/drivers/video/omap2/dss/dispc.c
>> > @@ -1622,6 +1622,58 @@ 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;
>> > +   int pcd = REG_GET(DISPC_DIVISORo(channel), 7, 0);
>>
>> pcd doesn't exist for TV out, which is the reason why you see
>> 'lclk/pclk' ratio used instead in the harmattan kernel.
>
> I'm surprised you still remember these things =). And even more
> surprised that you actually bothered to review it, thanks for that.
>
> Chandrabhanu, do you have an omap3 board with composite/svideo tv-out?
> It'd be good to test on that also, as, like Ville points out, the above
> code can't work for tv-out. Hmm, then again, it shouldn't work for HDMI
> on OMAP4 either. Did you test with HDMI?
>
> Or is the above code called for TV-manager at all? If not, perhaps it
> could be named *_lcd or something. Or perhaps have a BUG_ON(channel !> lcd1 or lcd2);
>

Yes, I do have a Beagle board and I have tested on HDMI which works
without synclost errors upto 640 * 480 resolution.
The above errata exists only upto OMAP4430 ES1 which is currently not
supported. Please refer to errata 2.14 Downscaling Limitations in
errata sheet.
This patch is only for Omap3.

>  Tomi
>
>



-- 
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.