Hi Emil, Many thanks for your detailed review!
On Tue, Jun 5, 2018 at 12:37 PM, Emil Velikov <emil.l.veli...@gmail.com> wrote: > Hi Olivier, > > There's a handful of mostly trivial suggestions below. The idea itself seems > reasonable IMHO. One gripe is that we're 'leaking' twice as much as before. > > Namely: even if the current backend cleans-up after itself (it some cases it > does not), the other backend 'leaks'. Not sure if/how much we should care in > that case. > > Was skimming if we cannot move more init_egl/init_screen tripping points > and I've noticed a few gotchas/bugs. None of which are requirement for the > series, although great to have. > > > xwl_glamor_gbm_init_egl > - if !render_node error path is leaking humm, not sure, leaking what? we haven't allocated anything yet, have we? But anyhow, it's unralted to this refactoring I think. > - surely we'd want to error out when GL_OES_EGL_image is missing > xwl_glamor_gbm_init_screen Sure, will add separate patch for that, seems it got lost with commit 1545e2dba ("xwayland: Decouple GBM from glamor"). > - no need to RegisterPrivateKey/AddCallback if a rendernode is opened Yeap, will add a separate patch for this. > xwl_glamor_eglstream_init_egl > - using EGL_IMG_context_priority w/o checking for it > xwl_glamor_eglstream_init_screen Ditto. > - the ->controller check is no longer needed Ditto. >> +static Bool >> +xwl_glamor_gbm_has_egl_extension(void) >> +{ >> + return epoxy_has_egl_extension(NULL, "EGL_MESA_platform_gbm"); > I'd also check for EGL_KHR_platform_gbm - it's trivial enough ;-) Sure! >> @@ -71,9 +71,24 @@ xwl_glamor_init_wl_registry(struct xwl_screen *xwl_screen, >> uint32_t id, const char *interface, >> uint32_t version) >> { >> - if (xwl_screen->egl_backend.init_wl_registry) >> - xwl_screen->egl_backend.init_wl_registry(xwl_screen, registry, >> - interface, id, version); >> +#ifdef GLAMOR_HAS_GBM >> + if (xwl_screen->gbm_backend.is_available && >> + xwl_screen->gbm_backend.init_wl_registry && >> + xwl_screen->gbm_backend.init_wl_registry(xwl_screen, >> + registry, >> + id, >> + interface, >> + version)); /* no-op */ >> +#endif >> +#ifdef XWL_HAS_EGLSTREAM >> + else if (xwl_screen->eglstreams_backend.is_available && >> + xwl_screen->eglstreams_backend.init_wl_registry && >> + xwl_screen->eglstreams_backend.init_wl_registry(xwl_screen, >> + registry, >> + id, >> + interface, >> + version)); /* >> no-op */ >> +#endif > Both ifdef guards can go. Well, the idea was to save a few conditionals if the support was not enabled at build time, but I can certainly remove those ifdefs. >> @@ -119,8 +134,8 @@ xwl_glamor_allow_commits(struct xwl_window *xwl_window) >> { >> struct xwl_screen *xwl_screen = xwl_window->xwl_screen; >> >> - if (xwl_screen->egl_backend.allow_commits) >> - return xwl_screen->egl_backend.allow_commits(xwl_window); >> + if (xwl_screen->egl_backend && xwl_screen->egl_backend->allow_commits) > Why the NULL check? Unless I'm missing something that will never happen. We can have no “egl_backend” set at all if “-shm” was specified. Either we keep that test here or we need to check for "xwl_screen->glamor" in xwl_screen_post_damage(), I'll change for the later for clarity. >> @@ -165,17 +180,35 @@ glamor_egl_fd_name_from_pixmap(ScreenPtr screen, >> void >> xwl_glamor_init_backends(struct xwl_screen *xwl_screen, Bool use_eglstreams) >> { > > General nit: > Drop the return type of xwl_glamor_init_$backend now that we check > ::is_available. Sure, it's unused anyway. > Instead of relying on the coded probe order (alongside the > is_available = false workaround), > we could use -eglstream to control which backend is checked/probed first. > > If that fails we fall-back to the other. > > If that sounds reasonable, then the following can be reworked as follows: Sounds reasonable. >> +#ifdef GLAMOR_HAS_GBM >> + /* Init GBM even if "-eglstream" is requested, as EGL streams may fail >> */ >> + xwl_glamor_init_gbm(xwl_screen); > Here we add: > > if (!xwl_screen->gbm_backend.is_available && !use_eglstreams) > ErrorF(xwayland glamor: GBM (default) is not available); > >> +#endif >> #ifdef XWL_HAS_EGLSTREAM >> + xwl_glamor_init_eglstream(xwl_screen); > >> if (use_eglstreams) { >> - if (!xwl_glamor_init_eglstream(xwl_screen)) { >> - ErrorF("xwayland glamor: failed to setup eglstream backend\n"); >> - use_eglstreams = FALSE; >> - } >> + /* Force GBM backend off */ >> + xwl_screen->gbm_backend.is_available = FALSE; > This is no longer needed, and we can now fold the two conditionals - > just like the GBM codepath. Good idea. >> + if (!xwl_screen->eglstreams_backend.is_available) >> + ErrorF("xwayland glamor: EGLstreams requested but not >> available\n"); >> } >> #endif >> - if (!use_eglstreams && !xwl_glamor_init_gbm(xwl_screen)) { >> - ErrorF("xwayland glamor: failed to setup GBM backend, falling back >> to sw accel\n"); >> - xwl_screen->glamor = 0; >> +} >> + >> +void >> +xwl_glamor_select_backend(struct xwl_screen *xwl_screen, Bool >> use_eglstreams) >> +{ >> + if (xwl_screen->egl_backend == NULL && >> xwl_screen->gbm_backend.is_available) { >> + if (xwl_glamor_has_wl_interfaces(xwl_screen, >> &xwl_screen->gbm_backend)) >> + xwl_screen->egl_backend = &xwl_screen->gbm_backend; >> + else >> + ErrorF("Missing Wayland requirements for glamor GBM backend\n"); >> + } >> + if (xwl_screen->egl_backend == NULL && >> xwl_screen->eglstreams_backend.is_available) { >> + if (xwl_glamor_has_wl_interfaces(xwl_screen, >> &xwl_screen->eglstreams_backend)) >> + xwl_screen->egl_backend = &xwl_screen->eglstreams_backend; >> + else >> + ErrorF("Missing Wayland requirements for glamor EGL streams >> backend\n"); > This function can become: > > if (use_eglstreams) > try_eglstreams, fallback to gbm > else > try gbm, fallback to eglstreams Yeap >> Bool >> xwl_present_init(ScreenPtr screen) >> { >> +#ifdef XWL_HAS_EGLSTREAM >> struct xwl_screen *xwl_screen = xwl_screen_get(screen); >> >> /* >> - * doesn't work with the streams backend. we don't have an explicit >> - * boolean for that, but we do know gbm doesn't fill in this hook... >> + * doesn't work with the streams backend. >> */ >> - if (xwl_screen->egl_backend.post_damage != NULL) >> + if (xwl_screen->egl_backend == &xwl_screen->eglstreams_backend) >> return FALSE; >> +#endif >> > Please drop the ifdef guard here - it's not needed. Yeah, same as befor, we can drop those for code clarity. > >> struct xwl_screen; >> >> struct xwl_egl_backend { > >> - void (*init_wl_registry)(struct xwl_screen *xwl_screen, >> + Bool (*init_wl_registry)(struct xwl_screen *xwl_screen, >> struct wl_registry *wl_registry, >> - const char *name, uint32_t id, >> + uint32_t id, const char *name, >> uint32_t version); > I'd keep the mechanical id/name swap a separate commit. Separate commit it will be. Will be posting those change in a minute! Cheers, Olivier. _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel