[prev in list] [next in list] [prev in thread] [next in thread] 

List:       freedesktop-xorg-devel
Subject:    Re: [PATCH xserver] glamor: Store the actual EGL/GLX context pointer in lastGLContext
From:       Eric Anholt <eric () anholt ! net>
Date:       2017-05-31 18:00:12
Message-ID: 87inkg3o0j.fsf () eliezer ! anholt ! net
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


Michel Dänzer <michel@daenzer.net> writes:

> From: Michel Dänzer <michel.daenzer@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.

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?

> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>  glamor/glamor_utils.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/glamor/glamor_utils.h b/glamor/glamor_utils.h
> index 6b88527e6..a35917c37 100644
> --- a/glamor/glamor_utils.h
> +++ b/glamor/glamor_utils.h
> @@ -723,8 +723,8 @@ glamor_is_large_pixmap(PixmapPtr pixmap)
>  static inline void
>  glamor_make_current(glamor_screen_private *glamor_priv)
>  {
> -    if (lastGLContext != &glamor_priv->ctx) {
> -        lastGLContext = &glamor_priv->ctx;
> +    if (lastGLContext != glamor_priv->ctx.ctx) {
> +        lastGLContext = glamor_priv->ctx.ctx;
>          glamor_priv->ctx.make_current(&glamor_priv->ctx);
>      }
>  }
> -- 
> 2.11.0
>
> _______________________________________________
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel

["signature.asc" (application/pgp-signature)]
[Attachment #6 (text/plain)]

_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[prev in list] [next in list] [prev in thread] [next in thread]