On Thu, 14 May 2015 20:14:53 -0700 Bryce Harrington <[email protected]> wrote:
> On Mon, May 11, 2015 at 02:19:02PM -0500, Derek Foreman wrote: > > Currently we pass either a single format or no formats to the gl renderer > > create and output_create functions. We extend this to any number of > > formats so we can allow fallback formats if we don't get our first pick. > > > > Signed-off-by: Derek Foreman <[email protected]> > > This all looks fine, but I have some bikesheddy comments. I'll give a > R-B and you can decide whether or not my suggestions have any utility. > > Reviewed-by: Bryce Harrington <[email protected]> Hi, I agree with Bryce's comments below, and also agree that this patch is also landable as is. Reviewed-by: Pekka Paalanen <[email protected]> I have made three further notes below. > > --- > > src/compositor-drm.c | 5 ++-- > > src/compositor-fbdev.c | 4 +-- > > src/compositor-wayland.c | 12 +++++---- > > src/compositor-x11.c | 5 ++-- > > src/gl-renderer.c | 68 > > +++++++++++++++++++++++++++++++----------------- > > src/gl-renderer.h | 6 +++-- > > 6 files changed, 63 insertions(+), 37 deletions(-) > > > > diff --git a/src/compositor-drm.c b/src/compositor-drm.c > > index 0cdb8f4..69bdcfd 100644 > > --- a/src/compositor-drm.c > > +++ b/src/compositor-drm.c > > @@ -1398,7 +1398,7 @@ drm_compositor_create_gl_renderer(struct > > drm_compositor *ec) > > > > format = ec->format; > > if (gl_renderer->create(&ec->base, EGL_PLATFORM_GBM_KHR, (void *) > > ec->gbm, > > - gl_renderer->opaque_attribs, &format) < 0) { > > + gl_renderer->opaque_attribs, &format, 1) < 0) { > > return -1; > > } > > > > @@ -1620,7 +1620,8 @@ drm_output_init_egl(struct drm_output *output, struct > > drm_compositor *ec) > > (EGLNativeDisplayType)output->surface, > > output->surface, > > gl_renderer->opaque_attribs, > > - &format) < 0) { > > + &format, > > + 1) < 0) { > > weston_log("failed to create gl renderer output state\n"); > > gbm_surface_destroy(output->surface); > > return -1; > > diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c > > index 7c505ce..3f3394f 100644 > > --- a/src/compositor-fbdev.c > > +++ b/src/compositor-fbdev.c > > @@ -570,7 +570,7 @@ fbdev_output_create(struct fbdev_compositor *compositor, > > if (gl_renderer->output_create(&output->base, > > (EGLNativeWindowType)NULL, NULL, > > gl_renderer->opaque_attribs, > > - NULL) < 0) { > > + NULL, 0) < 0) { > > weston_log("gl_renderer_output_create failed.\n"); > > goto out_shadow_surface; > > } > > @@ -871,7 +871,7 @@ fbdev_compositor_create(struct wl_display *display, int > > *argc, char *argv[], > > if (gl_renderer->create(&compositor->base, NO_EGL_PLATFORM, > > EGL_DEFAULT_DISPLAY, > > gl_renderer->opaque_attribs, > > - NULL) < 0) { > > + NULL, 0) < 0) { > > weston_log("gl_renderer_create failed.\n"); > > goto out_launcher; > > } > > diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c > > index 303151c..c9983e0 100644 > > --- a/src/compositor-wayland.c > > +++ b/src/compositor-wayland.c > > @@ -648,7 +648,8 @@ wayland_output_init_gl_renderer(struct wayland_output > > *output) > > output->gl.egl_window, > > output->gl.egl_window, > > gl_renderer->alpha_attribs, > > - NULL) < 0) > > + NULL, > > + 0) < 0) > > goto cleanup_window; > > > > return 0; > > @@ -1970,10 +1971,11 @@ wayland_compositor_create(struct wl_display > > *display, int use_pixman, > > > > if (!c->use_pixman) { > > if (gl_renderer->create(&c->base, > > - EGL_PLATFORM_WAYLAND_KHR, > > - c->parent.wl_display, > > - gl_renderer->alpha_attribs, > > - NULL) < 0) { > > + EGL_PLATFORM_WAYLAND_KHR, > > + c->parent.wl_display, > > + gl_renderer->alpha_attribs, > > + NULL, > > + 0) < 0) { > > 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 5129e85..3565677 100644 > > --- a/src/compositor-x11.c > > +++ b/src/compositor-x11.c > > @@ -906,7 +906,8 @@ x11_compositor_create_output(struct x11_compositor *c, > > int x, int y, > > (EGLNativeWindowType) > > output->window, > > &xid, > > gl_renderer->opaque_attribs, > > - NULL); > > + NULL, > > + 0); > > if (ret < 0) > > return NULL; > > } > > @@ -1493,7 +1494,7 @@ init_gl_renderer(struct x11_compositor *c) > > return -1; > > > > ret = gl_renderer->create(&c->base, EGL_PLATFORM_X11_KHR, (void *) > > c->dpy, > > - gl_renderer->opaque_attribs, NULL); > > + gl_renderer->opaque_attribs, NULL, 0); > > > > return ret; > > } > > diff --git a/src/gl-renderer.c b/src/gl-renderer.c > > index ae3122f..6746a9b 100644 > > --- a/src/gl-renderer.c > > +++ b/src/gl-renderer.c > > @@ -1898,14 +1898,37 @@ log_egl_config_info(EGLDisplay egldpy, EGLConfig > > eglconfig) > > } > > > > static int > > +match_config(EGLDisplay egl_display, > > This function name seems a bit non-descript. > match_config_to_visual() maybe? Or match_visual_config()? > > > + EGLint visual_id, > > + EGLConfig *configs, > > + int count) > > +{ > > + int i; > > + > > + for (i = 0; i < count; ++i) { > > + EGLint id; > > + > > + if (!eglGetConfigAttrib(egl_display, > > + configs[i], EGL_NATIVE_VISUAL_ID, > > + &id)) > > + continue; > > I gather eglGetConfigAttrib logs an error code somewhere when it fails? It doesn't, but it's also not important. If you can't query the visual of a config, that config is likely useless anyway. Or the whole EGL implementation is broken, or whatever. We don't care, because if it really turns out to be a problem, we will fail hard elsewhere anyway and this is only collateral fallout. > > + if (id == visual_id) > > + return i; > > + } > > + > > For future debugging purposes, might be friendly to log an error here. > > > + return -1; > > +} > > + > > +static int > > egl_choose_config(struct gl_renderer *gr, const EGLint *attribs, > > - const EGLint *visual_id, > > + const EGLint *visual_id, const int n_ids, > > EGLConfig *config_out) > > Since this appears to just return 0 or -1, would it be clearer to change > it to returning just a boolean? > > > { > > EGLint count = 0; > > EGLint matched = 0; > > EGLConfig *configs; > > - int i; > > + int i, config_index = -1; > > > > if (!eglGetConfigs(gr->egl_display, NULL, 0, &count) || count < 1) > > return -1; > > @@ -1915,31 +1938,25 @@ egl_choose_config(struct gl_renderer *gr, const > > EGLint *attribs, > > return -1; > > > > if (!eglChooseConfig(gr->egl_display, attribs, configs, > > - count, &matched)) > > + count, &matched) || !matched) > > goto out; > > > > - for (i = 0; i < matched; ++i) { > > - EGLint id; > > + if (!visual_id) > > + config_index = 0; > > > > - if (visual_id) { > > - if (!eglGetConfigAttrib(gr->egl_display, > > - configs[i], EGL_NATIVE_VISUAL_ID, > > - &id)) > > - continue; > > + for (i = 0; config_index == -1 && i < n_ids; i++) > > + config_index = match_config(gr->egl_display, visual_id[i], > > + configs, matched); > > > > - if (id != 0 && id != *visual_id) > > - continue; > > - } > > - > > - *config_out = configs[i]; > > - > > - free(configs); > > - return 0; > > - } > > + if (config_index != -1) > > + *config_out = configs[config_index]; > > > > out: > > free(configs); > > - return -1; > > + if (config_index == -1) > > + return -1; > > + > > + return 0; > > } > > The logic above with the config_index seems a bit clunky to me, but I > can't seem to work out something less clunky. My sense is that > overloading the return value of match_config() to both return indexes > and error codes makes handling the result more difficult. > > Perhaps instead of an index, what if you had match_config() returned a > pointer to the appropriate configs[] element (or NULL if none were > found)? The logic as is is fine by me. > > static void > > @@ -1976,7 +1993,8 @@ gl_renderer_output_create(struct weston_output > > *output, > > EGLNativeWindowType window_for_legacy, > > void *window_for_platform, > > const EGLint *attribs, > > - const EGLint *visual_id) > > + const EGLint *visual_id, > > + int n_ids) > > { > > struct weston_compositor *ec = output->compositor; > > struct gl_renderer *gr = get_renderer(ec); > > @@ -1984,7 +2002,8 @@ gl_renderer_output_create(struct weston_output > > *output, > > EGLConfig egl_config; > > int i; > > > > - if (egl_choose_config(gr, attribs, visual_id, &egl_config) == -1) { > > + if (egl_choose_config(gr, attribs, visual_id, > > + n_ids, &egl_config) == -1) { > > weston_log("failed to choose EGL config for output\n"); > > return -1; > > } > > @@ -2260,7 +2279,7 @@ platform_to_extension(EGLenum platform) > > static int > > gl_renderer_create(struct weston_compositor *ec, EGLenum platform, > > void *native_window, const EGLint *attribs, > > - const EGLint *visual_id) > > + const EGLint *visual_id, int n_formats) You call it n_ids everywhere else, would be nice to use the same here too. > > { > > struct gl_renderer *gr; > > EGLint major, minor; > > @@ -2323,7 +2342,8 @@ gl_renderer_create(struct weston_compositor *ec, > > EGLenum platform, > > goto err_egl; > > } > > > > - if (egl_choose_config(gr, attribs, visual_id, &gr->egl_config) < 0) { > > + if (egl_choose_config(gr, attribs, visual_id, > > + n_formats, &gr->egl_config) < 0) { > > weston_log("failed to choose EGL config\n"); > > goto err_egl; > > } > > diff --git a/src/gl-renderer.h b/src/gl-renderer.h > > index ebc139f..ceb4206 100644 > > --- a/src/gl-renderer.h > > +++ b/src/gl-renderer.h > > @@ -76,7 +76,8 @@ struct gl_renderer_interface { > > EGLenum platform, > > void *native_window, > > const EGLint *attribs, > > - const EGLint *visual_id); > > + const EGLint *visual_id, > > + const int n_ids); > > > > EGLDisplay (*display)(struct weston_compositor *ec); > > > > @@ -84,7 +85,8 @@ struct gl_renderer_interface { > > EGLNativeWindowType window_for_legacy, > > void *window_for_platform, > > const EGLint *attribs, > > - const EGLint *visual_id); > > + const EGLint *visual_id, > > + const int n_ids); > > > > void (*output_destroy)(struct weston_output *output); Thanks, pq _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
