Hi Gregory,
On 3 May 2017 at 17:34, Gregory Hainaut <gregory.hain...@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.hain...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
