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

[Nicolas Boichat]

Hi Emil,

On Tue, Aug 16, 2016 at 1:17 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> Hi Nicolas,
>
> On 4 August 2016 at 02:51, Nicolas Boichat <drinkcat at chromium.org> wrote:
>> On Thu, Aug 4, 2016 at 9:38 AM, Michel Dänzer <michel at daenzer.net> wrote:
>>> On 04.08.2016 09:53, Nicolas Boichat wrote:
>>>> 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?
>>>
>>> Your patch fixes this test for me.
>>>
>>> Tested-by: Michel Dänzer <michel.daenzer at amd.com>
>>>
>>> Please remove the reference to the egl_khr_fence_sync test from the
>>> commit log.
>>
>> Thanks.
>>
>> Emil: Can you fixup the commit message before applying?
>>
> Sure I can do that. Yet this change fixes another
> unexpected/undocumented bug

I don't think there is anything difference _before_ 9ee683f877
(egl/dri2: Add reference count for dri2_egl_display), and after this
patch.

After 9ee683f877, of course, there is a bug, as eglGetProcAddress just
returns NULL when called before initializing any display, which is
clearly wrong.

> - currently calling eglGetProcAddress will
> fail, when libEGL is built without the said platform.
> Yet the API/documentation is clear - entry points are independent of
> display (which is platform specific) or context.
>
> Did I miss something, does the above make sense ?

Well, spec says "A return value of NULL indicates that the specific
function does not exist for the EGL implementation.", so I guess it's
fine to return NULL is EGL is built without a given platform.

One possible issue is if the "default" display (TestOnly), is
different from the display being used later... But I don't see any
problem here, if I trace the code correctly:
eglGetProcAddress:
  => For many functions (egl*), returns a function pointer that is
always valid => No problem
  => For others (I guess mostly gl*): calls _eglGetDriverProc

_eglGetDriverProc:
- Creates a default display (which initializes _eglModules)
- Iterates over the modules, and calls
mod->Driver->API.GetProcAddress. I think this can only be
dri2_get_proc_address.

dri2_get_proc_address:
 - Calls dri2_drv->get_proc_address (that's dlsym(handle,
"_glapi_get_proc_address");, see dri2_load)

_glapi_get_proc_address:
/**
 * Return pointer to the named function.  If the function name isn't found
 * in the name of static functions, try generating a new API entrypoint on
 * the fly with assembly language.
 */
_glapi_proc
_glapi_get_proc_address(const char *funcName)
{
   const struct mapi_stub *stub = _glapi_get_stub(funcName, 1);
   return (stub) ? (_glapi_proc) stub_get_addr(stub) : NULL;
}

Which looks platform/display independent? Or am I missing something?

>>>> 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.
>>>
>>> Indeed, that fixes the egl_khr_fence_sync test for me FWIW.
>>
>> I'll send a series to fix that very soon, on all platforms.
>>
>>>> For the record, Emil spotted this issue when I submitted the offending
>>>> patch, and I haven't followed up ,-(
>>>
>>> For future patches, please make sure there are no piglit regressions, at
>>> least for the tests which run with swrast via LIBGL_ALWAYS_SOFTWARE=1.
>>
>> Is there some simple instructions to do that?
> Running piglit should be straight forward - please check the README
> (and tell is it somethings is odd).

Well, I didn't find ready-to-use instructions for lazy people like me
,-) In any case, I pasted my commands in another thread, they look
like this:

export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$MESA_DIR/lib
export LIBGL_DRIVERS_PATH=$MESA_DIR/lib/gallium
export EGL_DRIVERS_PATH=$MESA_DIR/lib
export EGL_LOG_LEVEL=debug
export LIBGL_ALWAYS_SOFTWARE=1
./piglit run -p x11_egl -t "spec at egl.*" all results/master