Re: [Qemu-devel] [PATCH 3/4] ui: add egl-helpers

[Max Reitz <mreitz@redhat.com>]

On 19.05.2015 16:29, Gerd Hoffmann wrote:
> Add helper functions to initialize OpenGL using egl.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   configure                |   2 +-
>   include/ui/egl-helpers.h |  16 ++++++
>   ui/Makefile.objs         |   3 +
>   ui/egl-helpers.c         | 143 +++++++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 163 insertions(+), 1 deletion(-)
>   create mode 100644 include/ui/egl-helpers.h
>   create mode 100644 ui/egl-helpers.c
>
> diff --git a/configure b/configure
> index df1048a..e0e6b54 100755
> --- a/configure
> +++ b/configure
> @@ -3153,7 +3153,7 @@ else
>   fi
>   
>   if test "$opengl" != "no" ; then
> -  opengl_pkgs="gl glesv2 epoxy"
> +  opengl_pkgs="gl glesv2 epoxy egl"
>     if $pkg_config $opengl_pkgs x11 && test "$have_glx" = "yes"; then
>       opengl_cflags="$($pkg_config --cflags $opengl_pkgs) $x11_cflags"
>       opengl_libs="$($pkg_config --libs $opengl_pkgs) $x11_libs"
> diff --git a/include/ui/egl-helpers.h b/include/ui/egl-helpers.h
> new file mode 100644
> index 0000000..5ad5dc3
> --- /dev/null
> +++ b/include/ui/egl-helpers.h
> @@ -0,0 +1,16 @@
> +#ifndef EGL_HELPERS_H
> +#define EGL_HELPERS_H
> +
> +#include <epoxy/gl.h>
> +#include <epoxy/egl.h>
> +
> +extern EGLDisplay *qemu_egl_display;
> +extern EGLConfig qemu_egl_config;
> +
> +EGLSurface qemu_egl_init_surface_x11(EGLContext ectx, Window win);
> +
> +int qemu_egl_init_dpy(EGLNativeDisplayType dpy, bool gles, bool debug);
> +EGLContext qemu_egl_init_ctx(void);
> +bool qemu_egl_has_ext(const char *haystack, const char *needle);
> +
> +#endif /* EGL_HELPERS_H */
> diff --git a/ui/Makefile.objs b/ui/Makefile.objs
> index 029a42a..5c46870 100644
> --- a/ui/Makefile.objs
> +++ b/ui/Makefile.objs
> @@ -30,11 +30,14 @@ sdl.mo-cflags := $(SDL_CFLAGS)
>   ifeq ($(CONFIG_OPENGL),y)
>   common-obj-y += shader.o
>   common-obj-y += console-gl.o
> +common-obj-y += egl-helpers.o
>   endif
>   
>   gtk.o-cflags := $(GTK_CFLAGS) $(VTE_CFLAGS)
>   shader.o-cflags += $(OPENGL_CFLAGS)
>   console-gl.o-cflags += $(OPENGL_CFLAGS)
> +egl-helpers.o-cflags += $(OPENGL_CFLAGS)
>   
>   shader.o-libs += $(OPENGL_LIBS)
>   console-gl.o-libs += $(OPENGL_LIBS)
> +egl-helpers.o-libs += $(OPENGL_LIBS)
> diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
> new file mode 100644
> index 0000000..4dcca38
> --- /dev/null
> +++ b/ui/egl-helpers.c
> @@ -0,0 +1,143 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <stdbool.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <glob.h>
> +
> +#include "ui/egl-helpers.h"
> +
> +EGLDisplay *qemu_egl_display;
> +EGLConfig qemu_egl_config;
> +
> +/* ---------------------------------------------------------------------- */
> +
> +static int egl_debug;
> +
> +#define egl_dbg(_x ...)                          \
> +    do {                                         \
> +        if (egl_debug) {                         \
> +            fprintf(stderr, "egl: " _x);         \
> +        }                                        \
> +    } while (0);
> +
> +/* ---------------------------------------------------------------------- */
> +
> +EGLSurface qemu_egl_init_surface_x11(EGLContext ectx, Window win)
> +{
> +    EGLSurface esurface;
> +    EGLBoolean b;
> +
> +    egl_dbg("eglCreateWindowSurface (x11 win id 0x%lx) ...\n",
> +            (unsigned long) win);
> +    esurface = eglCreateWindowSurface(qemu_egl_display,
> +                                      qemu_egl_config,
> +                                      (EGLNativeWindowType)win, NULL);
> +    if (!esurface) {

Since I don't have any insights into EGL other than looking up the 
functions online, all I can do is nitpick:

The spec says it's EGL_NO_SURFACE (which probably is 0/NULL, but anyway...).

> +        fprintf(stderr, "egl: eglCreateWindowSurface failed\n");
> +        return NULL;
> +    }
> +
> +    b = eglMakeCurrent(qemu_egl_display, esurface, esurface, ectx);
> +    if (!b) {

EGL_FALSE, but assuming that is 0 is pretty reasonable, I guess. :-)

> +        fprintf(stderr, "egl: eglMakeCurrent failed\n");
> +        return NULL;
> +    }
> +
> +    return esurface;
> +}
> +
> +/* ---------------------------------------------------------------------- */
> +
> +int qemu_egl_init_dpy(EGLNativeDisplayType dpy, bool gles, bool debug)
> +{
> +    static const EGLint conf_att_gl[] = {
> +        EGL_SURFACE_TYPE, EGL_WINDOW_BIT,
> +        EGL_RENDERABLE_TYPE, EGL_OPENGL_BIT,
> +        EGL_RED_SIZE,   1,
> +        EGL_GREEN_SIZE, 1,
> +        EGL_BLUE_SIZE,  1,

Wouldn't it be more like 5, 6, 5? Apparently it doesn't really matter 
whether one specifies 1 or e.g. 5, as long as there is a configuration 
with equal to or more than 5 bits; but we won't be able to work with 
less than 5/6/5 anyway...

> +        EGL_ALPHA_SIZE, 0,
> +        EGL_NONE,
> +    };
> +    static const EGLint conf_att_gles[] = {
> +        EGL_SURFACE_TYPE, EGL_WINDOW_BIT,
> +        EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
> +        EGL_RED_SIZE,   1,
> +        EGL_GREEN_SIZE, 1,
> +        EGL_BLUE_SIZE,  1,
> +        EGL_ALPHA_SIZE, 0,
> +        EGL_NONE,
> +    };
> +    EGLint major, minor;
> +    EGLBoolean b;
> +    EGLint n;
> +
> +    if (debug) {
> +        egl_debug = 1;
> +        setenv("EGL_LOG_LEVEL", "debug", true);
> +        setenv("LIBGL_DEBUG", "verbose", true);
> +    }
> +
> +    egl_dbg("eglGetDisplay (dpy %p) ...\n", dpy);
> +    qemu_egl_display = eglGetDisplay(dpy);
> +    if (!qemu_egl_display) {

EGL_NO_DISPLAY

> +        fprintf(stderr, "egl: eglGetDisplay failed\n");
> +        return -1;
> +    }
> +
> +    egl_dbg("eglInitialize ...\n");
> +    b = eglInitialize(qemu_egl_display, &major, &minor);
> +    if (!b) {
> +        fprintf(stderr, "egl: eglInitialize failed\n");
> +        return -1;
> +    }

Should we care for eglTerminate() after this point (on error) or do we 
just assume there's no hope on failure anyway?

> +
> +    egl_dbg("eglBindAPI ...\n");
> +    b = eglBindAPI(gles ? EGL_OPENGL_ES_API : EGL_OPENGL_API);
> +    if (!b) {
> +        fprintf(stderr, "egl: eglBindAPI failed\n");
> +        return -1;
> +    }
> +
> +    egl_dbg("eglChooseConfig ...\n");
> +    b = eglChooseConfig(qemu_egl_display,
> +                        gles ? conf_att_gles : conf_att_gl,
> +                        &qemu_egl_config, 1, &n);

Well... It would be sad if the system actually supported true color 
(like basically any system everywhere) and we get some 5/6/5 
configuration here (or even worse). I do think it would be better to 
actually receive the whole list and then scan through it to find the 
best configuration; if you want to keep it this way, we probably really 
have to make the attributes above reflect the minimum requirements 
(5/6/5 for R/G/B bit sizes).

> +    if (!b || n != 1) {
> +        fprintf(stderr, "egl: eglChooseConfig failed\n");
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +EGLContext qemu_egl_init_ctx(void)
> +{
> +    static const EGLint ctx_att[] = {
> +        EGL_CONTEXT_CLIENT_VERSION, 2,
> +        EGL_NONE
> +    };
> +
> +    EGLContext ectx;
> +    EGLBoolean b;
> +
> +    egl_dbg("eglCreateContext ...\n");
> +    ectx = eglCreateContext(qemu_egl_display, qemu_egl_config,
> +                            EGL_NO_CONTEXT, ctx_att);

The manpage says specifying EGL_CONTEXT_CLIENT_VERSION is only valid 
when creating an OpenGL ES context.

> +    if (!ectx) {

EGL_NO_CONTEXT :-)

> +        fprintf(stderr, "egl: eglCreateContext failed\n");
> +        return NULL;
> +    }
> +
> +    b = eglMakeCurrent(qemu_egl_display, EGL_NO_SURFACE, EGL_NO_SURFACE, ectx);

What is this call for? So that some rudimentary OpenGL operations can be 
executed which don't need access to a framebuffer?

Max

> +    if (!b) {
> +        fprintf(stderr, "egl: eglMakeCurrent failed\n");
> +        return NULL;
> +    }
> +
> +    return ectx;
> +}