On Wed, 19 Jul 2017 12:06:06 +0100
Emil Velikov <emil.l.veli...@gmail.com> wrote:
> On 18 July 2017 at 21:49, Miguel A. Vico <mvicom...@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 <mvicom...@nvidia.com>
> > Reviewed-by: James Jones <jajo...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
