[PATCH xserver 5/5] xwayland: refactor egl_backends for wayland registry

[Emil Velikov]

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 <ofourdan at 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