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

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- 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 --]