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

[Nicolas Boichat]

On Tue, Aug 23, 2016 at 9:26 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 22 August 2016 at 07:10, Nicolas Boichat <drinkcat at chromium.org> wrote:
>> 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.
>>
> Yes, that's correct. Yet I was thinking about the case explained below.
>
>>> - 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?
>>
> The gist is that _eglGetNativePlatform() can return a platform which
> isn't present/build in the libEGL implementation. See below for
> detailed before/after:
>
> With your patch:
>  - we dive into _eglMatchDriver calling dri2_initialize (via
> _eglMatchAndInitialize) with TestOnly == TRUE regardless of the
> platform requested
>  - thus we reach API.GetProcAddress/dri2_get_proc_address and provide
> a non NULL entry point function pointer.
>
> Before this patch (or any of your patches):
>  - in _eglGetDriverProc we use eglGetDisplay(EGL_DEFAULT_DISPLAY)
>  - the latter can return a dpy for a platform that is _not_ built into
> the libEGL implementation (thanks to _eglGetNativePlatform)
>  - thus by reusing the detected platform in dri2_initialize() we get
> false and we'll never get to the API.GetProcAddress call in
> _eglGetDriverProc.
>  - from a user POV they'll get NULL for eglGetProcAddress because we
> (sort of) miss-detect the EGL_DEFAULT_DISPLAY even if they are going
> to pick/use _any_ working platform via eglGetPlatformDisplay.
>
> As the above indicates it's quite a corner case, which ... should not
> be possible to hit with the patch.
> All the said, I'll double-check (build and run wise) that things don't
> explode and push this.

Ok, thanks, now I understand. To revert to the old behaviour this,
we'd have to copy paste
   if (disp->Options.TestOnly)
      return EGL_TRUE;
in each of the switch cases.

But, as you say, the new behaviour is probably more correct, it's just
that I did not think it would fix that issue ,-)

>>>>>> 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
>
> Yes that's perfect. Most of us know them by heart since they're but
> mesa (not piglit) specific and you'll not need them if you make
> install.

Yeah, I guess installing to /usr/local is simplest, but a bit more
risky on a development machine ,-)

piglit was also a somewhat confusing: I wasn't sure which platform to
pick, for example. I also found the valgrind option very useful and
worth using, especially when running so few tests.

> Yet checking with out docs... there isn't an newcomer friendly place
> that describes how to do your setup. If you can spare a few minutes
> and write something up that'll be amazing - I'm short of ideas where
> did you (and other newcomers) will expect to find such instructions.

I guess this would be a reasonable place: http://www.mesa3d.org/devinfo.html .

I'll try to write up something.

> Huge thanks again for the great work !

Thanks to you!

Best,

Nicolas