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 <[email protected]> wrote: > Reviewed-by: Derek Foreman <[email protected]> > Reviewed-by: Bryce Harrington <[email protected]> > --- > 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 _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
