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

[Emil Velikov]

On 20 July 2016 at 09:26, Nicolas Boichat <drinkcat at 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 <drinkcat at 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