Shifted framebuffer after X11 starts on mx28

Hi,

I am running 3.8.2 kernel on a mx28evk and when I start X11 I get a
"shifted" framebuffer.

The framebuffer works well if X11 is not called.

There is no issue with X11 when running with Freescale  2.6.35 kernel.

Looking at drivers/video/mxsfb.c there is the following comment:

	/*
	 * It seems, you can't re-program the controller if it is still running.
	 * This may lead into shifted pictures (FIFO issue?).
	 * So, first stop the controller and drain its FIFOs
	 */

So, it looks like that I am still facing this issue when X11 starts.

If anyone has any suggestions, please let me know.

Thanks,

Fabio Estevam

Dear Fabio Estevam,

Hi,

I am running 3.8.2 kernel on a mx28evk and when I start X11 I get a
"shifted" framebuffer.

The framebuffer works well if X11 is not called.

There is no issue with X11 when running with Freescale  2.6.35 kernel.

Looking at drivers/video/mxsfb.c there is the following comment:

	/*
	 * It seems, you can't re-program the controller if it is still running.
	 * This may lead into shifted pictures (FIFO issue?).
	 * So, first stop the controller and drain its FIFOs
	 */

So, it looks like that I am still facing this issue when X11 starts.

If anyone has any suggestions, please let me know.

This is not it, I almost hunted the bug down. Hang on.

Basically check mxsfb_set_par() and observe fb_info->var.sync . It's set to 
"something" during regular framebuffer operation and it's set to "something 
else" during Xorg operation. And that's what causes it to get broken. I forced 
fb_info->var.sync to the "something" value by hardcoding it in kernel just now 
for a quick test and X works for me.

Best regards,
Marek Vasut

Dear Marek Vasut,

Dear Fabio Estevam,

quoted

Hi,

I am running 3.8.2 kernel on a mx28evk and when I start X11 I get a
"shifted" framebuffer.

The framebuffer works well if X11 is not called.

There is no issue with X11 when running with Freescale  2.6.35 kernel.

Looking at drivers/video/mxsfb.c there is the following comment:
	/*
	
	 * It seems, you can't re-program the controller if it is still running.
	 * This may lead into shifted pictures (FIFO issue?).
	 * So, first stop the controller and drain its FIFOs
	 */

So, it looks like that I am still facing this issue when X11 starts.

If anyone has any suggestions, please let me know.

This is not it, I almost hunted the bug down. Hang on.

Basically check mxsfb_set_par() and observe fb_info->var.sync . It's set to
"something" during regular framebuffer operation and it's set to "something
else" during Xorg operation. And that's what causes it to get broken. I
forced fb_info->var.sync to the "something" value by hardcoding it in
kernel just now for a quick test and X works for me.

Uh, being in hacking-mode and replying to email doesn't work well together :-)

In my case, fb_info->var.sync is set to FB_SYNC_DATA_ENABLE_HIGH_ACT during 
regular operation (framebuffer console), but if X11 starts, this fb_info-

var.sync is forced to value 0 which causes the malfunction. Now back into 

hacking-mode ;-)

Best regards,
Marek Vasut

The issue fixed by this patch manifests only then using X11
with mxsfb driver. The X11 will display either shifted image
or otherwise distorted image on the LCD.

The problem is that the X11 tries to reconfigure the framebuffer
and along the way call fb_ops.fb_set_par() with it's configuration
values. The field of particular interest is fb_info->var.sync which
contains non-standard values if configured by kernel. These are
FB_SYNC_DATA_ENABLE_HIGH_ACT and FB_SYNC_DOTCLK_FAILING_ACT defined
in include/linux/mxsfb.h . The driver interprets those and configures
the LCD controller accordingly. Yet X11 only has access to standard
values for this field defined in include/uapi/linux/fb.h and thus
omits these special values. This results in distorted image on the
LCD.

This patch moves these non-standard values into new field of the
mxsfb_platform_data structure so the driver can in turn check this
field instead of the video mode field for these specific portions.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <redacted>
Cc: Linux ARM <redacted>
Cc: Linux FBDEV <redacted>
Cc: Lothar Waßmann <redacted>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Shawn Guo <redacted>
---
 arch/arm/mach-mxs/mach-mxs.c |   22 +++++++++++-----------
 drivers/video/mxsfb.c        |    7 +++++--
 include/linux/mxsfb.h        |    7 +++++--
 3 files changed, 21 insertions(+), 15 deletions(-)

NOTE: We ought to get rid of this platform data goo, but we can't do
      it in here. This is a bugfix.

diff --git a/arch/arm/mach-mxs/mach-mxs.c b/arch/arm/mach-mxs/mach-mxs.c
index c66129b..ebf592c 100644
--- a/arch/arm/mach-mxs/mach-mxs.c
+++ b/arch/arm/mach-mxs/mach-mxs.c

@@ -41,8 +41,6 @@ static struct fb_videomode mx23evk_video_modes[] = {
 		.lower_margin	= 4,
 		.hsync_len	= 1,
 		.vsync_len	= 1,
-		.sync		= FB_SYNC_DATA_ENABLE_HIGH_ACT |
-				  FB_SYNC_DOTCLK_FAILING_ACT,
 	},
 };
 

@@ -59,8 +57,6 @@ static struct fb_videomode mx28evk_video_modes[] = {
 		.lower_margin	= 10,
 		.hsync_len	= 10,
 		.vsync_len	= 10,
-		.sync		= FB_SYNC_DATA_ENABLE_HIGH_ACT |
-				  FB_SYNC_DOTCLK_FAILING_ACT,
 	},
 };
 

@@ -77,7 +73,6 @@ static struct fb_videomode m28evk_video_modes[] = {
 		.lower_margin	= 45,
 		.hsync_len	= 1,
 		.vsync_len	= 1,
-		.sync		= FB_SYNC_DATA_ENABLE_HIGH_ACT,
 	},
 };
 

@@ -94,9 +89,7 @@ static struct fb_videomode apx4devkit_video_modes[] = {
 		.lower_margin	= 13,
 		.hsync_len	= 48,
 		.vsync_len	= 3,
-		.sync		= FB_SYNC_HOR_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT |
-				  FB_SYNC_DATA_ENABLE_HIGH_ACT |
-				  FB_SYNC_DOTCLK_FAILING_ACT,
+		.sync		= FB_SYNC_HOR_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
 	},
 };
 

@@ -113,9 +106,7 @@ static struct fb_videomode apf28dev_video_modes[] = {
 		.lower_margin = 0x15,
 		.hsync_len = 64,
 		.vsync_len = 4,
-		.sync = FB_SYNC_HOR_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT |
-				FB_SYNC_DATA_ENABLE_HIGH_ACT |
-				FB_SYNC_DOTCLK_FAILING_ACT,
+		.sync = FB_SYNC_HOR_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
 	},
 };
 

@@ -250,6 +241,8 @@ static void __init imx23_evk_init(void)
 	mxsfb_pdata.mode_count = ARRAY_SIZE(mx23evk_video_modes);
 	mxsfb_pdata.default_bpp = 32;
 	mxsfb_pdata.ld_intf_width = STMLCDIF_24BIT;
+	mxsfb_pdata.sync = MXSFB_SYNC_DATA_ENABLE_HIGH_ACT |
+				MXSFB_SYNC_DOTCLK_FAILING_ACT;
 }
 
 static inline void enable_clk_enet_out(void)

@@ -269,6 +262,8 @@ static void __init imx28_evk_init(void)
 	mxsfb_pdata.mode_count = ARRAY_SIZE(mx28evk_video_modes);
 	mxsfb_pdata.default_bpp = 32;
 	mxsfb_pdata.ld_intf_width = STMLCDIF_24BIT;
+	mxsfb_pdata.sync = MXSFB_SYNC_DATA_ENABLE_HIGH_ACT |
+				MXSFB_SYNC_DOTCLK_FAILING_ACT;
 
 	mxs_saif_clkmux_select(MXS_DIGCTL_SAIF_CLKMUX_EXTMSTR0);
 }

@@ -288,6 +283,7 @@ static void __init m28evk_init(void)
 	mxsfb_pdata.mode_count = ARRAY_SIZE(m28evk_video_modes);
 	mxsfb_pdata.default_bpp = 16;
 	mxsfb_pdata.ld_intf_width = STMLCDIF_18BIT;
+	mxsfb_pdata.sync = MXSFB_SYNC_DATA_ENABLE_HIGH_ACT;
 }
 
 static void __init sc_sps1_init(void)

@@ -313,6 +309,8 @@ static void __init apx4devkit_init(void)
 	mxsfb_pdata.mode_count = ARRAY_SIZE(apx4devkit_video_modes);
 	mxsfb_pdata.default_bpp = 32;
 	mxsfb_pdata.ld_intf_width = STMLCDIF_24BIT;
+	mxsfb_pdata.sync = MXSFB_SYNC_DATA_ENABLE_HIGH_ACT |
+				MXSFB_SYNC_DOTCLK_FAILING_ACT;
 }
 
 #define ENET0_MDC__GPIO_4_0	MXS_GPIO_NR(4, 0)

@@ -403,6 +401,8 @@ static void __init apf28_init(void)
 	mxsfb_pdata.mode_count = ARRAY_SIZE(apf28dev_video_modes);
 	mxsfb_pdata.default_bpp = 16;
 	mxsfb_pdata.ld_intf_width = STMLCDIF_16BIT;
+	mxsfb_pdata.sync = MXSFB_SYNC_DATA_ENABLE_HIGH_ACT |
+				MXSFB_SYNC_DOTCLK_FAILING_ACT;
 }
 
 static void __init mxs_machine_init(void)

diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
index 755556c..45169cb 100644
--- a/drivers/video/mxsfb.c
+++ b/drivers/video/mxsfb.c

@@ -169,6 +169,7 @@ struct mxsfb_info {
 	unsigned dotclk_delay;
 	const struct mxsfb_devdata *devdata;
 	int mapped;
+	u32 sync;
 };
 
 #define mxsfb_is_v3(host) (host->devdata->ipversion = 3)

@@ -456,9 +457,9 @@ static int mxsfb_set_par(struct fb_info *fb_info)
 		vdctrl0 |= VDCTRL0_HSYNC_ACT_HIGH;
 	if (fb_info->var.sync & FB_SYNC_VERT_HIGH_ACT)
 		vdctrl0 |= VDCTRL0_VSYNC_ACT_HIGH;
-	if (fb_info->var.sync & FB_SYNC_DATA_ENABLE_HIGH_ACT)
+	if (host->sync & MXSFB_SYNC_DATA_ENABLE_HIGH_ACT)
 		vdctrl0 |= VDCTRL0_ENABLE_ACT_HIGH;
-	if (fb_info->var.sync & FB_SYNC_DOTCLK_FAILING_ACT)
+	if (host->sync & MXSFB_SYNC_DOTCLK_FAILING_ACT)
 		vdctrl0 |= VDCTRL0_DOTCLK_ACT_FAILING;
 
 	writel(vdctrl0, host->base + LCDC_VDCTRL0);

@@ -861,6 +862,8 @@ static int mxsfb_probe(struct platform_device *pdev)
 
 	INIT_LIST_HEAD(&fb_info->modelist);
 
+	host->sync = pdata->sync;
+
 	ret = mxsfb_init_fbinfo(host);
 	if (ret != 0)
 		goto error_init_fb;

diff --git a/include/linux/mxsfb.h b/include/linux/mxsfb.h
index f14943d..f80af86 100644
--- a/include/linux/mxsfb.h
+++ b/include/linux/mxsfb.h

@@ -24,8 +24,8 @@
 #define STMLCDIF_18BIT 2 /** pixel data bus to the display is of 18 bit width */
 #define STMLCDIF_24BIT 3 /** pixel data bus to the display is of 24 bit width */
 
-#define FB_SYNC_DATA_ENABLE_HIGH_ACT	(1 << 6)
-#define FB_SYNC_DOTCLK_FAILING_ACT	(1 << 7) /* failing/negtive edge sampling */
+#define MXSFB_SYNC_DATA_ENABLE_HIGH_ACT	(1 << 6)
+#define MXSFB_SYNC_DOTCLK_FAILING_ACT	(1 << 7) /* failing/negtive edge sampling */
 
 struct mxsfb_platform_data {
 	struct fb_videomode *mode_list;

@@ -44,6 +44,9 @@ struct mxsfb_platform_data {
 				 * allocated. If specified,fb_size must also be specified.
 				 * fb_phys must be unused by Linux.
 				 */
+	u32 sync;		/* sync mask, contains MXSFB specifics not
+				 * carried in fb_info->var.sync
+				 */
 };
 
 #endif /* __LINUX_MXSFB_H */

-- 
1.7.10.4

On Sat, Mar 16, 2013 at 7:06 PM, Marek Vasut [off-list ref] wrote:

The issue fixed by this patch manifests only then using X11
with mxsfb driver. The X11 will display either shifted image
or otherwise distorted image on the LCD.

The problem is that the X11 tries to reconfigure the framebuffer
and along the way call fb_ops.fb_set_par() with it's configuration
values. The field of particular interest is fb_info->var.sync which
contains non-standard values if configured by kernel. These are
FB_SYNC_DATA_ENABLE_HIGH_ACT and FB_SYNC_DOTCLK_FAILING_ACT defined
in include/linux/mxsfb.h . The driver interprets those and configures
the LCD controller accordingly. Yet X11 only has access to standard
values for this field defined in include/uapi/linux/fb.h and thus
omits these special values. This results in distorted image on the
LCD.

This patch moves these non-standard values into new field of the
mxsfb_platform_data structure so the driver can in turn check this
field instead of the video mode field for these specific portions.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <redacted>
Cc: Linux ARM <redacted>
Cc: Linux FBDEV <redacted>
Cc: Lothar Waßmann <redacted>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Shawn Guo <redacted>

This fixes the X11 offset issue on my mx28evk, thanks!

Tested-by: Fabio Estevam <redacted>

On Sat, Mar 16, 2013 at 11:06:21PM +0100, Marek Vasut wrote:

The issue fixed by this patch manifests only then using X11
with mxsfb driver. The X11 will display either shifted image
or otherwise distorted image on the LCD.

The problem is that the X11 tries to reconfigure the framebuffer
and along the way call fb_ops.fb_set_par() with it's configuration
values. The field of particular interest is fb_info->var.sync which
contains non-standard values if configured by kernel. These are
FB_SYNC_DATA_ENABLE_HIGH_ACT and FB_SYNC_DOTCLK_FAILING_ACT defined
in include/linux/mxsfb.h . The driver interprets those and configures
the LCD controller accordingly. Yet X11 only has access to standard
values for this field defined in include/uapi/linux/fb.h and thus
omits these special values. This results in distorted image on the
LCD.

This patch moves these non-standard values into new field of the
mxsfb_platform_data structure so the driver can in turn check this
field instead of the video mode field for these specific portions.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <redacted>
Cc: Linux ARM <redacted>
Cc: Linux FBDEV <redacted>
Cc: Lothar Waßmann <redacted>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Shawn Guo <redacted>
---
 arch/arm/mach-mxs/mach-mxs.c |   22 +++++++++++-----------
 drivers/video/mxsfb.c        |    7 +++++--
 include/linux/mxsfb.h        |    7 +++++--
 3 files changed, 21 insertions(+), 15 deletions(-)

Since it touches mach-mxs as well, let me send it through arm-soc tree
as fix for 3.9.

Shawn

On Mon, Mar 18, 2013 at 10:11:47AM -0300, Fabio Estevam wrote:

Shawn Guo wrote:

quoted

Since it touches mach-mxs as well, let me send it through arm-soc tree
as fix for 3.9.

Please CC stable as well, so that we can have X11 functional with 3.8.

Ok, done.

Shawn Guo wrote:

Since it touches mach-mxs as well, let me send it through arm-soc tree
as fix for 3.9.

Please CC stable as well, so that we can have X11 functional with 3.8.

Dear Shawn Guo,

On Mon, Mar 18, 2013 at 10:11:47AM -0300, Fabio Estevam wrote:

quoted

Shawn Guo wrote:

quoted

Since it touches mach-mxs as well, let me send it through arm-soc tree
as fix for 3.9.

Please CC stable as well, so that we can have X11 functional with 3.8.

Ok, done.

Shawn, I think this patch applies to 3.9-rc but there's one more platform in 
mach-mxs.c in 3.9-rc thus this will have compile issue. Just renaming those two 
FB_SYNC_ values to MXSFB_SYNC_ fixes the compile issue on 3.9-rc. Also the 
commit log doesn't look all that cool .... I probably should have marked the 
patch as RFC.

Shall I send you one for 3.9-rc and one for stable maybe ?

Best regards,
Marek Vasut

On Mon, Mar 18, 2013 at 02:58:17PM +0100, Marek Vasut wrote:

Dear Shawn Guo,

quoted

On Mon, Mar 18, 2013 at 10:11:47AM -0300, Fabio Estevam wrote:

quoted

Shawn Guo wrote:

quoted

Since it touches mach-mxs as well, let me send it through arm-soc tree
as fix for 3.9.

Please CC stable as well, so that we can have X11 functional with 3.8.

Ok, done.

Shawn, I think this patch applies to 3.9-rc but there's one more platform in 
mach-mxs.c in 3.9-rc thus this will have compile issue. Just renaming those two 
FB_SYNC_ values to MXSFB_SYNC_ fixes the compile issue on 3.9-rc.

Yes, I noticed that and fixed it when applying the patch.

Also the 
commit log doesn't look all that cool .... I probably should have marked the 
patch as RFC.

Shall I send you one for 3.9-rc and one for stable maybe ?

Yeah, improved patch is welcomed.  So I'm dropping the current one and
waiting for the new one.

Shawn

Dear Shawn Guo,

On Mon, Mar 18, 2013 at 02:58:17PM +0100, Marek Vasut wrote:

quoted

Dear Shawn Guo,

quoted

On Mon, Mar 18, 2013 at 10:11:47AM -0300, Fabio Estevam wrote:

quoted

Shawn Guo wrote:

quoted

Since it touches mach-mxs as well, let me send it through arm-soc
tree as fix for 3.9.

Please CC stable as well, so that we can have X11 functional with
3.8.

Ok, done.

Shawn, I think this patch applies to 3.9-rc but there's one more platform
in mach-mxs.c in 3.9-rc thus this will have compile issue. Just renaming
those two FB_SYNC_ values to MXSFB_SYNC_ fixes the compile issue on
3.9-rc.

Yes, I noticed that and fixed it when applying the patch.

quoted

Also the
commit log doesn't look all that cool .... I probably should have marked
the patch as RFC.

Shall I send you one for 3.9-rc and one for stable maybe ?

Yeah, improved patch is welcomed.  So I'm dropping the current one and
waiting for the new one.

Perfect, latest tomorrow then.

Did you manage to test it please?

Best regards,
Marek Vasut

On Mon, Mar 18, 2013 at 03:18:13PM +0100, Marek Vasut wrote:

Did you manage to test it please?

I do not have X11 environment handy to test, but framework console
seems still working.

Shawn

Dear Shawn Guo,

On Mon, Mar 18, 2013 at 03:18:13PM +0100, Marek Vasut wrote:

quoted

Did you manage to test it please?

I do not have X11 environment handy to test, but framework console
seems still working.

Even after me poking in it, lol :-)

You can grab [1], extract it onto card or NFS and just use it as root 
filesystem, the X11 will come up automagically.

[1] ftp://ftp.denx.de/pub/eldk/5.3/targets/armv5te/core-image-sato-sdk-generic-
armv5te.tar.gz

Best regards,
Marek Vasut

On Mon, Mar 18, 2013 at 04:19:52PM +0100, Marek Vasut wrote:

Dear Shawn Guo,

quoted

On Mon, Mar 18, 2013 at 03:18:13PM +0100, Marek Vasut wrote:

quoted

Did you manage to test it please?

I do not have X11 environment handy to test, but framework console
seems still working.

Even after me poking in it, lol :-)

You can grab [1], extract it onto card or NFS and just use it as root 
filesystem, the X11 will come up automagically.

Thanks, Marek.  Yeah, now I can see the problem and your patch does fix
it.

Shawn

[1] ftp://ftp.denx.de/pub/eldk/5.3/targets/armv5te/core-image-sato-sdk-generic-
armv5te.tar.gz

Dear Shawn Guo,

On Mon, Mar 18, 2013 at 04:19:52PM +0100, Marek Vasut wrote:

quoted

Dear Shawn Guo,

quoted

On Mon, Mar 18, 2013 at 03:18:13PM +0100, Marek Vasut wrote:

quoted

Did you manage to test it please?

I do not have X11 environment handy to test, but framework console
seems still working.

Even after me poking in it, lol :-)

You can grab [1], extract it onto card or NFS and just use it as root
filesystem, the X11 will come up automagically.

Thanks, Marek.  Yeah, now I can see the problem and your patch does fix
it.

Whew, cool. Can you please apply it and push mainline so I can re-send the one 
for -stable then?

Thank you!

Best regards,
Marek Vasut