[PATCH weston 1/4] gl-renderer: use eglGetPlatformDisplayEXT to get an EGLDisplay
Pekka Paalanen
ppaalanen at gmail.com
Mon Mar 23 03:10:03 PDT 2015
Hi,
sorry to come to this after it has already landed. I have some comments
here. It's mostly just clean-ups and simplifications, though.
On Fri, 20 Mar 2015 15:26:50 +0100
Jonny Lamb <jonny.lamb at collabora.co.uk> wrote:
> Reviewed-by: Derek Foreman <derekf at osg.samsung.com>
> Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>
> ---
> src/compositor-drm.c | 13 ++++++-
> src/compositor-fbdev.c | 2 +-
> src/compositor-wayland.c | 20 +++++++++--
> src/compositor-x11.c | 11 +++++-
> src/gl-renderer.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++--
> src/gl-renderer.h | 6 +++-
> 6 files changed, 132 insertions(+), 10 deletions(-)
>
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index ed4eabf..b519722 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -52,6 +52,8 @@
> #include "vaapi-recorder.h"
> #include "presentation_timing-server-protocol.h"
>
> +#include <EGL/eglext.h>
> +
> #ifndef DRM_CAP_TIMESTAMP_MONOTONIC
> #define DRM_CAP_TIMESTAMP_MONOTONIC 0x6
> #endif
> @@ -68,6 +70,10 @@
> #define GBM_BO_USE_CURSOR GBM_BO_USE_CURSOR_64X64
> #endif
>
> +#ifndef EGL_PLATFORM_GBM_KHR
> +#define EGL_PLATFORM_GBM_KHR 0x31D7
> +#endif
All this EGL stuff should probably be moved into gl-renderer.h where it
will be appropriately protected with #ifdef ENABLE_EGL.
> +
> static int option_current_mode = 0;
>
> enum output_config {
> @@ -1396,8 +1402,13 @@ drm_compositor_create_gl_renderer(struct drm_compositor *ec)
> {
> EGLint format;
>
> + if (!gl_renderer->supports ||
> + gl_renderer->supports(&ec->base, "gbm") < 0) {
> + return -1;
> + }
There is no way gl_renderer->supports could be NULL, so the check for
it is not necessary.
> +
> format = ec->format;
> - if (gl_renderer->create(&ec->base, ec->gbm,
> + if (gl_renderer->create(&ec->base, EGL_PLATFORM_GBM_KHR, (void *) ec->gbm,
> gl_renderer->opaque_attribs, &format) < 0) {
> return -1;
> }
> diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
> index fd22414..ec456bf 100644
> --- a/src/compositor-fbdev.c
> +++ b/src/compositor-fbdev.c
> @@ -868,7 +868,7 @@ fbdev_compositor_create(struct wl_display *display, int *argc, char *argv[],
> goto out_launcher;
> }
>
> - if (gl_renderer->create(&compositor->base, EGL_DEFAULT_DISPLAY,
> + if (gl_renderer->create(&compositor->base, 0, EGL_DEFAULT_DISPLAY,
> gl_renderer->opaque_attribs,
> NULL) < 0) {
I wonder, what is the reason to have gl_renderer->supports() as a
separate function, rather than having gl_renderer-create() take the
platform as an argument and just fail if supports() says it cannot work?
I suppose we could even have a fake fbdev platform definition here,
which makes the EGL-probing to fall back to the old eglGetDisplay.
> weston_log("gl_renderer_create failed.\n");
> diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c
> index 04a2331..b6fd37d 100644
> --- a/src/compositor-wayland.c
> +++ b/src/compositor-wayland.c
> @@ -45,8 +45,14 @@
> #include "fullscreen-shell-client-protocol.h"
> #include "presentation_timing-server-protocol.h"
>
> +#include <EGL/eglext.h>
> +
> #define WINDOW_TITLE "Weston Compositor"
>
> +#ifndef EGL_PLATFORM_WAYLAND_KHR
> +#define EGL_PLATFORM_WAYLAND_KHR 0x31D8
> +#endif
To gl-renderer.h.
> +
> struct wayland_compositor {
> struct weston_compositor base;
>
> @@ -1961,9 +1967,17 @@ wayland_compositor_create(struct wl_display *display, int use_pixman,
> }
>
> if (!c->use_pixman) {
> - if (gl_renderer->create(&c->base, c->parent.wl_display,
> - gl_renderer->alpha_attribs,
> - NULL) < 0) {
> + if (!gl_renderer->supports ||
> + gl_renderer->supports(&c->base, "wayland") < 0) {
> + weston_log("No support for "
> + "EGL_{KHR,EXT,MESA}_platform_wayland; "
> + "falling back to pixman.\n");
> + c->use_pixman = 1;
> + } else if (gl_renderer->create(&c->base,
> + EGL_PLATFORM_WAYLAND_KHR,
> + c->parent.wl_display,
> + gl_renderer->alpha_attribs,
> + NULL) < 0) {
Both branches are falling back to Pixman anyway, and gl-renderer could
log the failure messages, so this could be unified with the other
backends too.
> weston_log("Failed to initialize the GL renderer; "
> "falling back to pixman.\n");
> c->use_pixman = 1;
> diff --git a/src/compositor-x11.c b/src/compositor-x11.c
> index e9735c5..0ab2f81 100644
> --- a/src/compositor-x11.c
> +++ b/src/compositor-x11.c
> @@ -53,8 +53,14 @@
> #include "../shared/image-loader.h"
> #include "presentation_timing-server-protocol.h"
>
> +#include <EGL/eglext.h>
> +
> #define DEFAULT_AXIS_STEP_DISTANCE wl_fixed_from_int(10)
>
> +#ifndef EGL_PLATFORM_X11_KHR
> +#define EGL_PLATFORM_X11_KHR 0x31D5
> +#endif
To gl-renderer.h.
> +
> static int option_width;
> static int option_height;
> static int option_scale;
> @@ -1487,7 +1493,10 @@ init_gl_renderer(struct x11_compositor *c)
> if (!gl_renderer)
> return -1;
>
> - ret = gl_renderer->create(&c->base, (EGLNativeDisplayType) c->dpy,
> + if (!gl_renderer->supports || gl_renderer->supports(&c->base, "x11") < 0)
> + return -1;
> +
> + ret = gl_renderer->create(&c->base, EGL_PLATFORM_X11_KHR, (void *) c->dpy,
> gl_renderer->opaque_attribs, NULL);
>
> return ret;
> diff --git a/src/gl-renderer.c b/src/gl-renderer.c
> index 10d7d71..e598d1e 100644
> --- a/src/gl-renderer.c
> +++ b/src/gl-renderer.c
> @@ -166,6 +166,10 @@ struct gl_renderer {
> struct wl_signal destroy_signal;
> };
>
> +#ifdef EGL_EXT_platform_base
> +static PFNEGLGETPLATFORMDISPLAYEXTPROC get_platform_display = NULL;
> +#endif
> +
> static inline struct gl_output_state *
> get_output_state(struct weston_output *output)
> {
> @@ -2148,9 +2152,69 @@ static const EGLint gl_renderer_alpha_attribs[] = {
> EGL_NONE
> };
>
> +/** Checks whether a platform EGL client extension is supported
> + *
> + * \param ec The weston compositor
> + * \param extension_suffix The EGL client extension suffix
> + * \return 1 if supported, 0 if using fallbacks, -1 unsupported
> + *
> + * This function checks whether a specific platform_* extension is supported
> + * by EGL.
> + *
> + * The extension suffix should be the suffix of the platform extension (that
> + * specifies a <platform> argument as defined in EGL_EXT_platform_base). For
> + * example, passing "foo" will check whether either "EGL_KHR_platform_foo",
> + * "EGL_EXT_platform_foo", or "EGL_MESA_platform_foo" is supported.
> + *
> + * The return value is 1:
> + * - if the supplied EGL client extension is supported.
> + * The return value is 0:
> + * - if the platform_base client extension isn't supported so will
> + * fallback to eglGetDisplay and friends.
> + * The return value is -1:
> + * - if the supplied EGL client extension is not supported.
> + */
> static int
> -gl_renderer_create(struct weston_compositor *ec, EGLNativeDisplayType display,
> - const EGLint *attribs, const EGLint *visual_id)
> +gl_renderer_supports(struct weston_compositor *ec,
> + const char *extension_suffix)
> +{
> + static const char *extensions = NULL;
> + char s[64];
> +
> + if (!extensions) {
> + extensions = (const char *) eglQueryString(
> + EGL_NO_DISPLAY, EGL_EXTENSIONS);
> +
> + if (!extensions)
> + return 0;
> +
> + log_extensions("EGL client extensions",
> + extensions);
> + }
> +
> + snprintf(s, sizeof s, "EGL_KHR_platform_%s", extension_suffix);
> + if (strstr(extensions, s))
> + return 1;
> +
> + snprintf(s, sizeof s, "EGL_EXT_platform_%s", extension_suffix);
> + if (strstr(extensions, s))
> + return 1;
> +
> + snprintf(s, sizeof s, "EGL_MESA_platform_%s", extension_suffix);
> + if (strstr(extensions, s))
> + return 1;
> +
> + /* at this point we definitely have some client extensions but
> + * haven't found the supplied client extension, so chances are it's
> + * not supported. */
> +
> + return -1;
> +}
> +
> +static int
> +gl_renderer_create(struct weston_compositor *ec, EGLenum platform,
> + void *native_window, const EGLint *attribs,
> + const EGLint *visual_id)
> {
> struct gl_renderer *gr;
> EGLint major, minor;
> @@ -2169,7 +2233,26 @@ gl_renderer_create(struct weston_compositor *ec, EGLNativeDisplayType display,
> gl_renderer_surface_get_content_size;
> gr->base.surface_copy_content = gl_renderer_surface_copy_content;
>
> - gr->egl_display = eglGetDisplay(display);
> +#ifdef EGL_EXT_platform_base
> + if (!get_platform_display) {
> + get_platform_display =
> + (void *) eglGetProcAddress("eglGetPlatformDisplayEXT");
> + }
> +
> + if (get_platform_display && platform) {
> + gr->egl_display =
> + get_platform_display(platform, native_window,
> + NULL);
> + } else {
> +#endif
> + weston_log("warning: either no EGL_EXT_platform_base "
> + "support or specific platform support; "
> + "falling back to eglGetDisplay.\n");
> + gr->egl_display = eglGetDisplay(native_window);
> +#ifdef EGL_EXT_platform_base
> + }
> +#endif
Do we actually need to #ifdef EGL_EXT_platform_base?
If we had a fallback definition for PFNEGLGETPLATFORMDISPLAYEXTPROC,
couldn't we drop all those #ifdefs?
> +
> if (gr->egl_display == EGL_NO_DISPLAY) {
> weston_log("failed to create display\n");
> goto err_egl;
> @@ -2381,6 +2464,7 @@ WL_EXPORT struct gl_renderer_interface gl_renderer_interface = {
> .opaque_attribs = gl_renderer_opaque_attribs,
> .alpha_attribs = gl_renderer_alpha_attribs,
>
> + .supports = gl_renderer_supports,
> .create = gl_renderer_create,
> .display = gl_renderer_display,
> .output_create = gl_renderer_output_create,
> diff --git a/src/gl-renderer.h b/src/gl-renderer.h
> index 77b1952..bbf0ac6 100644
> --- a/src/gl-renderer.h
> +++ b/src/gl-renderer.h
> @@ -50,8 +50,12 @@ struct gl_renderer_interface {
> const EGLint *opaque_attribs;
> const EGLint *alpha_attribs;
>
> + int (*supports)(struct weston_compositor *ec,
> + const char *extension_suffix);
> +
> int (*create)(struct weston_compositor *ec,
> - EGLNativeDisplayType display,
> + EGLenum platform,
> + void *native_window,
> const EGLint *attribs,
> const EGLint *visual_id);
If we relied on create() just failing with proper logging, there would
be no need to expose supports(), is there?
Testing the build and run of all this with --disable-egl for Weston's
configure would be nice if you didn't already.
Also testing the runtime renderer switch with DRM-backend would be
cool. I don't think that gets much testing.
Thanks,
pq
More information about the wayland-devel
mailing list