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

[Emil Velikov]

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 - 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 ?

>>> 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).

> Or maybe some continuous
> integration farm somewhere we can submit the patch to? (I see
> appveyor.yml, but that's for Windows?)
Appveyor/Travis is for build testing piglit only.

Most people have their CI setups, but they're not public. Would be
great if we can have such a beast (I think we can use jenkins.fd.o),
but I think if mostly boils down to having access to a ton of hardware
;-)
The Intel guys have their setup files available on github [1], if
you're interested.

Regards,
Emil

[1] https://github.com/janesma/mesa_jenkins