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

2017-05-03 Thread Nicolai Hähnle 

On 28.04.2017 23:11, 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)

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

Signed-off-by: Gregory Hainaut 
---
 include/GL/internal/dri_interface.h |  9 +
 src/egl/drivers/dri2/egl_dri2.c | 30 ++
 src/glx/dri2_glx.c  |  9 +
 src/glx/dri3_glx.c  |  8 
 4 files changed, 56 insertions(+)

diff --git a/include/GL/internal/dri_interface.h 
b/include/GL/internal/dri_interface.h
index 86efd1bdc9..28a52ccdb9 100644
--- a/include/GL/internal/dri_interface.h
+++ b/include/GL/internal/dri_interface.h
@@ -1713,13 +1713,22 @@ 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);


Add a space here.



+   /**
+* 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 2cab7d00c1..df2db97bcf 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -85,24 +85,54 @@

 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.


Change the comment style.



+   return false;
+#else
+   return true;
+#endif
+}
+
 const __DRIbackgroundCallableExtension background_callable_extension = {
.base = { __DRI_BACKGROUND_CALLABLE, 1 },

.setBackgroundContext = dri_set_background_context,
+   .isGlThreadSafe   = dri_is_glthread_safe,
 };


You're adding a function pointer, so bump the extension version.




 const __DRIuseInvalidateExtension use_invalidate = {
.base = { __DRI_USE_INVALIDATE, 1 }
 };

 EGLint dri2_to_egl_attribute_map[] = {
0,
EGL_BUFFER_SIZE,/* __DRI_ATTRIB_BUFFER_SIZE */
EGL_LEVEL,/* __DRI_ATTRIB_LEVEL */
diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
index 145f44d6e8..8f4d2f027f 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 = { __DRI_DRI2_LOADER, 3 },
@@ -970,20 +978,21 @@ static const __DRIdri2LoaderExtension

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

2017-04-28 Thread Matt Turner 
On Fri, Apr 28, 2017 at 2:11 PM, 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)
>
> 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
>
> Signed-off-by: Gregory Hainaut 
> ---
>  include/GL/internal/dri_interface.h |  9 +
>  src/egl/drivers/dri2/egl_dri2.c | 30 ++
>  src/glx/dri2_glx.c  |  9 +
>  src/glx/dri3_glx.c  |  8 
>  4 files changed, 56 insertions(+)
>
> diff --git a/include/GL/internal/dri_interface.h 
> b/include/GL/internal/dri_interface.h
> index 86efd1bdc9..28a52ccdb9 100644
> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -1713,13 +1713,22 @@ 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 2cab7d00c1..df2db97bcf 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -85,24 +85,54 @@
>
>  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.

Don't use C++ comments.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2017-04-28 Thread 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

Signed-off-by: Gregory Hainaut 
---
 include/GL/internal/dri_interface.h |  9 +
 src/egl/drivers/dri2/egl_dri2.c | 30 ++
 src/glx/dri2_glx.c  |  9 +
 src/glx/dri3_glx.c  |  8 
 4 files changed, 56 insertions(+)

diff --git a/include/GL/internal/dri_interface.h 
b/include/GL/internal/dri_interface.h
index 86efd1bdc9..28a52ccdb9 100644
--- a/include/GL/internal/dri_interface.h
+++ b/include/GL/internal/dri_interface.h
@@ -1713,13 +1713,22 @@ 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 2cab7d00c1..df2db97bcf 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -85,24 +85,54 @@
 
 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 },
 
.setBackgroundContext = dri_set_background_context,
+   .isGlThreadSafe   = dri_is_glthread_safe,
 };
 
 const __DRIuseInvalidateExtension use_invalidate = {
.base = { __DRI_USE_INVALIDATE, 1 }
 };
 
 EGLint dri2_to_egl_attribute_map[] = {
0,
EGL_BUFFER_SIZE,/* __DRI_ATTRIB_BUFFER_SIZE */
EGL_LEVEL,/* __DRI_ATTRIB_LEVEL */
diff --git a/src/glx/dri2_glx.c b/src/glx/dri2_glx.c
index 145f44d6e8..8f4d2f027f 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 = { __DRI_DRI2_LOADER, 3 },
@@ -970,20 +978,21 @@ static const __DRIdri2LoaderExtension 
dri2LoaderExtension_old = {
 };
 
 static const __DRIuseInvalidateExtension dri2UseInvalidate = {
.base = { __DRI_USE_INVALIDATE, 1 }
 };
 
 static const