[waffle] [PATCH v3 4/4] gbm: initial functional gbm support

[Chad Versace]

[Apologies for long lines. Thunderbird is misbehaving today.]

On 09/13/2012 10:04 AM, Jordan Justen wrote:
> src/waffle/gbm is based on src/waffle/wayland, and then heavily
> modified.
> 
> Tested as functional with: examples/gl_basic gbm gl|gles2
> 
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> ---
>  src/waffle/gbm/gbm_config.c   |   75 ++++++++++++++++++++++++++++-
>  src/waffle/gbm/gbm_context.c  |   55 ++++++++++++++++++++-
>  src/waffle/gbm/gbm_display.c  |  107 +++++++++++++++++++++++++++++++++++++++--
>  src/waffle/gbm/gbm_platform.c |   51 ++++++++++++++++++--
>  src/waffle/gbm/gbm_window.c   |   71 +++++++++++++++++++++++++--
>  5 files changed, 343 insertions(+), 16 deletions(-)

I also tested the series with `examples/gl_basic gbm gl|gles1|gles2`, and it worked for me. Mostly this patch is good, but I found a few problems.

> diff --git a/src/waffle/gbm/gbm_config.c b/src/waffle/gbm/gbm_config.c

> +static uint32_t
> +get_gbm_format(const struct wcore_config_attrs *attrs)
> +{
> +    if (attrs->depth_size != WAFFLE_DONT_CARE ||
> +        attrs->stencil_size != WAFFLE_DONT_CARE) {
> +        return 0;
> +    }
> +
> +    if (attrs->red_size == 8 && attrs->blue_size == 8 && attrs->green_size == 8) {
> +        if (attrs->alpha_size == WAFFLE_DONT_CARE) {
> +            return GBM_FORMAT_XRGB8888;
> +        } else if (attrs->alpha_size == 8) {
> +            return GBM_FORMAT_ABGR8888;
> +        }
> +    }
> +
> +    return 0;
>  }

I had a conversation with Kristian at XDC. According to him, it should be possible to use an EGLConfig with depth and stencil bits. I'll perform an experiment and see how it works. But this patch shouldn't be blocked by that pending experiment: there is no harm in excluding such configs in this patch since its purpose it just to add initial support.

Anyway, assuming that depth/stencil configs are not allowed, the checks should be as below because, according to table 3.4 in the EGL 1.4 spec, the selection criteria for each of the related EGLConfig attributes is 'AtLeast'.

    // Using special knowledge that WAFFLE_DONT_CARE is -1 and
    // the only valid negative attribute value.

    if (attrs->depth_size > 0 || attrs->stencil_size > 0) {
        return 0;
    }

    if (attrs->red_size != 0 && attrs->blue_size != 0 && attrs->green_size != 0) {
        if (attrs->alpha_size > 0)
            return GBM_FORMAT_ARGB8888;
        else
            return GBM_FORMAT_XRGB8888;
    }
        
[snip]

>  static union waffle_native_config*
>  gbm_config_get_native(struct wcore_config *wc_self)
>  {
> -    return NULL;
> +    struct gbm_config *self = gbm_config(wc_self);
> +    struct gbm_display *dpy = gbm_display(wc_self->display);
> +    struct waffle_gbm_config *n_config;
> +
> +    n_config = wcore_malloc(sizeof(*n_config));
> +    if (n_config == NULL)
> +        return NULL;
> +
> +    gbm_display_fill_native(dpy, &n_config->display);
> +    n_config->egl_config = self->egl;
> +
> +    return (union waffle_native_config*) n_config;
>  }

You faithfully copied my buggy code in the Wayland backend :). I've fixed the bug,
so take a look at the current implementations of wayland_*_get_native.

Also, it looks like the path series forgets to add the 'gbm' fields to some of the `union waffle_*_native` types.

> diff --git a/src/waffle/gbm/gbm_display.c b/src/waffle/gbm/gbm_display.c
> index a38f8a4..896b837 100644
> --- a/src/waffle/gbm/gbm_display.c
> +++ b/src/waffle/gbm/gbm_display.c
> @@ -47,13 +47,97 @@ static const struct wcore_display_vtbl gbm_display_wcore_vtbl;
>  static bool
>  gbm_display_destroy(struct wcore_display *wc_self)
>  {
> -    return false;
> +    struct gbm_display *self = gbm_display(wc_self);
> +    bool ok = true;
> +
> +    if (!self)
> +        return ok;
> +
> +    if (self->egl)
> +        ok &= egl_terminate(self->egl);
> +
> +    if (self->gbm_device)
> +        gbm_device_destroy(self->gbm_device);
> +
> +    ok &= wcore_display_teardown(&self->wcore);
> +    free(self);
> +    return ok;
> +}

Should Waffle close the fd in gbm_display_destroy()? I quickly dove into gbm_device_destroy(), and, at first glance, it looks like it doesn't close the fd for us.

> diff --git a/src/waffle/gbm/gbm_platform.c b/src/waffle/gbm/gbm_platform.c
> index fca6ab5..c8f57e2 100644
> --- a/src/waffle/gbm/gbm_platform.c
> +++ b/src/waffle/gbm/gbm_platform.c
> @@ -43,12 +43,48 @@ static const struct wcore_platform_vtbl gbm_platform_wcore_vtbl;
>  static bool
>  gbm_platform_destroy(struct wcore_platform *wc_self)
>  {
> -    return false;
> +    struct gbm_platform *self = gbm_platform(wc_self);
> +    bool ok = true;
> +
> +    if (!self)
> +        return true;
> +
> +    unsetenv("EGL_PLATFORM");
> +    unsetenv("EGL_DRIVER");

I think it's mean to unset EGL_DRIVER behind the user's back, especially in the case where the user has manually set it himself.

> +
> +    if (self->linux)
> +        ok &= linux_platform_destroy(self->linux);
> +
> +    ok &= wcore_platform_teardown(wc_self);
> +    free(self);
> +    return ok;
>  }