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
- surely we'd want to error out when GL_OES_EGL_image is missing
xwl_glamor_gbm_init_screen
- no need to RegisterPrivateKey/AddCallback if a rendernode is opened
xwl_glamor_eglstream_init_egl
- using EGL_IMG_context_priority w/o checking for it
xwl_glamor_eglstream_init_screen
- the ->controller check is no longer needed
On 1 June 2018 at 15:31, Olivier Fourdan <ofour...@redhat.com> wrote:
> +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 ;-)
> @@ -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.
> @@ -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.
> @@ -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.
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:
> +#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.
> + 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
> 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.
> 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.
HTH
Emil
_______________________________________________
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
