Re: [Mesa-dev] [PATCH v2] egl/dri2: Add reference count for dri2_egl_display

2016-08-08 Thread
Hello

https://patchwork.freedesktop.org/series/10079/ has been merged to
master but it is still marked as "new" at patchwork.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH v2] egl/dri2: Add reference count for dri2_egl_display

2016-07-28 Thread Emil Velikov
On 22 July 2016 at 04:27, Nicolas Boichat  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.
>
> 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.
>
> Signed-off-by: Nicolas Boichat 
> Reviewed-by: Eric Engestrom 
>
r-b and pushed to master.

Thanks again for the great work in resolving this !
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


[Mesa-dev] [PATCH v2] egl/dri2: Add reference count for dri2_egl_display

2016-07-21 Thread Nicolas Boichat
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.

Signed-off-by: Nicolas Boichat 
Reviewed-by: Eric Engestrom 

---
 src/egl/drivers/dri2/egl_dri2.c | 111 
 src/egl/drivers/dri2/egl_dri2.h |   4 ++
 2 files changed, 95 insertions(+), 20 deletions(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index ac2be86..114cf4e 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -757,10 +757,33 @@ 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;
@@ -769,52 +792,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++;
+   }
+
+   return ret;
 }
 
 /**
- * Called via eglTerminate(), drv->API.