Merge lp:~sil2100/compiz/revert_3427_0.9.8 into lp:compiz/0.9.8

[Łukasz Zemczak]

Revert revision 3427 - it causes regressions on VirtualBox

Re-submitting the same merge request. The real fix for the issues that has been commited in lp:compiz is a series of more risky changes, while we want to release a quick compiz update this week. Reverting this revision unblocks compiz for quantal VirtualBox.

So my proposition: for now, a revert, and then we can re-introduce the backported fix along with the right fix for the VB driver.

- Problem:

The current 0.9.8 snapshot branch causes crashes and hangups on VirtualBox. Francis bisected it to be caused by fix of LP: #1060327 .

- Fix:

Reverting revision 3427 (fix for LP: #1060327).

- Tests:

N/A

[Sam Spilsbury (smspillaz)]

I would suggest updating virtual box instead. They told me they've fixed
the bug on their end.

Putting this fix in will likely regress the crash with llvmpipe.
On 25/01/2013 3:17 AM, "Łukasz Zemczak" <email address hidden>
wrote:

> Łukasz Zemczak has proposed merging lp:~sil2100/compiz/revert_3427_0.9.8
> into lp:compiz/0.9.8.
>
> Commit message:
> Revert revision 3427 - it seems to cause regressions on VirtualBox for
> quantal.
>
> Requested reviews:
> Compiz Maintainers (compiz-team)
>
> For more details, see:
> https://code.launchpad.net/~sil2100/compiz/revert_3427_0.9.8/+merge/144764
>
> - Problem:
>
> The current 0.9.8 snapshot branch causes crashes and hangups on
> VirtualBox. Francis bisected it to be caused by fix of LP: #1060327 .
>
> - Fix:
>
> Reverting revision 3427 (fix for LP: #1060327). It is said to be fixing
> the regressions.
>
> - Tests:
>
> N/A
> --
> https://code.launchpad.net/~sil2100/compiz/revert_3427_0.9.8/+merge/144764
> Your team Compiz Maintainers is requested to review the proposed merge of
> lp:~sil2100/compiz/revert_3427_0.9.8 into lp:compiz/0.9.8.
>
> === modified file 'plugins/opengl/src/glxtfpbind/src/glx-tfp-bind.cpp'
> --- plugins/opengl/src/glxtfpbind/src/glx-tfp-bind.cpp 2012-12-11
> 04:53:02 +0000
> +++ plugins/opengl/src/glxtfpbind/src/glx-tfp-bind.cpp 2013-01-24
> 18:16:22 +0000
> @@ -52,12 +52,6 @@
> #endif
> if (!checkPixmapValidity (x11Pixmap))
> return false;
> -
> -#ifdef LP_1030891_NOT_FIXED
> - // We need to bind before the above ServerLock is lost
> - bindTexImageEXT (glxPixmap);
> - return true;
> -#endif
> }
>
> bindTexImageEXT (glxPixmap);
>
>
>

[Sam Spilsbury (smspillaz)]

See: https://www.virtualbox.org/ticket/10894

They didn't specify which revision the fix went in though, but they did say
which release.
On Jan 25, 2013 7:19 AM, "Sam Spilsbury" <email address hidden> wrote:

> I would suggest updating virtual box instead. They told me they've fixed
> the bug on their end.
>
> Putting this fix in will likely regress the crash with llvmpipe.
> On 25/01/2013 3:17 AM, "Łukasz Zemczak" <email address hidden>
> wrote:
>
> > Łukasz Zemczak has proposed merging lp:~sil2100/compiz/revert_3427_0.9.8
> > into lp:compiz/0.9.8.
> >
> > Commit message:
> > Revert revision 3427 - it seems to cause regressions on VirtualBox for
> > quantal.
> >
> > Requested reviews:
> > Compiz Maintainers (compiz-team)
> >
> > For more details, see:
> >
> https://code.launchpad.net/~sil2100/compiz/revert_3427_0.9.8/+merge/144764
> >
> > - Problem:
> >
> > The current 0.9.8 snapshot branch causes crashes and hangups on
> > VirtualBox. Francis bisected it to be caused by fix of LP: #1060327 .
> >
> > - Fix:
> >
> > Reverting revision 3427 (fix for LP: #1060327). It is said to be fixing
> > the regressions.
> >
> > - Tests:
> >
> > N/A
> > --
> >
> https://code.launchpad.net/~sil2100/compiz/revert_3427_0.9.8/+merge/144764
> > Your team Compiz Maintainers is requested to review the proposed merge of
> > lp:~sil2100/compiz/revert_3427_0.9.8 into lp:compiz/0.9.8.
> >
> > === modified file 'plugins/opengl/src/glxtfpbind/src/glx-tfp-bind.cpp'
> > --- plugins/opengl/src/glxtfpbind/src/glx-tfp-bind.cpp 2012-12-11
> > 04:53:02 +0000
> > +++ plugins/opengl/src/glxtfpbind/src/glx-tfp-bind.cpp 2013-01-24
> > 18:16:22 +0000
> > @@ -52,12 +52,6 @@
> > #endif
> > if (!checkPixmapValidity (x11Pixmap))
> > return false;
> > -
> > -#ifdef LP_1030891_NOT_FIXED
> > - // We need to bind before the above ServerLock is lost
> > - bindTexImageEXT (glxPixmap);
> > - return true;
> > -#endif
> > }
> >
> > bindTexImageEXT (glxPixmap);
> >
> >
> >
>
> --
> https://code.launchpad.net/~sil2100/compiz/revert_3427_0.9.8/+merge/144764
> Your team Compiz Maintainers is requested to review the proposed merge of
> lp:~sil2100/compiz/revert_3427_0.9.8 into lp:compiz/0.9.8.
>

[Łukasz Zemczak (sil2100)]

Oh, thanks for the info! Will poke Alan to give it a try once again!

[Alan Pope 🍺🐧🐱 🦄 (popey)]

I'm using latest VirtualBox 4.2.6 and the matching vboxvideo 4.2.6 video driver, and it still crashes on start.

[Sam Spilsbury (smspillaz)]

Does lp:compiz crash on startup with that version?
On 26/01/2013 12:55 AM, "Alan Pope" <email address hidden> wrote:

> I'm using latest VirtualBox 4.2.6 and the matching vboxvideo 4.2.6 video
> driver, and it still crashes on start.
> --
> https://code.launchpad.net/~sil2100/compiz/revert_3427_0.9.8/+merge/144764
> Your team Compiz Maintainers is requested to review the proposed merge of
> lp:~sil2100/compiz/revert_3427_0.9.8 into lp:compiz/0.9.8.
>

[Sam Spilsbury (smspillaz)]

Now that I've had a look at the context, this is definitely unsafe for LLVMpipe and other drivers that require that the pixmap be valid on the server side in order to be bound to a texture:

bool
cgl::bindTexImageGLX (ServerGrabInterface *serverGrabInterface,
        Pixmap x11Pixmap,
        GLXPixmap glxPixmap,
        const cgl::PixmapCheckValidityFunc &checkPixmapValidity,
        const cgl::BindTexImageEXTFunc &bindTexImageEXT,
        const cgl::WaitGLXFunc &waitGLX,
        cgl::PixmapSource source)
{
#ifndef LP_1030891_NOT_FIXED
    ServerLock lock (serverGrabInterface);

    waitGLX ();
#endif

    /* External pixmaps can disappear on us, but not
     * while we have a server grab at least */
    if (source == cgl::ExternallyManaged)
    {
#ifdef LP_1030891_NOT_FIXED
 ServerLock lock (serverGrabInterface);
#endif
 if (!checkPixmapValidity (x11Pixmap))
     return false;
    }

    bindTexImageEXT (glxPixmap);

    return true;
}

IF checkPixmapValidity succeeds, and then the scheduler interrupts us between that and the end of bindTexImageEXT, and the application that controls the external pixmap frees and it causes a server sync, then those drivers will crash due to the race condition.

On the other side of the coin, older versions of VirtualBox will hang or crash in the driver when the very first pixmap to be bound is done so while a server grab is active, because the driver is (IMO) broken and does XOpenDisplay using glXBindTexImageEXT.

I've been assured, and have checked in the code that newer versions of VirtualBox do not do this (they call XOpenDisplay in glXCreateContext).

The most appropriate workaround in that case would probably be to manually create, bind, unbind and free a pixmap within the opengl plugin startup code. It would look something like this:

Pixmap p = XCreatePixmap (dpy, root, 1, 1, CopyFromParent);
unsigned int width, height, depth;
Window root;
int x, y;

XGetWindowAttributes (dpy, p, &x, &y, &width, &height, &depth);

GLXPixmap glxP = GL::createPixmap (dpy, p, GLScreen::get (screen)->priv->fbConfigs[depth]);
GLuint tex;
glGenTextures (1, &tex);
glBindTexture (GL_TEXTURE_2D, tex);
GL::bindTexImage (dpy, glxP);
GL::releaseTexImage (dpy, glXP);
GL::destroyPixmap (dpy, glXP);
glDeleteTextures (tex);
XFreePixmap (dpy, p);

That's only off the top of my head - have a look at TfpTexture for exactly how it is done.

review: Needs Fixing

[Francis Ginther (fginther)]

I finally got the 4.2.6 r82870 vbox additions to build on my 3.8.0 kernel. Still no luck getting ppa:unity-team/staging to run on vbox. Still see the same freeze while the desktop is starting. This is with compiz 1:0.9.9~daily13.01.25bzr3592pkg0raring0.

Will keep tinkering.

[Sam Spilsbury (smspillaz)]

I've poked sil2100 with the correct fix for this problem, hopefully
he'll see it tomorrow.

On Sat, Feb 2, 2013 at 11:27 AM, Francis Ginther
<email address hidden> wrote:
> I finally got the 4.2.6 r82870 vbox additions to build on my 3.8.0 kernel. Still no luck getting ppa:unity-team/staging to run on vbox. Still see the same freeze while the desktop is starting. This is with compiz 1:0.9.9~daily13.01.25bzr3592pkg0raring0.
>
> Will keep tinkering.
> --
> https://code.launchpad.net/~sil2100/compiz/revert_3427_0.9.8/+merge/144764
> You are reviewing the proposed merge of lp:~sil2100/compiz/revert_3427_0.9.8 into lp:compiz/0.9.8.

--
Sam Spilsbury

[Łukasz Zemczak (sil2100)]

What do you think? I think that it's the best action we can take to release anything for quantal.

[Timo Jyrinki (timo-jyrinki)]

In case of doubt the only suitable SRU action is no change, and this revert acts as such. There are other important fixes that need to be had, so this issue needs to be revisited later (if ever there is a possibility of backporting a complete fix).

review: Approve

=== modified file 'plugins/opengl/src/glxtfpbind/src/glx-tfp-bind.cpp'
--- plugins/opengl/src/glxtfpbind/src/glx-tfp-bind.cpp	2012-12-11 04:53:02 +0000
+++ plugins/opengl/src/glxtfpbind/src/glx-tfp-bind.cpp	2013-02-25 16:34:19 +0000
@@ -52,12 +52,6 @@
 #endif
 	if (!checkPixmapValidity (x11Pixmap))
 	    return false;
-
-#ifdef LP_1030891_NOT_FIXED
-	// We need to bind before the above ServerLock is lost
-	bindTexImageEXT (glxPixmap);
-	return true;
-#endif
     }
 
     bindTexImageEXT (glxPixmap);