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

[Miguel Angel Vico]

On Wed, 19 Jul 2017 12:06:06 +0100
Emil Velikov <emil.l.velikov at gmail.com> wrote:

> 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 :-(

Yeah... The only think I can think of is pushing both this and 5/5 as a
single commit.

I usually like to keep things separate. Is it much of a deal given that
they'll go in at the same time?

> 
> > ---
> >  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.

Done. 

> 
> > +   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,
> };

This won't work as I made the version field const. I can remove the
const if you feel that's better than my _INIT_ + memcpy solution.

> 
> 
> > @@ -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.

Yeah. Fixed.

> 
> Thanks
> Emil

Thanks.

-- 
Miguel