[Mesa-dev] [PATCH mesa 4/5] wayland-egl: Make wl_egl_window a versioned struct

[Emil Velikov]

On 18 July 2017 at 21:49, Miguel A. Vico <mvicomoya at nvidia.com> wrote:
> We need wl_egl_window to be a versioned struct in order to keep track of
> ABI changes.
>
> This change makes the first member of wl_egl_window the version number.
>
> An heuristic in the wayland driver is added so that we don't break
> backwards compatibility:
>
>  - If the first field (version) is an actual pointer, it is an old
>    implementation of wl_egl_window, and version points to the wl_surface
>    proxy.
>
>  - Else, the first field is the version number, and we have
>    wl_egl_window::surface pointing to the wl_surface proxy.
>
> Signed-off-by: Miguel A. Vico <mvicomoya at nvidia.com>
> Reviewed-by: James Jones <jajones at nvidia.com>

This commit will cause a break in the ABI checker. Yet again, I'm
short on ideas how to avoid that :-(

> ---
>  src/egl/drivers/dri2/platform_wayland.c        | 13 ++++++++++++-
>  src/egl/wayland/wayland-egl/wayland-egl-priv.h |  6 +++++-
>  src/egl/wayland/wayland-egl/wayland-egl.c      |  6 +++++-
>  3 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/src/egl/drivers/dri2/platform_wayland.c b/src/egl/drivers/dri2/platform_wayland.c
> index ee68284217..0f0a12fd80 100644
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -41,6 +41,7 @@
>  #include "egl_dri2.h"
>  #include "egl_dri2_fallbacks.h"
>  #include "loader.h"
> +#include "eglglobals.h"
>
>  #include <wayland-client.h>
>  #include "wayland-drm-client-protocol.h"
> @@ -100,6 +101,16 @@ destroy_window_callback(void *data)
>     dri2_surf->wl_win = NULL;
>  }
>
> +static struct wl_surface *
> +get_wl_surface_proxy(struct wl_egl_window *window)
> +{
> +   if (_eglPointerIsDereferencable((void *)(window->version))) {
> +      /* window->version points to actual wl_surface data */
> +      return wl_proxy_create_wrapper((void *)(window->version));
> +   }
Please add a comment in there. I'm thinking about the following
although use whatever you prefer.

Version 3 of wl_egl_window introduced a version field, at the same
location where a pointer to wl_surface was stored.

> +   return wl_proxy_create_wrapper(window->surface);
> +}
> +
>  /**
>   * Called via eglCreateWindowSurface(), drv->API.CreateWindowSurface().
>   */
> @@ -171,7 +182,7 @@ dri2_wl_create_window_surface(_EGLDriver *drv, _EGLDisplay *disp,
>     wl_proxy_set_queue((struct wl_proxy *)dri2_surf->wl_dpy_wrapper,
>                        dri2_surf->wl_queue);
>
> -   dri2_surf->wl_surface_wrapper = wl_proxy_create_wrapper(window->surface);
> +   dri2_surf->wl_surface_wrapper = get_wl_surface_proxy(window);
>     if (!dri2_surf->wl_surface_wrapper) {
>        _eglError(EGL_BAD_ALLOC, "dri2_create_surface");
>        goto cleanup_drm;
> diff --git a/src/egl/wayland/wayland-egl/wayland-egl-priv.h b/src/egl/wayland/wayland-egl/wayland-egl-priv.h
> index 92c31d9454..3b59908cc1 100644
> --- a/src/egl/wayland/wayland-egl/wayland-egl-priv.h
> +++ b/src/egl/wayland/wayland-egl/wayland-egl-priv.h
> @@ -41,8 +41,10 @@
>  extern "C" {
>  #endif
>
> +#define WL_EGL_WINDOW_VERSION 3
> +
>  struct wl_egl_window {
> -       struct wl_surface *surface;
> +       const intptr_t version;
>
>         int width;
>         int height;
> @@ -55,6 +57,8 @@ struct wl_egl_window {
>         void *private;
>         void (*resize_callback)(struct wl_egl_window *, void *);
>         void (*destroy_window_callback)(void *);
> +
> +       struct wl_surface *surface;
>  };
>
>  #ifdef  __cplusplus
> diff --git a/src/egl/wayland/wayland-egl/wayland-egl.c b/src/egl/wayland/wayland-egl/wayland-egl.c
> index 4a4701a2de..02645549e0 100644
> --- a/src/egl/wayland/wayland-egl/wayland-egl.c
> +++ b/src/egl/wayland/wayland-egl/wayland-egl.c
> @@ -28,6 +28,7 @@
>   */
>
>  #include <stdlib.h>
> +#include <string.h>
>
>  #include <wayland-client.h>
>  #include "wayland-egl.h"
> @@ -54,6 +55,7 @@ WL_EGL_EXPORT struct wl_egl_window *
>  wl_egl_window_create(struct wl_surface *surface,
>                      int width, int height)
>  {
> +       struct wl_egl_window _INIT_ = { .version = WL_EGL_WINDOW_VERSION };
>         struct wl_egl_window *egl_window;
>
>         if (width <= 0 || height <= 0)
> @@ -63,6 +65,8 @@ wl_egl_window_create(struct wl_surface *surface,
>         if (!egl_window)
>                 return NULL;
>
> +       memcpy(egl_window, &_INIT_, sizeof *egl_window);
> +
The _INIT_ and memcpy seems like an overkill. At the same time the
current malloc + init each member is not that robust.
Something like the following seems reasonable, imho.

*egl_window = (struct wl_egl_window) {
 .version = WL_EGL_WINDOW_VERSION,
 .surface = surface,
 .width = width,
 .height = height,
};


> @@ -70,7 +74,7 @@ wl_egl_window_create(struct wl_surface *surface,
>         wl_egl_window_resize(egl_window, width, height, 0, 0);
>         egl_window->attached_width  = 0;
>         egl_window->attached_height = 0;
> -
> +
Unrelated whitespace change.

Thanks
Emil