[Mesa-dev] [PATCH] egl/dri2: dri2_initialize: Do not reference-count TestOnly display

[Nicolas Boichat]

On Thu, Aug 4, 2016 at 12:22 AM, Martin Peres
<martin.peres at linux.intel.com> wrote:
>
>
> On 03/08/16 16:54, Nicolas Boichat wrote:
>>
>> In the case where dri2_initialize is called with a TestOnly display,
>> the display is not actually initialized, so dri2_egl_display always
>> fails, and we cannot do any reference counting.
>>
>> Fixes piglit spec at egl_khr_create_context@verify gl flavor (reproducible
>> with LIBGL_ALWAYS_SOFTWARE=1) and spec at egl_khr_fence_sync@conformance.
>>
>> Fixes: 9ee683f877 (egl/dri2: Add reference count for dri2_egl_display)
>> Cc: "12.0" <mesa-stable at lists.freedesktop.org>
>> Reported-by: Michel Dänzer <michel at daenzer.net>
>> Signed-off-by: Nicolas Boichat <drinkcat at chromium.org>
>> ---
>>
>> Compile-tested only, please give it a spin, thanks!
>
> Still crashes, same backtrace before and after the patch:

Actually, I was thinking about this bug:
https://bugs.freedesktop.org/show_bug.cgi?id=97136, which should be
spec at egl_khr_create_context@verify gl flavor? Did you try that test?

I probably should not have mentionned
spec at egl_khr_fence_sync@conformance in the commit message...

>
> Program received signal SIGSEGV, Segmentation fault.
> 0x00007ffff3b90de9 in wl_proxy_destroy () from
> /usr/lib/libwayland-client.so.0
> (gdb) bt
> #0  0x00007ffff3b90de9 in wl_proxy_destroy () from
> /usr/lib/libwayland-client.so.0
> #1  0x00007ffff679bc4e in wl_registry_destroy (wl_registry=<optimized out>)
> at /usr/include/wayland-client-protocol.h:1047
> #2  dri2_display_release (disp=disp at entry=0x7913f0) at
> drivers/dri2/egl_dri2.c:896
> #3  0x00007ffff679c031 in dri2_terminate (drv=<optimized out>,
> disp=0x7913f0) at drivers/dri2/egl_dri2.c:932
> #4  0x00007ffff6790f7d in eglTerminate (dpy=0x7913f0) at main/eglapi.c:532
> #5  0x00000000004016f5 in init_display (platform=12760,
> out_dpy=0x7fffffffe3a0) at
> /home/mupuf/programmation/piglit/tests/egl/spec/egl_khr_fence_sync/egl_khr_fence_sync.c:136
> #6  0x000000000040285f in init_other_display (out_other_dpy=0x7fffffffe3d8,
> orig_dpy=0x617c50) at
> /home/mupuf/programmation/piglit/tests/egl/spec/egl_khr_fence_sync/egl_khr_fence_sync.c:956
> #7  0x0000000000402917 in test_eglCreateSyncKHR_wrong_display_same_thread
> (test_data=0x0) at
> /home/mupuf/programmation/piglit/tests/egl/spec/egl_khr_fence_sync/egl_khr_fence_sync.c:1006
> #8  0x00007ffff75e10aa in piglit_run_selected_subtests
> (all_subtests=0x404380 <fence_sync_subtests>, selected_subtests=0x0,
> num_selected_subtests=0, previous_result=PIGLIT_SKIP) at
> /home/mupuf/programmation/piglit/tests/util/piglit-util.c:756
> #9  0x000000000040319d in main (argc=1, argv=0x7fffffffe578) at
> /home/mupuf/programmation/piglit/tests/egl/spec/egl_khr_fence_sync/egl_khr_fence_sync.c:1435
>
> If you cannot reproduce, this is something I could be persuaded to look
> into.

Not easy for me to reproduce, but... Looking that the test source code:
https://cgit.freedesktop.org/piglit/tree/tests/egl/spec/egl_khr_fence_sync/egl_khr_fence_sync.c

Do you know why we end up in the error path in init_display?

My guess is that
eglInitialize->dri2_initialize->dri2_initialize_wayland fails after
setting disp->DriverData, so the display refcount is == 0, but the
display is not null, leading to the crash in egl_terminate.

I just spotted this patch for x11:
https://patchwork.freedesktop.org/patch/101934/

platform_wayland needs to be modified in a similar way. Or, better, we
need to refactor dri2_initialize_* functions to avoid touching
_EGLDisplay.

For the record, Emil spotted this issue when I submitted the offending
patch, and I haven't followed up ,-(

Best,

>
>
>>
>>  src/egl/drivers/dri2/egl_dri2.c | 29 +++++++++--------------------
>>  1 file changed, 9 insertions(+), 20 deletions(-)
>>
>> diff --git a/src/egl/drivers/dri2/egl_dri2.c
>> b/src/egl/drivers/dri2/egl_dri2.c
>> index a5cab68..8cdca6a 100644
>> --- a/src/egl/drivers/dri2/egl_dri2.c
>> +++ b/src/egl/drivers/dri2/egl_dri2.c
>> @@ -788,45 +788,34 @@ dri2_initialize(_EGLDriver *drv, _EGLDisplay *disp)
>>     if (disp->Options.UseFallback)
>>        return EGL_FALSE;
>>
>> +   /* Nothing to initialize for a test only display */
>> +   if (disp->Options.TestOnly)
>> +      return EGL_TRUE;
>> +
>>     switch (disp->Platform) {
>>  #ifdef HAVE_SURFACELESS_PLATFORM
>>     case _EGL_PLATFORM_SURFACELESS:
>> -      if (disp->Options.TestOnly)
>> -         ret = EGL_TRUE;
>> -      else
>> -         ret = dri2_initialize_surfaceless(drv, disp);
>> +      ret = dri2_initialize_surfaceless(drv, disp);
>>        break;
>>  #endif
>>  #ifdef HAVE_X11_PLATFORM
>>     case _EGL_PLATFORM_X11:
>> -      if (disp->Options.TestOnly)
>> -         ret = EGL_TRUE;
>> -      else
>> -         ret = dri2_initialize_x11(drv, disp);
>> +      ret = dri2_initialize_x11(drv, disp);
>>        break;
>>  #endif
>>  #ifdef HAVE_DRM_PLATFORM
>>     case _EGL_PLATFORM_DRM:
>> -      if (disp->Options.TestOnly)
>> -         ret = EGL_TRUE;
>> -      else
>> -         ret = dri2_initialize_drm(drv, disp);
>> +      ret = dri2_initialize_drm(drv, disp);
>>        break;
>>  #endif
>>  #ifdef HAVE_WAYLAND_PLATFORM
>>     case _EGL_PLATFORM_WAYLAND:
>> -      if (disp->Options.TestOnly)
>> -         ret = EGL_TRUE;
>> -      else
>> -         ret = dri2_initialize_wayland(drv, disp);
>> +      ret = dri2_initialize_wayland(drv, disp);
>>        break;
>>  #endif
>>  #ifdef HAVE_ANDROID_PLATFORM
>>     case _EGL_PLATFORM_ANDROID:
>> -      if (disp->Options.TestOnly)
>> -         ret = EGL_TRUE;
>> -      else
>> -         ret = dri2_initialize_android(drv, disp);
>> +      ret = dri2_initialize_android(drv, disp);
>>        break;
>>  #endif
>>     default:
>>
>