OMAPDSS: DISPC: horizontal timing too tight errors

OMAPDSS: DISPC: horizontal timing too tight errors

[Ivaylo Dimitrov]

Hi,

commit 7faa92339bbb1e6b9a80983b206642517327eb75 "OMAPDSS: DISPC: Handle 
synclost errors in OMAP3" introduces some limits check to prevent 
SYNCLOST errors on OMAP3 in a specific usecase. The problem I see here 
(on Nokia N900, Maemo 5, linux 3.13-rc6, DSP accel video decoding) is 
that those checks effectively prevent fullscreen video playback of 
anything above lets say 640x350 with "horizontal timing too tight" 
errors spit in dmesg log. If I hack check_horiz_timing_omap3 function to 
always return true, I can happily play videos up to (and including) 720p 
resolutions, with no SYNCLOST errors.

So, a couple of questions:

Where do the values in static const u8 limits[3] come from? Are those 
documented somewhere?

Commit message says "This code is written based on code written by Ville 
Syrjälä <ville.syrjala@nokia.com> in Linux OMAP kernel.", is that code 
publicly available and where (if it is).

Besides compiling DSS driver with DEBUG enabled and providing the log 
(yeah, I know I should've done it already and have the logs included in 
this mail, but... :) ), is there anything else I can do to find the 
culprit for those errors.

Regards,
Ivo

Re: OMAPDSS: DISPC: horizontal timing too tight errors

[Tomi Valkeinen]

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

On 2014-01-06 11:43, Ivaylo Dimitrov wrote:
> Hi,
> 
> commit 7faa92339bbb1e6b9a80983b206642517327eb75 "OMAPDSS: DISPC: Handle
> synclost errors in OMAP3" introduces some limits check to prevent
> SYNCLOST errors on OMAP3 in a specific usecase. The problem I see here
> (on Nokia N900, Maemo 5, linux 3.13-rc6, DSP accel video decoding) is
> that those checks effectively prevent fullscreen video playback of
> anything above lets say 640x350 with "horizontal timing too tight"
> errors spit in dmesg log. If I hack check_horiz_timing_omap3 function to
> always return true, I can happily play videos up to (and including) 720p
> resolutions, with no SYNCLOST errors.

I never worked with the patch in question, but my understanding is that
the core issue is quite difficult to solve optimally for all cases.
There are so many variables involved. So it may well be that the patch
in question does it a bit over-safely. Then again, it might as well have
a bug =).

One option is also to increase the blanking timings and pixel clock,
presuming the panel is fine with it. That would allow some more time for
the DISPC to manage scaling.

So are you saying that you can't even play any video in the LCD's native
resolution (i.e. no scaling needed) with fullscreen?

> So, a couple of questions:
> 
> Where do the values in static const u8 limits[3] come from? Are those
> documented somewhere?

If I remember right, these horizontal check timings information came
from some non-public TI guides. Not from the TRM.

> Commit message says "This code is written based on code written by Ville
> Syrjälä <ville.syrjala@nokia.com> in Linux OMAP kernel.", is that code
> publicly available and where (if it is).

It should be in the Nokia's kernel for N9.

> Besides compiling DSS driver with DEBUG enabled and providing the log
> (yeah, I know I should've done it already and have the logs included in
> this mail, but... :) ), is there anything else I can do to find the
> culprit for those errors.

You could look at the original patch in the Nokia kernel to see if the
mainline version is ok. Or maybe even better, try the same use case on
Nokia's kernel to see if it works.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: OMAPDSS: DISPC: horizontal timing too tight errors

[Pavel Machek]

On Tue 2014-01-07 14:35:02, Tomi Valkeinen wrote:
> On 2014-01-06 11:43, Ivaylo Dimitrov wrote:
> > Hi,
> > 
> > commit 7faa92339bbb1e6b9a80983b206642517327eb75 "OMAPDSS: DISPC: Handle
> > synclost errors in OMAP3" introduces some limits check to prevent
> > SYNCLOST errors on OMAP3 in a specific usecase. The problem I see here
> > (on Nokia N900, Maemo 5, linux 3.13-rc6, DSP accel video decoding) is
> > that those checks effectively prevent fullscreen video playback of
> > anything above lets say 640x350 with "horizontal timing too tight"
> > errors spit in dmesg log. If I hack check_horiz_timing_omap3 function to
> > always return true, I can happily play videos up to (and including) 720p
> > resolutions, with no SYNCLOST errors.
> 
> I never worked with the patch in question, but my understanding is that
> the core issue is quite difficult to solve optimally for all cases.
> There are so many variables involved. So it may well be that the patch
> in question does it a bit over-safely. Then again, it might as well have
> a bug =).

Can we simply revert 7faa92339bbb1e6b9a80983b206642517327eb75 ?

Working around undocumented problems on unspecified machine, but
breaking configuration people actually use seems like a bad idea.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: OMAPDSS: DISPC: horizontal timing too tight errors

[Ivaylo Dimitrov]

On 07.01.2014 20:05, Pavel Machek wrote:
> On Tue 2014-01-07 14:35:02, Tomi Valkeinen wrote:
>> On 2014-01-06 11:43, Ivaylo Dimitrov wrote:
>>> Hi,
>>>
>>> commit 7faa92339bbb1e6b9a80983b206642517327eb75 "OMAPDSS: DISPC: Handle
>>> synclost errors in OMAP3" introduces some limits check to prevent
>>> SYNCLOST errors on OMAP3 in a specific usecase. The problem I see here
>>> (on Nokia N900, Maemo 5, linux 3.13-rc6, DSP accel video decoding) is
>>> that those checks effectively prevent fullscreen video playback of
>>> anything above lets say 640x350 with "horizontal timing too tight"
>>> errors spit in dmesg log. If I hack check_horiz_timing_omap3 function to
>>> always return true, I can happily play videos up to (and including) 720p
>>> resolutions, with no SYNCLOST errors.
>>
>> I never worked with the patch in question, but my understanding is that
>> the core issue is quite difficult to solve optimally for all cases.
>> There are so many variables involved. So it may well be that the patch
>> in question does it a bit over-safely. Then again, it might as well have
>> a bug =).
>
> Can we simply revert 7faa92339bbb1e6b9a80983b206642517327eb75 ?
>
> Working around undocumented problems on unspecified machine, but
> breaking configuration people actually use seems like a bad idea.
>
> 									Pavel
>
The similar code exists in both omap1(N900 stock) and N9 kernels, I 
guess there is some other bug (somewhere else), trying to find it as we 
speak :)

Ivo

Re: OMAPDSS: DISPC: horizontal timing too tight errors

[Ivaylo Dimitrov]

On 07.01.2014 14:35, Tomi Valkeinen wrote:
> On 2014-01-06 11:43, Ivaylo Dimitrov wrote:
>> Hi,
>>
>> commit 7faa92339bbb1e6b9a80983b206642517327eb75 "OMAPDSS: DISPC: Handle
>> synclost errors in OMAP3" introduces some limits check to prevent
>> SYNCLOST errors on OMAP3 in a specific usecase. The problem I see here
>> (on Nokia N900, Maemo 5, linux 3.13-rc6, DSP accel video decoding) is
>> that those checks effectively prevent fullscreen video playback of
>> anything above lets say 640x350 with "horizontal timing too tight"
>> errors spit in dmesg log. If I hack check_horiz_timing_omap3 function to
>> always return true, I can happily play videos up to (and including) 720p
>> resolutions, with no SYNCLOST errors.
>
> I never worked with the patch in question, but my understanding is that
> the core issue is quite difficult to solve optimally for all cases.
> There are so many variables involved. So it may well be that the patch
> in question does it a bit over-safely. Then again, it might as well have
> a bug =).
>
> One option is also to increase the blanking timings and pixel clock,
> presuming the panel is fine with it. That would allow some more time for
> the DISPC to manage scaling.
>
> So are you saying that you can't even play any video in the LCD's native
> resolution (i.e. no scaling needed) with fullscreen?
>
>> So, a couple of questions:
>>
>> Where do the values in static const u8 limits[3] come from? Are those
>> documented somewhere?
>
> If I remember right, these horizontal check timings information came
> from some non-public TI guides. Not from the TRM.
>
>> Commit message says "This code is written based on code written by Ville
>> Syrjälä <ville.syrjala@nokia.com> in Linux OMAP kernel.", is that code
>> publicly available and where (if it is).
>
> It should be in the Nokia's kernel for N9.
>
>> Besides compiling DSS driver with DEBUG enabled and providing the log
>> (yeah, I know I should've done it already and have the logs included in
>> this mail, but... :) ), is there anything else I can do to find the
>> culprit for those errors.
>
> You could look at the original patch in the Nokia kernel to see if the
> mainline version is ok. Or maybe even better, try the same use case on
> Nokia's kernel to see if it works.
>
>   Tomi
>
>

Ok, after looking at what both N900 and N9 Nokia kernels do, I came up 
with the patch bellow. If you are ok with the changes, I'll submit the 
patch as it should. With that patch I tried more than 20 videos of 
different resolutions(including 720p), not a single failure :) . 
Basically it changes the core clock calculation to be done in the same 
way as in the Nokia kernels.

Regards,
Ivo



diff --git a/drivers/video/omap2/dss/dispc.c 
b/drivers/video/omap2/dss/dispc.c
index 67e413e..ab5aa05 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -1999,7 +1999,8 @@ static void calc_tiler_rotation_offset(u16 
screen_width, u16 width,
   */
  static int check_horiz_timing_omap3(unsigned long pclk, unsigned long 
lclk,
  		const struct omap_video_timings *t, u16 pos_x,
-		u16 width, u16 height, u16 out_width, u16 out_height)
+		u16 width, u16 height, u16 out_width, u16 out_height,
+				    bool five_taps)
  {
  	const int ds = DIV_ROUND_UP(height, out_height);
  	unsigned long nonactive;
@@ -2019,6 +2020,10 @@ static int check_horiz_timing_omap3(unsigned long 
pclk, unsigned long lclk,
  	if (blank <= limits[i])
  		return -EINVAL;

+	/* FIXME add checks for 3-tap filter once the limitations are known */
+	if (!five_taps)
+		return 0;
+	
  	/*
  	 * Pixel data should be prepared before visible display point starts.
  	 * So, atleast DS-2 lines must have already been fetched by DISPC
@@ -2191,24 +2196,33 @@ static int dispc_ovl_calc_scaling_34xx(unsigned 
long pclk, unsigned long lclk,
  	const int maxsinglelinewidth =
  			dss_feat_get_param_max(FEAT_PARAM_LINEWIDTH);

+	*five_taps = height > out_height;
+
  	do {
  		in_height = DIV_ROUND_UP(height, *decim_y);
  		in_width = DIV_ROUND_UP(width, *decim_x);
-		*core_clk = calc_core_clk_five_taps(pclk, mgr_timings,
-			in_width, in_height, out_width, out_height, color_mode);
-
-		error = check_horiz_timing_omap3(pclk, lclk, mgr_timings,
-				pos_x, in_width, in_height, out_width,
-				out_height);

  		if (in_width > maxsinglelinewidth)
  			if (in_height > out_height &&
  						in_height < out_height * 2)
  				*five_taps = false;
-		if (!*five_taps)
+again:
+		if(*five_taps)
+			*core_clk = calc_core_clk_five_taps(pclk, mgr_timings,
+						in_width, in_height, out_width,
+						out_height, color_mode);
+		else
  			*core_clk = dispc.feat->calc_core_clk(pclk, in_width,
-					in_height, out_width, out_height,
-					mem_to_mem);
+				in_height, out_width, out_height,
+				mem_to_mem);
+
+		error = check_horiz_timing_omap3(pclk, lclk, mgr_timings,
+				pos_x, in_width, in_height, out_width,
+				out_height, *five_taps);
+		if(*five_taps && error) {
+			*five_taps = false;
+			goto again;
+		}

  		error = (error || in_width > maxsinglelinewidth * 2 ||
  			(in_width > maxsinglelinewidth && *five_taps) ||
@@ -2226,7 +2240,7 @@ static int dispc_ovl_calc_scaling_34xx(unsigned 
long pclk, unsigned long lclk,
  	} while (*decim_x <= *x_predecim && *decim_y <= *y_predecim && error);

  	if (check_horiz_timing_omap3(pclk, lclk, mgr_timings, pos_x, width,
-				height, out_width, out_height)){
+				height, out_width, out_height, *five_taps)){
  			DSSERR("horizontal timing too tight\n");
  			return -EINVAL;
  	}

Re: OMAPDSS: DISPC: horizontal timing too tight errors

[Pavel Machek]

Hi!

> Ok, after looking at what both N900 and N9 Nokia kernels do, I came
> up with the patch bellow. If you are ok with the changes, I'll
> submit the patch as it should. With that patch I tried more than 20
> videos of different resolutions(including 720p), not a single
> failure :) . Basically it changes the core clock calculation to be
> done in the same way as in the Nokia kernels.

Thanks for the investigation.


> +again:
> +		if(*five_taps)

>From a quick view, you may want to add space after if.

> +		if(*five_taps && error) {
> +			*five_taps = false;
> +			goto again;
> +		}

Here too.

>  		error = (error || in_width > maxsinglelinewidth * 2 ||
>  			(in_width > maxsinglelinewidth && *five_taps) ||
> @@ -2226,7 +2240,7 @@ static int
> dispc_ovl_calc_scaling_34xx(unsigned long pclk, unsigned long lclk,
>  	} while (*decim_x <= *x_predecim && *decim_y <= *y_predecim && error);
> 
>  	if (check_horiz_timing_omap3(pclk, lclk, mgr_timings, pos_x, width,
> -				height, out_width, out_height)){
> +				height, out_width, out_height, *five_taps)){

...and before {.

(calc_scaling... that function has 16 arguments. Whoever created
it should be shot... or sold to Microsoft ;-).

								Pavel 
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: OMAPDSS: DISPC: horizontal timing too tight errors

[Tomi Valkeinen]

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

On 2014-01-07 22:25, Ivaylo Dimitrov wrote:

>>> Besides compiling DSS driver with DEBUG enabled and providing the log
>>> (yeah, I know I should've done it already and have the logs included in
>>> this mail, but... :) ), is there anything else I can do to find the
>>> culprit for those errors.
>>
>> You could look at the original patch in the Nokia kernel to see if the
>> mainline version is ok. Or maybe even better, try the same use case on
>> Nokia's kernel to see if it works.
>>
>>   Tomi
>>
>>
> 
> Ok, after looking at what both N900 and N9 Nokia kernels do, I came up
> with the patch bellow. If you are ok with the changes, I'll submit the
> patch as it should. With that patch I tried more than 20 videos of
> different resolutions(including 720p), not a single failure :) .
> Basically it changes the core clock calculation to be done in the same
> way as in the Nokia kernels.

Hmm ok. So the code was only partially ported to mainline?

You only seem to touch omap3 specific funcs, so I guess it's safe to
presume omap2/omap4+ work as well (or bad) as without this patch (i.e.
no need to test on various omaps =).

Thanks for figuring this out. Please send a proper patch, and if
possible, with an url to the Nokia kernel's code (presuming it's
available in a sane way).

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]