From e82567c02b3796c48c013327dc0a34711b0d3724 Mon Sep 17 00:00:00 2001 From: Nicolas Boichat Date: Fri, 22 Jul 2016 11:27:41 +0800 Subject: [PATCH] egl/dri2: Add reference count for dri2_egl_display MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit android.opengl.cts.WrapperTest#testGetIntegerv1 CTS test calls eglTerminate, followed by eglReleaseThread. A similar case is observed in this bug: https://bugs.freedesktop.org/show_bug.cgi?id=69622, where the test calls eglTerminate, then eglMakeCurrent(dpy, NULL, NULL, NULL). With the current code, dri2_dpy structure is freed on eglTerminate call, so the display is not initialized when eglReleaseThread calls MakeCurrent with NULL parameters, to unbind the context, which causes a a segfault in drv->API.MakeCurrent (dri2_make_current), either in glFlush or in a latter call. eglTerminate specifies that "If contexts or surfaces associated with display is current to any thread, they are not released until they are no longer current as a result of eglMakeCurrent." However, to properly free the current context/surface (i.e., call glFlush, unbindContext, driDestroyContext), we still need the display vtbl (and possibly an active dri dpy connection). Therefore, we add some reference counter to dri2_egl_display, to make sure the structure is kept allocated as long as it is required. One drawback of this is that eglInitialize may not completely reinitialize the display (if eglTerminate was called with a current context), however, this seems to meet the EGL spec quite well, and does not permanently leak any context/display even for incorrectly written apps. Cc: "12.0" Signed-off-by: Nicolas Boichat Reviewed-by: Eric Engestrom Reviewed-by: Emil Velikov (cherry picked from commit 9ee683f877b283020c6f24776236f1145cb7a4ea) Squashed with commit egl/dri2: dri2_make_current: Release previous context's display eglMakeCurrent can also be used to change the active display. In that case, we need to decrement ref_count of the previous display (possibly destroying it), and increment it on the next display. Also, old_dsurf/old_rsurf cannot be non-NULL if old_ctx is NULL, so we only need to test if old_ctx is non-NULL. v2: Save the old display before destroying the context. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97214 Fixes: 9ee683f877 (egl/dri2: Add reference count for dri2_egl_display) Cc: "12.0" Reported-by: Alexandr Zelinsky Tested-by: Alexandr Zelinsky Reviewed-and-Tested-by: Michel Dänzer Signed-off-by: Nicolas Boichat (cherry picked from commit 78e3cea4197802253401766fc44362786898e024) Squashed with commit egl/dri2: dri2_initialize: Do not reference-count TestOnly display In the case where dri2_initialize is called with a TestOnly display, the display is not actually initialized, so dri2_egl_display always fails, and we cannot do any reference counting. Fixes piglit spec@egl_khr_create_context@verify gl flavor (reproducible with LIBGL_ALWAYS_SOFTWARE=1). Fixes: 9ee683f877 (egl/dri2: Add reference count for dri2_egl_display) Cc: "12.0" Reported-by: Michel Dänzer Signed-off-by: Nicolas Boichat Reviewed-by: Emil Velikov (cherry picked from commit 4f3f8bb59dd98e39c363fe47a55a7f97e7df9f4b) --- src/egl/drivers/dri2/egl_dri2.c | 113 +++++++++++++++++++++++++++++++--------- src/egl/drivers/dri2/egl_dri2.h | 4 ++ 2 files changed, 91 insertions(+), 26 deletions(-) diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c index 2e97d85c6a1..1659af6e016 100644 --- a/src/egl/drivers/dri2/egl_dri2.c +++ b/src/egl/drivers/dri2/egl_dri2.c @@ -757,64 +757,99 @@ dri2_create_screen(_EGLDisplay *disp) /** * Called via eglInitialize(), GLX_drv->API.Initialize(). + * + * This must be guaranteed to be called exactly once, even if eglInitialize is + * called many times (without a eglTerminate in between). */ static EGLBoolean dri2_initialize(_EGLDriver *drv, _EGLDisplay *disp) { + EGLBoolean ret = EGL_FALSE; + struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp); + + /* In the case where the application calls eglMakeCurrent(context1), + * eglTerminate, then eglInitialize again (without a call to eglReleaseThread + * or eglMakeCurrent(NULL) before that), dri2_dpy structure is still + * initialized, as we need it to be able to free context1 correctly. + * + * It would probably be safest to forcibly release the display with + * dri2_display_release, to make sure the display is reinitialized correctly. + * However, the EGL spec states that we need to keep a reference to the + * current context (so we cannot call dri2_make_current(NULL)), and therefore + * we would leak context1 as we would be missing the old display connection + * to free it up correctly. + */ + if (dri2_dpy) { + dri2_dpy->ref_count++; + return EGL_TRUE; + } + /* not until swrast_dri is supported */ if (disp->Options.UseFallback) return EGL_FALSE; + /* Nothing to initialize for a test only display */ + if (disp->Options.TestOnly) + return EGL_TRUE; + switch (disp->Platform) { #ifdef HAVE_SURFACELESS_PLATFORM case _EGL_PLATFORM_SURFACELESS: - if (disp->Options.TestOnly) - return EGL_TRUE; - return dri2_initialize_surfaceless(drv, disp); + ret = dri2_initialize_surfaceless(drv, disp); + break; #endif - #ifdef HAVE_X11_PLATFORM case _EGL_PLATFORM_X11: - if (disp->Options.TestOnly) - return EGL_TRUE; - return dri2_initialize_x11(drv, disp); + ret = dri2_initialize_x11(drv, disp); + break; #endif - #ifdef HAVE_DRM_PLATFORM case _EGL_PLATFORM_DRM: - if (disp->Options.TestOnly) - return EGL_TRUE; - return dri2_initialize_drm(drv, disp); + ret = dri2_initialize_drm(drv, disp); + break; #endif #ifdef HAVE_WAYLAND_PLATFORM case _EGL_PLATFORM_WAYLAND: - if (disp->Options.TestOnly) - return EGL_TRUE; - return dri2_initialize_wayland(drv, disp); + ret = dri2_initialize_wayland(drv, disp); + break; #endif #ifdef HAVE_ANDROID_PLATFORM case _EGL_PLATFORM_ANDROID: - if (disp->Options.TestOnly) - return EGL_TRUE; - return dri2_initialize_android(drv, disp); + ret = dri2_initialize_android(drv, disp); + break; #endif - default: _eglLog(_EGL_WARNING, "No EGL platform enabled."); return EGL_FALSE; } + + if (ret) { + dri2_dpy = dri2_egl_display(disp); + + if (!dri2_dpy) { + return EGL_FALSE; + } + + dri2_dpy->ref_count++; + } + + return ret; } /** - * Called via eglTerminate(), drv->API.Terminate(). + * Decrement display reference count, and free up display if necessary. */ -static EGLBoolean -dri2_terminate(_EGLDriver *drv, _EGLDisplay *disp) -{ +static void +dri2_display_release(_EGLDisplay *disp) { struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp); unsigned i; - _eglReleaseDisplayResources(drv, disp); + assert(dri2_dpy->ref_count > 0); + dri2_dpy->ref_count--; + + if (dri2_dpy->ref_count > 0) + return; + _eglCleanupDisplay(disp); if (dri2_dpy->own_dri_screen) @@ -869,6 +904,21 @@ dri2_terminate(_EGLDriver *drv, _EGLDisplay *disp) } free(dri2_dpy); disp->DriverData = NULL; +} + +/** + * Called via eglTerminate(), drv->API.Terminate(). + * + * This must be guaranteed to be called exactly once, even if eglTerminate is + * called many times (without a eglInitialize in between). + */ +static EGLBoolean +dri2_terminate(_EGLDriver *drv, _EGLDisplay *disp) +{ + /* Release all non-current Context/Surfaces. */ + _eglReleaseDisplayResources(drv, disp); + + dri2_display_release(disp); return EGL_TRUE; } @@ -1188,6 +1238,10 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *dsurf, _EGLSurface *tmp_dsurf, *tmp_rsurf; __DRIdrawable *ddraw, *rdraw; __DRIcontext *cctx; + EGLBoolean unbind; + + if (!dri2_dpy) + return _eglError(EGL_NOT_INITIALIZED, "eglMakeCurrent"); /* make new bindings */ if (!_eglBindContext(ctx, dsurf, rsurf, &old_ctx, &old_dsurf, &old_rsurf)) { @@ -1208,14 +1262,21 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *dsurf, dri2_dpy->core->unbindContext(old_cctx); } - if ((cctx == NULL && ddraw == NULL && rdraw == NULL) || - dri2_dpy->core->bindContext(cctx, ddraw, rdraw)) { + unbind = (cctx == NULL && ddraw == NULL && rdraw == NULL); + + if (unbind || dri2_dpy->core->bindContext(cctx, ddraw, rdraw)) { if (old_dsurf) drv->API.DestroySurface(drv, disp, old_dsurf); if (old_rsurf) drv->API.DestroySurface(drv, disp, old_rsurf); - if (old_ctx) + + if (!unbind) + dri2_dpy->ref_count++; + if (old_ctx) { + EGLDisplay old_disp = _eglGetDisplayHandle(old_ctx->Resource.Display); drv->API.DestroyContext(drv, disp, old_ctx); + dri2_display_release(old_disp); + } return EGL_TRUE; } else { diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h index 925294b27d0..02f960475b1 100644 --- a/src/egl/drivers/dri2/egl_dri2.h +++ b/src/egl/drivers/dri2/egl_dri2.h @@ -177,6 +177,10 @@ struct dri2_egl_display const __DRI2interopExtension *interop; int fd; + /* dri2_initialize/dri2_terminate increment/decrement this count, so does + * dri2_make_current (tracks if there are active contexts/surfaces). */ + int ref_count; + int own_device; int swap_available; int invalidate_available; -- 2.11.0