Re: [Qemu-devel] [PATCH] egl-helpers: Support newer MESA versions

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 13-03-17 10:39, Frediano Ziglio wrote:
>>
>> <resend with some more people added to the Cc>
>>
>> Hi,
>>
>> On 20-02-17 10:50, Frediano Ziglio wrote:
>>> According to
>>> https://www.khronos.org/registry/EGL/extensions/MESA/EGL_MESA_platform_gbm.txt
>>> if MESA_platform_gbm is supported display should be initialized
>>> from a GBM handle using eglGetPlatformDisplayEXT.
>>>
>>> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
>>> ---
>>> This should fix
>>> http://www.spinics.net/linux/fedora/libvir/msg142837.html
>>>
>>> Tested on Fedora rawhide.
>>> ---
>>>  ui/egl-helpers.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
>>> index cd24568..964c5a5 100644
>>> --- a/ui/egl-helpers.c
>>> +++ b/ui/egl-helpers.c
>>> @@ -219,7 +219,11 @@ int qemu_egl_init_dpy(EGLNativeDisplayType dpy, bool
>>> gles, bool debug)
>>>      }
>>>
>>>      egl_dbg("eglGetDisplay (dpy %p) ...\n", dpy);
>>> +#ifdef EGL_MESA_platform_gbm
>>> +    qemu_egl_display = eglGetPlatformDisplayEXT(EGL_PLATFORM_GBM_MESA,
>>> dpy, NULL);
>>> +#else
>>>      qemu_egl_display = eglGetDisplay(dpy);
>>> +#endif
>>>      if (qemu_egl_display == EGL_NO_DISPLAY) {
>>>          error_report("egl: eglGetDisplay failed");
>>>          return -1;
>>>
>>
>> That fix is incomplete, you need some magic to work properly on older libGL
>> versions.
>>
>> Attached is a (compile tested only) proper patch. I do not have a qemu git
>> clone
>> handy atm, so this is not a git format-patch patch, it is against the Fedora
>> Rawhide
>> srpm. I've a test build with these patches here:
>>
>> https://fedorapeople.org/~jwrdegoede/qemu-glvnd/
>>
>> I was planning on doing a git clone qemu and send a proper patch after I got
>> some
>> testing feedback. Feel free to use this as a base for a v2 of your patch.
>>
>> Regards,
>>
>> Hans
>>
>
> Wouldn't be easier to call the "old" eglGetDisplay if eglGetPlatformDisplayEXT
> returns EGL_NO_DISPLAY ? Kind of

With libEGL you (unfortunately) cannot assume that the lib your building
against is the lib you will run against.

>
>      egl_dbg("eglGetDisplay (dpy %p) ...\n", dpy);
> -    qemu_egl_display = eglGetDisplay(dpy);
> +#ifdef EGL_MESA_platform_gbm
> +    qemu_egl_display = eglGetPlatformDisplayEXT(EGL_PLATFORM_GBM_MESA, dpy, NULL);

So this may very well lead to a runtime unresolved symbol error. IOW you really
need to do the special magic dance I'm afraid.

> +#else
> +    qemu_egl_display = EGL_NO_DISPLAY;
> +#endif
> +    if (qemu_egl_display == EGL_NO_DISPLAY)
> +        qemu_egl_display = eglGetDisplay(dpy);
>      if (qemu_egl_display == EGL_NO_DISPLAY) {
>          error_report("egl: eglGetDisplay failed");
>          return -1;
>
> Your patch should not even compile on older system which does not define
> EGL_PLATFORM_GBM_MESA.

True, so I guess my patch needs an #ifdef around the new qemu_egl_get_display()
function and then on the actual call site #ifdef #else #endif like your patch,
but instead of directly calling eglGetPlatformDisplayEXT call qemu_egl_get_display.

> I tested my patch and works on both rawhide and FC25 (with very recent Mesa version)

mesa is not the problem, it is say running qemu on ARM and using some binary
driver libEGL.so which is the problem, without the dance that will result in
a runtime dynamic linker error and qemu aborting.

Regards,

Hans