On 20 July 2016 at 15:42, Emil Velikov <emil.l.veli...@gmail.com> wrote: > 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. > A bit more meat to it: Upper layer(s) will ensure that upon second call to eglInitialize (without a eglTerminate in between) we won't get here. Thus only case we get this is on user misuse/leak - missing explicit/implicit eglMakeCurrent(...NULL, NULL) call while having called eglTerminate.
If we ref count we exacerbate the leak. At the same time, returning error in case of a user leak sounds silly, so dri2_display_release might be like the better option ? -Emil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
