[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 <gregory.hainaut at gmail.com> 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 <gregory.hainaut at gmail.com>
> ---
>  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(&pcp->base);
>  }
>
> +static GLboolean
> +driIsGlThreadSafe(void *loaderPrivate)
> +{
> +   struct dri2_context *pcp = (struct dri2_context *) loaderPrivate;
> +   return pcp->base.psc->dpy->lock_fns != NULL;
Do add two-line comment what/why we do here.

> +}
> +
> +
Nit: just one blank line.


> +static GLboolean
> +dri_is_glthread_safe(void *loaderPrivate)
> +{
> +   struct dri3_context *pcp = (struct dri3_context *) loaderPrivate;
> +   return pcp->base.psc->dpy->lock_fns != NULL;
Comment please.

Thanks
Emil