matroxfb: another attempt to rectify jitter (G450/G550)

PATCH In review Paul A. Clarke 3 participants 8 messages Jan 23, 2007 – Feb 17, 2007 1 message not in the archive Framebuffer layer

lore

Share & tools

lore

Paul A. Clarke

author

2007-01-23 00:18 UTC #0 lore raw

PATCH 1/1 matroxfb: another attempt to rectify jitter (G450/G550)

This builds upon my previous attempts to resolve some jitter problems seen with the Matrox G450 and G550 -based cards, including odd disparities observed between x86 and Power -based machines in a somewhat less hackish way (removing the hacked ifdefs).

Apparently, preference should be given to use the DVI PLL when frequencies permit, the Standard PLL otherwise. The max pixel clock for the panellink interface is extracted from the PInS information on the card and used as a limit to determine which PLL to use.

Signed-off-by Paul A. Clarke <[email protected]>

Signature · 90 lines

--- linux-2.6.19-rc6/drivers/video/matrox/matroxfb_base.h	2006-11-15 
22:03:40.000000000 -0600
+++ patched/drivers/video/matrox/matroxfb_base.h	2006-11-22 13:02:58.000000000 -0600
@@ -424,6 +424,7 @@
  		      } mmio;

  	unsigned int	max_pixel_clock;
+	unsigned int	max_pixel_clock_panellink;

  	struct matrox_switch*	hw_switch;


--- linux-2.6.19-rc6/drivers/video/matrox/matroxfb_DAC1064.h	2006-11-15 
22:03:40.000000000 -0600
+++ patched/drivers/video/matrox/matroxfb_DAC1064.h	2006-11-16 
08:43:33.000000000 -0600
@@ -40,6 +40,21 @@
  #define M1064_XCURCOL1GREEN	0x0D
  #define M1064_XCURCOL1BLUE	0x0E
  #define M1064_XDVICLKCTRL	0x0F
+	/* drive DVI by standard(0)/DVI(1) PLL */
+	/* if set(1), C?DVICLKEN and C?DVICLKSEL must be set(1) */
+#define      M1064_XDVICLKCTRL_DVIDATAPATHSEL   0x01
+	/* drive CRTC1 by standard(0)/DVI(1) PLL */
+#define      M1064_XDVICLKCTRL_C1DVICLKSEL      0x02
+	/* drive CRTC2 by standard(0)/DVI(1) PLL */
+#define      M1064_XDVICLKCTRL_C2DVICLKSEL      0x04
+	/* pixel clock allowed to(0)/blocked from(1) driving CRTC1 */
+#define      M1064_XDVICLKCTRL_C1DVICLKEN       0x08
+	/* DVI PLL loop filter bandwidth selection bits */
+#define      M1064_XDVICLKCTRL_DVILOOPCTL       0x30
+	/* CRTC2 pixel clock allowed to(0)/blocked from(1) driving CRTC2 */
+#define      M1064_XDVICLKCTRL_C2DVICLKEN       0x40
+	/* P1PLL loop filter bandwith selection */
+#define      M1064_XDVICLKCTRL_P1LOOPBWDTCTL    0x80
  #define M1064_XCURCOL2RED	0x10
  #define M1064_XCURCOL2GREEN	0x11
  #define M1064_XCURCOL2BLUE	0x12

--- linux-2.6.19-rc6/drivers/video/matrox/matroxfb_misc.c	2006-11-15 
22:03:40.000000000 -0600
+++ patched/drivers/video/matrox/matroxfb_misc.c	2006-11-22 13:03:46.000000000 -0600
@@ -658,6 +658,7 @@
  		MINFO->values.reg.mctlwtst_core = (MINFO->values.reg.mctlwtst & ~7) |
  		                                  wtst_xlat[MINFO->values.reg.mctlwtst & 7];
  	}
+	MINFO->max_pixel_clock_panellink = bd->pins[47] * 4000;
  	return 0;
  }


--- linux-2.6.19-rc6/drivers/video/matrox/g450_pll.c	2006-11-15 
22:03:40.000000000 -0600
+++ patched/drivers/video/matrox/g450_pll.c	2006-11-22 13:04:31.000000000 -0600
@@ -331,15 +331,15 @@
  					tmp |= M1064_XPIXCLKCTRL_PLL_UP;
  				}
  				matroxfb_DAC_out(PMINFO M1064_XPIXCLKCTRL, tmp);
-#ifdef __powerpc__
-				/* This is necessary to avoid jitter on PowerPC
-				 * (OpenFirmware) systems, but apparently
-				 * introduces jitter, at least on a x86-64
-				 * using DVI.
-				 * A simple workaround is disable for non-PPC.
-				 */
-				matroxfb_DAC_out(PMINFO M1064_XDVICLKCTRL, 0);
-#endif /* __powerpc__ */
+				/* DVI PLL preferred for frequencies up to panellink max, standard PLL 
otherwise */
+				if (fout >= MINFO->max_pixel_clock_panellink) tmp = 0;
+				else tmp =
+					M1064_XDVICLKCTRL_DVIDATAPATHSEL |
+					M1064_XDVICLKCTRL_C1DVICLKSEL |
+					M1064_XDVICLKCTRL_C1DVICLKEN |
+					M1064_XDVICLKCTRL_DVILOOPCTL |
+					M1064_XDVICLKCTRL_P1LOOPBWDTCTL;
+				matroxfb_DAC_out(PMINFO M1064_XDVICLKCTRL,tmp);
  				matroxfb_DAC_out(PMINFO M1064_XPWRCTRL, xpwrctrl);

  				matroxfb_DAC_unlock_irqrestore(flags);

--
Regards,
Paul A. Clarke, IBM

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

A message that is not in the archive (sent off-list or not yet received): [email protected]

Paul A. Clarke

author Acked-by Petr Vandrovec

2007-01-30 20:25 UTC #2 lore raw

I'm resubmitting this with Petr's "Ack", which he sent to me privately.

Could this patch be added to -mm ?

Quoted 10 lines · Petr Vandrovec

I assume that Matrox blessed this code ;-) Unfortunately I cannot
test it anymore as I [,,,] have no
Matrox hardware anymore... So let's try to put it into -mm kernels, and let's see what happens ;-)
Acked-by Petr Vandrovec <[email protected]>
[somehow Mozilla dropped patch from reply...]
Petr

--
This builds upon my previous attempts to resolve some jitter problems seen with the Matrox G450 and G550 -based cards, including odd disparities observed between x86 and Power -based machines in a somewhat less hackish way (removing the hacked ifdefs for architecture).

Signed-off-by Paul A. Clarke <[email protected]>

Acked-by Petr Vandrovec <[email protected]>

--- linux-2.6.19-rc6/drivers/video/matrox/matroxfb_base.h 2006-11-15
22:03:40.000000000 -0600
+++ patched/drivers/video/matrox/matroxfb_base.h 2006-11-22 13:02:58.000000000 -0600
@@ -424,6 +424,7 @@
} mmio;

unsigned int max_pixel_clock;
+ unsigned int max_pixel_clock_panellink;

struct matrox_switch* hw_switch;

--- linux-2.6.19-rc6/drivers/video/matrox/matroxfb_DAC1064.h 2006-11-15
22:03:40.000000000 -0600
+++ patched/drivers/video/matrox/matroxfb_DAC1064.h 2006-11-16
08:43:33.000000000 -0600
@@ -40,6 +40,21 @@
#define M1064_XCURCOL1GREEN 0x0D
#define M1064_XCURCOL1BLUE 0x0E
#define M1064_XDVICLKCTRL 0x0F
+ /* drive DVI by standard(0)/DVI(1) PLL */
+ /* if set(1), C?DVICLKEN and C?DVICLKSEL must be set(1) */
+#define M1064_XDVICLKCTRL_DVIDATAPATHSEL 0x01
+ /* drive CRTC1 by standard(0)/DVI(1) PLL */
+#define M1064_XDVICLKCTRL_C1DVICLKSEL 0x02
+ /* drive CRTC2 by standard(0)/DVI(1) PLL */
+#define M1064_XDVICLKCTRL_C2DVICLKSEL 0x04
+ /* pixel clock allowed to(0)/blocked from(1) driving CRTC1 */
+#define M1064_XDVICLKCTRL_C1DVICLKEN 0x08
+ /* DVI PLL loop filter bandwidth selection bits */
+#define M1064_XDVICLKCTRL_DVILOOPCTL 0x30
+ /* CRTC2 pixel clock allowed to(0)/blocked from(1) driving CRTC2 */
+#define M1064_XDVICLKCTRL_C2DVICLKEN 0x40
+ /* P1PLL loop filter bandwith selection */
+#define M1064_XDVICLKCTRL_P1LOOPBWDTCTL 0x80
#define M1064_XCURCOL2RED 0x10
#define M1064_XCURCOL2GREEN 0x11
#define M1064_XCURCOL2BLUE 0x12

--- linux-2.6.19-rc6/drivers/video/matrox/matroxfb_misc.c 2006-11-15
22:03:40.000000000 -0600
+++ patched/drivers/video/matrox/matroxfb_misc.c 2006-11-22 13:03:46.000000000 -0600
@@ -658,6 +658,7 @@
MINFO->values.reg.mctlwtst_core = (MINFO->values.reg.mctlwtst & ~7) |
wtst_xlat[MINFO->values.reg.mctlwtst & 7];
}
+ MINFO->max_pixel_clock_panellink = bd->pins[47] * 4000;
return 0;
}

--- linux-2.6.19-rc6/drivers/video/matrox/g450_pll.c 2006-11-15
22:03:40.000000000 -0600
+++ patched/drivers/video/matrox/g450_pll.c 2006-11-22 13:04:31.000000000 -0600
@@ -331,15 +331,15 @@
tmp |= M1064_XPIXCLKCTRL_PLL_UP;
}
matroxfb_DAC_out(PMINFO M1064_XPIXCLKCTRL, tmp);
-#ifdef __powerpc__
- /* This is necessary to avoid jitter on PowerPC
- * (OpenFirmware) systems, but apparently
- * introduces jitter, at least on a x86-64
- * using DVI.
- * A simple workaround is disable for non-PPC.
- */
- matroxfb_DAC_out(PMINFO M1064_XDVICLKCTRL, 0);
-#endif /* __powerpc__ */
+ /* DVI PLL preferred for frequencies up to panellink max, standard PLL
otherwise */
+ if (fout >= MINFO->max_pixel_clock_panellink) tmp = 0;
+ else tmp =
+ M1064_XDVICLKCTRL_DVIDATAPATHSEL |
+ M1064_XDVICLKCTRL_C1DVICLKSEL |
+ M1064_XDVICLKCTRL_C1DVICLKEN |
+ M1064_XDVICLKCTRL_DVILOOPCTL |
+ M1064_XDVICLKCTRL_P1LOOPBWDTCTL;
+ matroxfb_DAC_out(PMINFO M1064_XDVICLKCTRL,tmp);
matroxfb_DAC_out(PMINFO M1064_XPWRCTRL, xpwrctrl);

   				matroxfb_DAC_unlock_irqrestore(flags);

--
Regards,
Paul A. Clarke, IBM

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Ville Syrjälä

2007-01-31 11:54 UTC #3 lore raw

On Tue, Jan 30, 2007 at 02:22:37PM -0600, Paul A. Clarke wrote:
snip...

Review

Quoted 7 lines

(quoted diff) +1 -0

+				/* DVI PLL preferred for frequencies up to panellink max, standard PLL

otherwise */

(quoted diff) +5 -0

+				if (fout >= MINFO->max_pixel_clock_panellink) tmp = 0;
+				else tmp =
+					M1064_XDVICLKCTRL_DVIDATAPATHSEL |
+					M1064_XDVICLKCTRL_C1DVICLKSEL |
+					M1064_XDVICLKCTRL_C1DVICLKEN |

Shouldn't this check which CRTC is driving the DVI?

Quoted 7 lines

(quoted diff) +3 -0

+					M1064_XDVICLKCTRL_DVILOOPCTL |
+					M1064_XDVICLKCTRL_P1LOOPBWDTCTL;
+				matroxfb_DAC_out(PMINFO M1064_XDVICLKCTRL,tmp);
   				matroxfb_DAC_out(PMINFO M1064_XPWRCTRL, xpwrctrl);
 
   				matroxfb_DAC_unlock_irqrestore(flags);

Signature · 9 lines

Ville Syrjälä
[email protected]
http://www.sci.fi/~syrjala/

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Paul A. Clarke

author

2007-01-31 21:59 UTC #4 lore raw

Quoted 11 lines · Ville Syrjälä

On Tue, Jan 30, 2007 at 02:22:37PM -0600, Paul A. Clarke wrote:
snip...
Review
Quoted 7 lines
(quoted diff) +1 -0
+				/* DVI PLL preferred for frequencies up to panellink max, standard PLL
otherwise */
(quoted diff) +5 -0
+				if (fout >= MINFO->max_pixel_clock_panellink) tmp = 0;
+				else tmp =
+					M1064_XDVICLKCTRL_DVIDATAPATHSEL |
+					M1064_XDVICLKCTRL_C1DVICLKSEL |
+					M1064_XDVICLKCTRL_C1DVICLKEN |
Shouldn't this check which CRTC is driving the DVI?

If I read the code correctly, I don't see any way this code gets invoked for CRTC2. __g450_setclk() is only called by matroxfb_g450_setclk(), which is in turn called in 8 places:
- 2 with pll == M_VIDEO_PLL (a different case in the enclosing switch)
- 1 with pll == M_SYSTEM_PLL (a different case in the enclosing switch)
- 3 which if (m->crtc != MATROXFB_SRC_CRTC1) then pll == M_VIDEO_PLL
- 2 with M_PIXEL_PLL_A and M_PIXEL_PLL_B (once each) at init time to set legacy VGA clocks with both CRTCs disabled

Quoted 6 lines

Review

Quoted 6 lines

(quoted diff) +3 -0

+					M1064_XDVICLKCTRL_DVILOOPCTL |
+					M1064_XDVICLKCTRL_P1LOOPBWDTCTL;
+				matroxfb_DAC_out(PMINFO M1064_XDVICLKCTRL,tmp);
   				matroxfb_DAC_out(PMINFO M1064_XPWRCTRL, xpwrctrl);
 
   				matroxfb_DAC_unlock_irqrestore(flags);

I'll also admit that my level of expertise here is not what I'd prefer, but my (perhaps suboptimal) testing showed good results.

Signature · 8 lines

Regards,
Paul Clarke

-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Ville Syrjälä

2007-01-31 22:37 UTC #5 lore raw

Quoted 13 lines · Paul A. Clarke

Quoted 11 lines · Ville Syrjälä
On Tue, Jan 30, 2007 at 02:22:37PM -0600, Paul A. Clarke wrote:
snip...
Review
Quoted 7 lines
(quoted diff) +1 -0
+				/* DVI PLL preferred for frequencies up to panellink max, standard PLL
otherwise */
(quoted diff) +5 -0
+				if (fout >= MINFO->max_pixel_clock_panellink) tmp = 0;
+				else tmp =
+					M1064_XDVICLKCTRL_DVIDATAPATHSEL |
+					M1064_XDVICLKCTRL_C1DVICLKSEL |
+					M1064_XDVICLKCTRL_C1DVICLKEN |
Shouldn't this check which CRTC is driving the DVI?

Quoted 6 lines

If I read the code correctly, I don't see any way this code gets invoked for CRTC2. __g450_setclk() is only called by matroxfb_g450_setclk(), which is in turn called in 8 places:
- 2 with pll == M_VIDEO_PLL (a different case in the enclosing switch)
- 1 with pll == M_SYSTEM_PLL (a different case in the enclosing switch)
- 3 which if (m->crtc != MATROXFB_SRC_CRTC1) then pll == M_VIDEO_PLL
- 2 with M_PIXEL_PLL_A and M_PIXEL_PLL_B (once each) at init time to set legacy VGA clocks with both CRTCs disabled

I see. It seems to always use VIDPLL with CRTC2. So if DVICLK is to be used with CRTC2 it apparently needs to be added in the VIDPLL case. Hmmm. I wonder if the code even supports CRTC2+DVI because the CRTC2 code seems to leave the C2PIXCLK selection to PCICLK if CRTC2 drives DVI only. Did you try to map CRTC2 to DVI?

Unfortunately I don't have a TFT so I can't actually test this stuff. Probably time to get one... :)

Signature · 9 lines

Ville Syrjälä
[email protected]
http://www.sci.fi/~syrjala/

-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Paul A. Clarke

author

2007-02-15 20:48 UTC #6 lore raw

Ville,

Quoted 5 lines · Ville Syrjälä

I see. It seems to always use VIDPLL with CRTC2. So if DVICLK is to be used with CRTC2 it apparently needs to be added in the VIDPLL case. Hmmm. I wonder if the code even supports CRTC2+DVI because the CRTC2 code seems to leave the C2PIXCLK selection to PCICLK if CRTC2 drives DVI only. Did you try to map CRTC2 to DVI?

So, I just (finally) ran the following tests, on a SLES10 system (2.6.16+) with G450, both with and without the patch (results were identical):

- with "video=matroxfb:outputs:012"
D-shell DVI-D
on boot OK junk
X dual fb0/fb1 OK OK
X fb0 only black(!) junk
X fb1 only signal problems(!) OK, but yellow(?)

- with "video=matroxfb:outputs:021"
D-shell DVI-D

on boot            junk                   OK
X dual fb0/fb1      OK                    OK
X fb0 only         junk                   OK

X fb1 only OK, but yellow OK (blank)

Note that I ignored the DVI-A output, which is apparently the first "output". Earlier testing showed pretty much what I'd expect on that channel.

So, it appears that CRTC2 can indeed drive DVI-D.

It also appears that this area still needs work, but any objections to moving the suggested patch into -mm ? It provides clean signals and removes the platform-specific hacked ifdefs.

Signature · 8 lines

Regards,
Paul Clarke

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Ville Syrjälä

2007-02-16 17:30 UTC #7 lore raw

Quoted 4 lines · Paul A. Clarke

It also appears that this area still needs work, but any objections to moving the suggested patch into -mm ? It provides clean signals and removes the platform-specific hacked ifdefs.

No objections from me at least.

Signature · 9 lines

Ville Syrjälä
[email protected]
http://www.sci.fi/~syrjala/

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Petr Vandrovec

2007-02-17 06:34 UTC #8 lore raw

Quoted 6 lines · Ville Syrjälä

Quoted 3 lines · Paul A. Clarke
It also appears that this area still needs work, but any objections to moving the suggested patch into -mm ? It provides clean signals and removes the platform-specific hacked ifdefs.
No objections from me at least.

I'm fine with this as well, as I announced earlier...
Petr

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV