[PATCH xserver] glamor: Store the actual EGL/GLX context pointer in lastGLContext
eric at anholt.net
Thu Jun 1 18:36:38 UTC 2017
Michel Dänzer <michel at daenzer.net> writes:
> [ Unknown signature status ]
> On 01/06/17 03:00 AM, Eric Anholt wrote:
>> Michel Dänzer <michel at daenzer.net> writes:
>>> From: Michel Dänzer <michel.daenzer at amd.com>
>>> Fixes subtle breakage which could sometimes trigger after a server reset
>>> with multiple screens using glamor:
>>> Screen A enters glamor_close_screen last and calls various cleanup
>>> functions, which at some point call glamor_make_current to make sure
>>> screen x's GL context is current. This sets lastGLContext to screen A's
>>> &glamor_priv->ctx. Finally, glamor_close_screen calls
>>> glamor_release_screen_priv, which calls free(glamor_priv).
>>> Later, screen B enters glamor_init, which allocates a new glamor_priv.
>>> With bad luck, this can return the same pointer which was previously
>>> used for screen A's glamor_priv. So when screen B's glamor_init calls
>>> glamor_make_current, lastGLContext == &glamor_priv->ctx, so MakeCurrent
>>> isn't called for screen B's GL context, and the following OpenGL API
>>> calls triggered by glamor_init mess up screen A's GL context.
>>> The observeed end result of this was a crash in glamor_get_vbo_space
>>> because glamor_priv->vbo didn't match the GL context, though there might
>>> be other possible outcomes.
>>> Assigning the actual GL context pointer to lastGLContext prevents this
>>> by preventing the false negative test in glamor_make_current.
>> Thanks for debugging -- this sounds like a pain.
> Indeed, it was. :}
>> After this patch, don't we have a similar problem where we've freed the
>> old GL context, and the new GL context gets allocated at the old
>> address? Shouldn't we just null out lastGLContext once we've destroyed
>> our old context in closescreen?
> The GL context isn't destroyed in CloseScreen, only in FreeScreen, which
> is only called when the server shuts down (if at all).
> Setting lastGLContext = NULL from CloseScreen would also prevent the
> problem; I considered doing that instead of or in addition to the fix in
> glamor_make_current, but finally decided against it because it would be
> more of a workaround than a fix for the actual problem (and could
> unnecessarily force a MakeCurrent call after a server reset in some cases).
I see now. r-b.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 832 bytes
Desc: not available
More information about the xorg-devel