On 20 July 2016 at 09:26, Nicolas Boichat <drink...@chromium.org> wrote: > 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. > Looks very, just a couple of suggestions below.
> Signed-off-by: Nicolas Boichat <drink...@chromium.org> > --- > > Replaces https://patchwork.freedesktop.org/patch/98874/. > > src/egl/drivers/dri2/egl_dri2.c | 96 > ++++++++++++++++++++++++++++++++--------- > src/egl/drivers/dri2/egl_dri2.h | 4 ++ > 2 files changed, 80 insertions(+), 20 deletions(-) > > diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c > index ac2be86..00269d3 100644 > --- a/src/egl/drivers/dri2/egl_dri2.c > +++ b/src/egl/drivers/dri2/egl_dri2.c > @@ -761,6 +761,14 @@ dri2_create_screen(_EGLDisplay *disp) > static EGLBoolean > dri2_initialize(_EGLDriver *drv, _EGLDisplay *disp) > { > + EGLBoolean ret = EGL_FALSE; > + struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp); > + > + if (dri2_dpy) { > + dri2_dpy->ref_count++; > + return EGL_TRUE; > + } > + I'm not sure that reusing the dpy is what we want here. IMHO we should either call dri2_display_release (to release existing resources) or simply error out. > /* not until swrast_dri is supported */ > if (disp->Options.UseFallback) > return EGL_FALSE; > @@ -769,52 +777,75 @@ dri2_initialize(_EGLDriver *drv, _EGLDisplay *disp) > #ifdef HAVE_SURFACELESS_PLATFORM > case _EGL_PLATFORM_SURFACELESS: > if (disp->Options.TestOnly) > - return EGL_TRUE; > - return dri2_initialize_surfaceless(drv, disp); > + ret = EGL_TRUE; > + else > + 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 = EGL_TRUE; > + else > + 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 = EGL_TRUE; > + else > + 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 = EGL_TRUE; > + else > + 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 = EGL_TRUE; > + else > + 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++; > + } > + This should really be part of a dri2_initialize_screen or similar helper and we ought to fix our dri2_initialize_$platform functions to leave the base struct _EGLdisplay unchanged upon error. Atm, the latter can return a non-null dri2_dpy, thus above (new code) will crash. ^^ Just an idea for follow-up patch(es). Thanks Emil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev