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

["Mahapatra, Chandrabhanu" <cmahapatra@ti.com>]

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.