Re: [Mesa-dev] [PATCH v2 1/2] glx|egl: allow to test if glthread is safe enough on X11 platform

[Emil Velikov]

Hi Gregory,

On 3 May 2017 at 17:34, Gregory Hainaut  wrote:
> I extended the struct __DRIbackgroundCallableExtensionRec because
> the other function pointer is already related for glthread.
>
> DRI2/DRI3 glx code path check that display can be locked (basically
> XInitThread was called)
>
How about something like the following:

Extend __DRIbackgroundCallableExtensionRec to include a callback that
checks for thread safety.
That is required because ... XInitThread... with either GLX or EGL.


> EGL code path is more tricky as we don't want to pull X11 header. Instead
> the code will assume that it is safe if X11 isn't used or there is no display
> (i.e. 100% XCB)
>
See my comment below - this assumption seems to be off.

> The new function will be used in the next commit
>
> v2: based on Nicolai and Matt feedback
> Use C style comment
> Bump __DRIbackgroundCallableExtension version
>
> Signed-off-by: Gregory Hainaut 
> ---
>  include/GL/internal/dri_interface.h | 10 ++
>  src/egl/drivers/dri2/egl_dri2.c | 34 +-
>  src/glx/dri2_glx.c  | 11 ++-
>  src/glx/dri3_glx.c  | 10 +-
>  4 files changed, 62 insertions(+), 3 deletions(-)
>
Can you make this three patches:
 - interface - should answer the why we need it, bugzilla entries/ML
discussions are encouraged
 - egl implementation
 - glx implementation


> diff --git a/include/GL/internal/dri_interface.h 
> b/include/GL/internal/dri_interface.h
> index 86efd1bdc9..ef897b6483 100644
> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -1713,13 +1713,23 @@ struct __DRIbackgroundCallableExtensionRec {
>  * non-background thread (i.e. a thread that has already been bound to a
>  * context using __DRIcoreExtensionRec::bindContext()); when this happens,
>  * the \c loaderPrivate pointer must be equal to the pointer that was
>  * passed to the driver when the currently bound context was created.
>  *
>  * This call should execute quickly enough that the driver can call it 
> with
>  * impunity whenever a background thread starts performing drawing
>  * operations (e.g. it should just set a thread-local variable).
>  */
> void (*setBackgroundContext)(void *loaderPrivate);
> +
> +   /**
> +* Indicate that it is multithread safe to use glthread.

> Typically
> +* XInitThread was called in GLX setup.
For GLX/EGL platforms using Xlib, that involves calling XInitThread, before XXX.

> +*
> +* \param loaderPrivate is the value that was passed to to the driver when
> +* the context was created.  This can be used by the loader to identify
> +* which context any callbacks are associated with.
> +*/
> +   GLboolean (*isGlThreadSafe)(void *loaderPrivate);
Nit: Maybe call this threadSafe or isThreadSafe?

>  };
>
>  #endif
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index 91456b025d..07cafee468 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -85,24 +85,56 @@
>
>  static void
>  dri_set_background_context(void *loaderPrivate)
>  {
> _EGLContext *ctx = _eglGetCurrentContext();
> _EGLThreadInfo *t = _eglGetCurrentThread();
>
> _eglBindContextToThread(ctx, t);
>  }
>
> +static GLboolean
> +dri_is_glthread_safe(void *loaderPrivate)
> +{
> +#ifdef HAVE_X11_PLATFORM
> +   struct dri2_egl_surface *dri2_surf = loaderPrivate;
> +   _EGLDisplay *display =  dri2_surf->base.Resource.Display;
> +   Display *dpy;
> +
> +   /* Only the libX11 isn't safe */
> +   if (display->Platform != _EGL_PLATFORM_X11)
> +  return true;
> +
> +   /* Will use pure XCB so no libX11 here either */
> +   if (display->PlatformDisplay == NULL)
> +  return true;
> +
> +   /**
> +* In an ideal world we would check the X11 lock pointer
> +* (display->PlatformDisplay->lock_fns). Unfortunately it
> +* requires to know the full type. And we don't want to bring X11
> +* headers here.
> +*
Not sure what is the problem here? You should be able to include the
header at the top of the file within a ifdef HAVE_X11_PLATFORM guard.

Writing the lot like the following should be a bit easier on the eyes:

#ifdef HAVE_X11_PLATFORM
   /* comment */
   if (display->Platform == _EGL_PLATFORM_X11 &&
!display->PlatformDisplay && !dpy->lock_fns)
  return false;
#endif
   return true;

> --- a/src/glx/dri2_glx.c
> +++ b/src/glx/dri2_glx.c
> @@ -946,20 +946,28 @@ dri2GetSwapInterval(__GLXDRIdrawable *pdraw)
>return priv->swap_interval;
>  }
>
>  static void
>  driSetBackgroundContext(void *loaderPrivate)
>  {
> struct dri2_context *pcp = (struct dri2_context *) loaderPrivate;
> __glXSetCurrentContext(>base);
>  }
>
> +static GLboolean
> +driIsGlThreadSafe(void *loaderPrivate)
> +{
> +   struct dri2_context *pcp = (struct dri2_context *) loaderPrivate;
> + 

[Mesa-dev] [PATCH v2 1/2] glx|egl: allow to test if glthread is safe enough on X11 platform

[Gregory Hainaut]

I extended the struct __DRIbackgroundCallableExtensionRec because
the other function pointer is already related for glthread.

DRI2/DRI3 glx code path check that display can be locked (basically
XInitThread was called)

EGL code path is more tricky as we don't want to pull X11 header. Instead
the code will assume that it is safe if X11 isn't used or there is no display
(i.e. 100% XCB)

The new function will be used in the next commit

v2: based on Nicolai and Matt feedback
Use C style comment
Bump __DRIbackgroundCallableExtension version

Signed-off-by: Gregory Hainaut 
---
 include/GL/internal/dri_interface.h | 10 ++
 src/egl/drivers/dri2/egl_dri2.c | 34 +-
 src/glx/dri2_glx.c  | 11 ++-
 src/glx/dri3_glx.c  | 10 +-
 4 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/include/GL/internal/dri_interface.h 
b/include/GL/internal/dri_interface.h
index 86efd1bdc9..ef897b6483 100644
--- a/include/GL/internal/dri_interface.h
+++ b/include/GL/internal/dri_interface.h
@@ -1713,13 +1713,23 @@ struct __DRIbackgroundCallableExtensionRec {
 * non-background thread (i.e. a thread that has already been bound to a
 * context using __DRIcoreExtensionRec::bindContext()); when this happens,
 * the \c loaderPrivate pointer must be equal to the pointer that was
 * passed to the driver when the currently bound context was created.
 *
 * This call should execute quickly enough that the driver can call it with
 * impunity whenever a background thread starts performing drawing
 * operations (e.g. it should just set a thread-local variable).
 */
void (*setBackgroundContext)(void *loaderPrivate);
+
+   /**
+* Indicate that it is multithread safe to use glthread. Typically
+* XInitThread was called in GLX setup.
+*
+* \param loaderPrivate is the value that was passed to to the driver when
+* the context was created.  This can be used by the loader to identify
+* which context any callbacks are associated with.
+*/
+   GLboolean (*isGlThreadSafe)(void *loaderPrivate);
 };
 
 #endif
diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index 91456b025d..07cafee468 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -85,24 +85,56 @@
 
 static void
 dri_set_background_context(void *loaderPrivate)
 {
_EGLContext *ctx = _eglGetCurrentContext();
_EGLThreadInfo *t = _eglGetCurrentThread();
 
_eglBindContextToThread(ctx, t);
 }
 
+static GLboolean
+dri_is_glthread_safe(void *loaderPrivate)
+{
+#ifdef HAVE_X11_PLATFORM
+   struct dri2_egl_surface *dri2_surf = loaderPrivate;
+   _EGLDisplay *display =  dri2_surf->base.Resource.Display;
+   Display *dpy;
+
+   /* Only the libX11 isn't safe */
+   if (display->Platform != _EGL_PLATFORM_X11)
+  return true;
+
+   /* Will use pure XCB so no libX11 here either */
+   if (display->PlatformDisplay == NULL)
+  return true;
+
+   /**
+* In an ideal world we would check the X11 lock pointer
+* (display->PlatformDisplay->lock_fns). Unfortunately it
+* requires to know the full type. And we don't want to bring X11
+* headers here.
+*
+* So let's assume an unsafe behavior. Modern EGL code shouldn't use
+* libX11 anyway.
+*/
+   return false;
+#else
+   return true;
+#endif
+}
+
 const __DRIbackgroundCallableExtension background_callable_extension = {
-   .base = { __DRI_BACKGROUND_CALLABLE, 1 },
+   .base = { __DRI_BACKGROUND_CALLABLE, 2 },
 
.setBackgroundContext = dri_set_background_context,
+   .isGlThreadSafe   = dri_is_glthread_safe,
 };
 
 const __DRIuseInvalidateExtension use_invalidate = {
.base = { __DRI_USE_INVALIDATE, 1 }
 };
 
 static const EGLint dri2_to_egl_attribute_map[] = {
[__DRI_ATTRIB_BUFFER_SIZE ]  = EGL_BUFFER_SIZE,
[__DRI_ATTRIB_LEVEL] = EGL_LEVEL,
[__DRI_ATTRIB_RED_SIZE]  = EGL_RED_SIZE,
diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
index 145f44d6e8..7a6511e2de 100644
--- a/src/glx/dri2_glx.c
+++ b/src/glx/dri2_glx.c
@@ -946,20 +946,28 @@ dri2GetSwapInterval(__GLXDRIdrawable *pdraw)
   return priv->swap_interval;
 }
 
 static void
 driSetBackgroundContext(void *loaderPrivate)
 {
struct dri2_context *pcp = (struct dri2_context *) loaderPrivate;
__glXSetCurrentContext(>base);
 }
 
+static GLboolean
+driIsGlThreadSafe(void *loaderPrivate)
+{
+   struct dri2_context *pcp = (struct dri2_context *) loaderPrivate;
+   return pcp->base.psc->dpy->lock_fns != NULL;
+}
+
+
 static const __DRIdri2LoaderExtension dri2LoaderExtension = {
.base = { __DRI_DRI2_LOADER, 3 },
 
.getBuffers  = dri2GetBuffers,
.flushFrontBuffer= dri2FlushFrontBuffer,
.getBuffersWithFormat= dri2GetBuffersWithFormat,
 };
 
 static const __DRIdri2LoaderExtension dri2LoaderExtension_old = {
.base = {