Merge lp:~vanvugt/compiz/fix-763005-trunk into lp:~compiz/compiz/ubuntu

[Daniel van Vugt]

Fix slow/stuttering display when sync to Vblank is enabled. (LP: #763005)

For fglrx the cause appears to be an ATI/AMD driver bug where glXGetVideoSyncSGI and glXWaitVideoSyncSGI are particularly slow, so we can't afford to call both on every redraw or we end up with 1/3-1/2 framerate. For intel and really all drivers, another cause appears to be excessive waiting in the compiz architecture not allowing enough CPU time to draw the full frame before the next vertical refresh.

This patch addresses both possible problems, providing optimal framerates with sync-to-vblank support, most of the time.

It is possible this might also fix similar nvidia issues where DynamicTwinView reduces the framerate; bug 762749 and maybe bug 92599...

[Didier Roche-Tolomelli (didrocks)]

Sam, can you look at this? Thanks!

[Sam Spilsbury (smspillaz)]

Sorry I got to this so late.

This looks good. Could you possibly retarget this against lp:compiz-core and make sure comments are on a new line ?

review: Approve

Unmerged revisions

648. By Daniel van Vugt on 2011-08-12

Fix slow/stuttering display when sync to Vblank is enabled. (LP: #763005)

=== added file 'debian/patches/fix_slow_vsync_lp763005.patch'
--- debian/patches/fix_slow_vsync_lp763005.patch	1970-01-01 00:00:00 +0000
+++ debian/patches/fix_slow_vsync_lp763005.patch	2011-08-12 07:46:22 +0000
@@ -0,0 +1,175 @@
+Description: Fix slow/stuttering display when sync to Vblank is enabled.
+ For fglrx the cause appears to be an ATI/AMD driver bug where
+ glXGetVideoSyncSGI and glXWaitVideoSyncSGI are particularly slow, so we
+ can't afford to call both on every redraw or we end up with 1/3-1/2 framerate.
+ For intel and really all drivers, another cause appears to be excessive
+ waiting in the compiz architecture not allowing enough CPU time to draw the
+ full frame before the next vertical refresh.
+ This patch addresses both possible problems, providing optimal framerates
+ with sync-to-vblank support, most of the time.
+Author: Daniel van Vugt <[e-mail]>
+Bug-Ubuntu: https://launchpad.net/bugs/763005
+
+===================================================================
+--- old/plugins/opengl/include/opengl/opengl.h	2011-08-12 15:00:38.809906000 +0800
++++ new/plugins/opengl/include/opengl/opengl.h	2011-08-12 15:09:05.000000000 +0800
+@@ -98,11 +98,13 @@
+ 					  int	  width,
+ 					  int	  height);
+ 
+-    typedef int (*GLXGetVideoSyncProc)  (unsigned int *count);
+     typedef int (*GLXWaitVideoSyncProc) (int	  divisor,
+ 					 int	  remainder,
+ 					 unsigned int *count);
+ 
++    // http://www.opengl.org/registry/specs/SGI/swap_control.txt
++    typedef int (*GLXSwapIntervalProc) (int interval);
++
+     #ifndef GLX_VERSION_1_3
+     typedef struct __GLXFBConfigRec *GLXFBConfig;
+     #endif
+@@ -163,8 +165,8 @@
+     extern GLXReleaseTexImageProc   releaseTexImage;
+     extern GLXQueryDrawableProc     queryDrawable;
+     extern GLXCopySubBufferProc     copySubBuffer;
+-    extern GLXGetVideoSyncProc      getVideoSync;
+     extern GLXWaitVideoSyncProc     waitVideoSync;
++    extern GLXSwapIntervalProc      swapInterval;
+     extern GLXGetFBConfigsProc      getFBConfigs;
+     extern GLXGetFBConfigAttribProc getFBConfigAttrib;
+     extern GLXCreatePixmapProc      createPixmap;
+===================================================================
+--- old/plugins/opengl/src/screen.cpp	2011-08-12 15:00:38.809906000 +0800
++++ new/plugins/opengl/src/screen.cpp	2011-08-12 15:09:05.000000000 +0800
+@@ -36,8 +36,8 @@
+     GLXReleaseTexImageProc   releaseTexImage = NULL;
+     GLXQueryDrawableProc     queryDrawable = NULL;
+     GLXCopySubBufferProc     copySubBuffer = NULL;
+-    GLXGetVideoSyncProc      getVideoSync = NULL;
+     GLXWaitVideoSyncProc     waitVideoSync = NULL;
++    GLXSwapIntervalProc      swapInterval = NULL;
+     GLXGetFBConfigsProc      getFBConfigs = NULL;
+     GLXGetFBConfigAttribProc getFBConfigAttrib = NULL;
+     GLXCreatePixmapProc      createPixmap = NULL;
+@@ -227,13 +227,16 @@
+ 
+     if (strstr (glxExtensions, "GLX_SGI_video_sync"))
+     {
+-	GL::getVideoSync = (GL::GLXGetVideoSyncProc)
+-	    getProcAddress ("glXGetVideoSyncSGI");
+-
+ 	GL::waitVideoSync = (GL::GLXWaitVideoSyncProc)
+ 	    getProcAddress ("glXWaitVideoSyncSGI");
+     }
+ 
++    if (strstr (glxExtensions, "GLX_SGI_swap_control"))
++    {
++	GL::swapInterval = (GL::GLXSwapIntervalProc)
++	    getProcAddress ("glXSwapIntervalSGI");
++    }
++
+     glXMakeCurrent (dpy, CompositeScreen::get (s)->output (), priv->ctx);
+ 
+     glExtensions = (const char *) glGetString (GL_EXTENSIONS);
+@@ -1002,19 +1005,30 @@
+ }
+ 
+ void
+-PrivateGLScreen::waitForVideoSync ()
++PrivateGLScreen::controlSwapVideoSync ()
+ {
+-    unsigned int sync;
+-
+-    if (!optionGetSyncToVblank ())
+-	return;
++    bool sync = optionGetSyncToVblank ();
++    if (GL::swapInterval)
++    {   // Docs: http://www.opengl.org/registry/specs/SGI/swap_control.txt
++	(*GL::swapInterval) (sync ? 1 : 0);
++    }
++    else if (sync)
++    {   // ugly fallback - blocks the CPU.
++	waitForVideoSync ();
++    }
++}
+ 
+-    if (GL::getVideoSync)
++void
++PrivateGLScreen::waitForVideoSync ()
++{
++    if (optionGetSyncToVblank () && GL::waitVideoSync)
+     {
+-	glFlush ();
++	if (GL::swapInterval)
++	    (*GL::swapInterval) (0); // Don't wait twice. Just in case.
+ 
+-	(*GL::getVideoSync) (&sync);
+-	(*GL::waitVideoSync) (2, (sync + 1) % 2, &sync);
++	// Docs: http://www.opengl.org/registry/specs/SGI/video_sync.txt
++	unsigned int frameno;
++	(*GL::waitVideoSync) (1, 0, &frameno);
+     }
+ }
+ 
+@@ -1087,10 +1101,23 @@
+ 
+     targetOutput = &screen->outputDevs ()[0];
+ 
+-    waitForVideoSync ();
++    glFlush ();  // used to be called from waitForVideoSync
+ 
++    //
++    // FIXME: Actually fix the composite plugin to be more efficient;
++    // If GL::swapInterval == NULL && GL::waitVideoSync != NULL then the
++    // composite plugin should not be imposing any framerate restriction
++    // (ie. blocking the CPU) at all. Because the framerate will be controlled
++    // and optimized here:
++    //
+     if (mask & COMPOSITE_SCREEN_DAMAGE_ALL_MASK)
+     {
++	//
++	// controlSwapVideoSync is much faster than waitForVideoSync because
++	// it won't block the CPU. The waiting is offloaded to the GPU.
++	// Unfortunately it only works with glXSwapBuffers in most drivers.
++	//
++	controlSwapVideoSync ();
+ 	glXSwapBuffers (screen->dpy (), cScreen->output ());
+     }
+     else
+@@ -1098,6 +1125,7 @@
+ 	BoxPtr pBox;
+ 	int    nBox, y;
+ 
++	waitForVideoSync ();
+ 	pBox = const_cast <Region> (tmpRegion.handle ())->rects;
+ 	nBox = const_cast <Region> (tmpRegion.handle ())->numRects;
+ 
+@@ -1153,7 +1181,7 @@
+ bool
+ PrivateGLScreen::hasVSync ()
+ {
+-   return (GL::getVideoSync && optionGetSyncToVblank ());
++   return (GL::waitVideoSync && optionGetSyncToVblank ());
+ }
+ 
+ void
+@@ -1161,7 +1189,7 @@
+ {
+     if (pendingCommands)
+     {
+-	glFinish ();
++	glFlush ();  // glFlush! glFinish would block the CPU, which is bad.
+ 	pendingCommands = false;
+     }
+ }
+===================================================================
+--- old/plugins/opengl/src/privates.h	2011-08-12 15:10:58.007681085 +0800
++++ new/plugins/opengl/src/privates.h	2011-08-12 15:13:02.617681069 +0800
+@@ -70,6 +70,7 @@
+ 
+ 	void prepareDrawing ();
+ 
++	void controlSwapVideoSync ();
+ 	void waitForVideoSync ();
+ 
+ 	void paintBackground (const CompRegion &region,
 
=== modified file 'debian/patches/series'
--- debian/patches/series	2011-08-12 07:07:23 +0000
+++ debian/patches/series	2011-08-12 07:46:22 +0000
@@ -1,2 +1,3 @@
 01_don_t_init_a11y.patch
 091_no_use_gnome_but_desktop_file.patch
+fix_slow_vsync_lp763005.patch