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

[Emil Velikov]

On 20 July 2016 at 15:42, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> 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.
>
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