[PATCH xserver] glamor: Store the actual EGL/GLX context pointer in lastGLContext

[Eric Anholt]

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...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.x.org/archives/xorg-devel/attachments/20170601/fed8e5bc/attachment.sig>