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

[Olivier Fourdan]

Hi Emil,

Many thanks for your detailed review!

On Tue, Jun 5, 2018 at 12:37 PM, Emil Velikov <emil.l.velikov at 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.