[RFC][RFT][PATCH] OMAPFB: LCDC: change update_mode to DISABLED when going suspend

[RFC][RFT][PATCH] OMAPFB: LCDC: change update_mode to DISABLED when going suspend

[Janusz Krzysztofik]

I was observing the following error messages on my OMAP1 based Amstrad Delta 
board when first changing from text to graphics mode or vice versa after the 
LCD display had been blanked:
	omapfb omapfb: timeout waiting for FRAME DONE
with a followup error message while unblanking it back:
	omapfb omapfb: resetting (status 0xffffffb2,reset count 1)
As a visible result, image pixels happened to be shifted by a few bits, 
giving wrong colors.

Examining the code, I found that this problem occures when an OMAP1 internal 
LCD controller is disabled from omap_lcdc_suspend() and then a subsequent 
omap_lcdc_setup_plane() calls disable_controller() again. This potentially 
error provoking behaviour is triggered by the lcdc.update_mode flag being kept 
at OMAP_AUTO_UPDATE, regardless of the controller and panel being suspended. 

This patch tries to correct the problem by replacing both omap_lcdc_suspend() 
and omap_lcdc_resume() function bodies with single calls to 
omap_lcdc_set_update_mode() with a respective OMAP_UPDATE_DISABLE or 
OMAP_AUTO_UPDATE argument. As a result, exactly the same lower level 
operations are performed, with addition of changing the lcdc.update_mode flag 
to a value better suited for the controller state. This prevents any further 
calls to disable_controller() from omap_lcdc_setup_plane() while the display 
is suspended.

Created against linux-2.6.34-rc7.
Tested on Amstrad Delta.

Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
---
Hi,

Those of you having access to other display equipped OMAP1 boards, could you 
please test if this patch doesn't break anything?

Thanks,
Janusz

 drivers/video/omap/lcdc.c |   14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

--- git/drivers/video/omap/lcdc.c.orig	2010-05-16 21:39:20.000000000 +0200
+++ git/drivers/video/omap/lcdc.c	2010-05-17 00:00:43.000000000 +0200
@@ -572,22 +572,12 @@ static enum omapfb_update_mode omap_lcdc
 /* PM code called only in internal controller mode */
 static void omap_lcdc_suspend(void)
 {
-	if (lcdc.update_mode = OMAPFB_AUTO_UPDATE) {
-		disable_controller();
-		omap_stop_lcd_dma();
-	}
+	omap_lcdc_set_update_mode(OMAPFB_UPDATE_DISABLED);
 }
 
 static void omap_lcdc_resume(void)
 {
-	if (lcdc.update_mode = OMAPFB_AUTO_UPDATE) {
-		setup_regs();
-		load_palette();
-		setup_lcd_dma();
-		set_load_mode(OMAP_LCDC_LOAD_FRAME);
-		enable_irqs(OMAP_LCDC_IRQ_DONE);
-		enable_controller();
-	}
+	omap_lcdc_set_update_mode(OMAPFB_AUTO_UPDATE);
 }
 
 static void omap_lcdc_get_caps(int plane, struct omapfb_caps *caps)

Re: [RFC][RFT][PATCH] OMAPFB: LCDC: change update_mode to DISABLED when going suspend

[Janusz Krzysztofik]

Monday 17 May 2010 03:20:13 Janusz Krzysztofik wrote:
> I was observing the following error messages on my OMAP1 based Amstrad
> Delta board when first changing from text to graphics mode or vice versa
> after the LCD display had been blanked:
> 	omapfb omapfb: timeout waiting for FRAME DONE
> with a followup error message while unblanking it back:
> 	omapfb omapfb: resetting (status 0xffffffb2,reset count 1)
> As a visible result, image pixels happened to be shifted by a few bits,
> giving wrong colors.
>
> Examining the code, I found that this problem occures when an OMAP1
> internal LCD controller is disabled from omap_lcdc_suspend() and then a
> subsequent omap_lcdc_setup_plane() calls disable_controller() again. This
> potentially error provoking behaviour is triggered by the lcdc.update_mode
> flag being kept at OMAP_AUTO_UPDATE, regardless of the controller and panel
> being suspended.
>
> This patch tries to correct the problem by replacing both
> omap_lcdc_suspend() and omap_lcdc_resume() function bodies with single
> calls to
> omap_lcdc_set_update_mode() with a respective OMAP_UPDATE_DISABLE or
> OMAP_AUTO_UPDATE argument. As a result, exactly the same lower level
> operations are performed, with addition of changing the lcdc.update_mode
> flag to a value better suited for the controller state. This prevents any
> further calls to disable_controller() from omap_lcdc_setup_plane() while
> the display is suspended.

Hi,

One more week of successfull, trouble-free testing on my side. Although there 
were no other test reports, no objections nor change requests were raised as 
well on this relatively simple and straightforward patch. Could we then agree 
on it being accepted for inclusion? Tomi?

Thanks,
Janusz

> Created against linux-2.6.34-rc7.
> Tested on Amstrad Delta.
>
> Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
> ---
> Hi,
>
> Those of you having access to other display equipped OMAP1 boards, could
> you please test if this patch doesn't break anything?
>
> Thanks,
> Janusz
>
>  drivers/video/omap/lcdc.c |   14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> --- git/drivers/video/omap/lcdc.c.orig	2010-05-16 21:39:20.000000000 +0200
> +++ git/drivers/video/omap/lcdc.c	2010-05-17 00:00:43.000000000 +0200
> @@ -572,22 +572,12 @@ static enum omapfb_update_mode omap_lcdc
>  /* PM code called only in internal controller mode */
>  static void omap_lcdc_suspend(void)
>  {
> -	if (lcdc.update_mode = OMAPFB_AUTO_UPDATE) {
> -		disable_controller();
> -		omap_stop_lcd_dma();
> -	}
> +	omap_lcdc_set_update_mode(OMAPFB_UPDATE_DISABLED);
>  }
>
>  static void omap_lcdc_resume(void)
>  {
> -	if (lcdc.update_mode = OMAPFB_AUTO_UPDATE) {
> -		setup_regs();
> -		load_palette();
> -		setup_lcd_dma();
> -		set_load_mode(OMAP_LCDC_LOAD_FRAME);
> -		enable_irqs(OMAP_LCDC_IRQ_DONE);
> -		enable_controller();
> -	}
> +	omap_lcdc_set_update_mode(OMAPFB_AUTO_UPDATE);
>  }
>
>  static void omap_lcdc_get_caps(int plane, struct omapfb_caps *caps)

Re: [RFC][RFT][PATCH] OMAPFB: LCDC: change update_mode to DISABLED

[Tomi Valkeinen]

Hi,

On Sun, 2010-05-23 at 14:09 +0200, ext Janusz Krzysztofik wrote:
> Monday 17 May 2010 03:20:13 Janusz Krzysztofik wrote:
> > I was observing the following error messages on my OMAP1 based Amstrad
> > Delta board when first changing from text to graphics mode or vice versa
> > after the LCD display had been blanked:
> > 	omapfb omapfb: timeout waiting for FRAME DONE
> > with a followup error message while unblanking it back:
> > 	omapfb omapfb: resetting (status 0xffffffb2,reset count 1)
> > As a visible result, image pixels happened to be shifted by a few bits,
> > giving wrong colors.
> >
> > Examining the code, I found that this problem occures when an OMAP1
> > internal LCD controller is disabled from omap_lcdc_suspend() and then a
> > subsequent omap_lcdc_setup_plane() calls disable_controller() again. This
> > potentially error provoking behaviour is triggered by the lcdc.update_mode
> > flag being kept at OMAP_AUTO_UPDATE, regardless of the controller and panel
> > being suspended.
> >
> > This patch tries to correct the problem by replacing both
> > omap_lcdc_suspend() and omap_lcdc_resume() function bodies with single
> > calls to
> > omap_lcdc_set_update_mode() with a respective OMAP_UPDATE_DISABLE or
> > OMAP_AUTO_UPDATE argument. As a result, exactly the same lower level
> > operations are performed, with addition of changing the lcdc.update_mode
> > flag to a value better suited for the controller state. This prevents any
> > further calls to disable_controller() from omap_lcdc_setup_plane() while
> > the display is suspended.
> 
> Hi,
> 
> One more week of successfull, trouble-free testing on my side. Although there 
> were no other test reports, no objections nor change requests were raised as 
> well on this relatively simple and straightforward patch. Could we then agree 
> on it being accepted for inclusion? Tomi?

Yep, looks ok to me. Applied to DSS tree.

 Tomi

Re: [RFC][RFT][PATCH] OMAPFB: LCDC: change update_mode to DISABLED when going suspend

[Janusz Krzysztofik]

Monday 24 May 2010 09:26:03 Tomi Valkeinen napisał(a):
> Hi,
>
> On Sun, 2010-05-23 at 14:09 +0200, ext Janusz Krzysztofik wrote:
> > Monday 17 May 2010 03:20:13 Janusz Krzysztofik wrote:
> > > I was observing the following error messages on my OMAP1 based Amstrad
> > > Delta board when first changing from text to graphics mode or vice
> > > versa after the LCD display had been blanked:
> > > 	omapfb omapfb: timeout waiting for FRAME DONE
> > > with a followup error message while unblanking it back:
> > > 	omapfb omapfb: resetting (status 0xffffffb2,reset count 1)
> > > As a visible result, image pixels happened to be shifted by a few bits,
> > > giving wrong colors.
> > >
> > > Examining the code, I found that this problem occures when an OMAP1
> > > internal LCD controller is disabled from omap_lcdc_suspend() and then a
> > > subsequent omap_lcdc_setup_plane() calls disable_controller() again.
> > > This potentially error provoking behaviour is triggered by the
> > > lcdc.update_mode flag being kept at OMAP_AUTO_UPDATE, regardless of the
> > > controller and panel being suspended.
> > >
> > > This patch tries to correct the problem by replacing both
> > > omap_lcdc_suspend() and omap_lcdc_resume() function bodies with single
> > > calls to
> > > omap_lcdc_set_update_mode() with a respective OMAP_UPDATE_DISABLE or
> > > OMAP_AUTO_UPDATE argument. As a result, exactly the same lower level
> > > operations are performed, with addition of changing the
> > > lcdc.update_mode flag to a value better suited for the controller
> > > state. This prevents any further calls to disable_controller() from
> > > omap_lcdc_setup_plane() while the display is suspended.
> >
> > Hi,
> >
> > One more week of successfull, trouble-free testing on my side. Although
> > there were no other test reports, no objections nor change requests were
> > raised as well on this relatively simple and straightforward patch. Could
> > we then agree on it being accepted for inclusion? Tomi?
>
> Yep, looks ok to me. Applied to DSS tree.

Tomi,
Thanks, you were so fast that I missed asking you if it could still be pushed 
as a fix in the upcoming 2.6.35-rc cycle.

Thanks,
Janusz