[Mesa-dev] [PATCH mesa 4/5] wayland-egl: Make wl_egl_window a versioned struct
Emil Velikov
emil.l.velikov at gmail.com
Thu Jul 20 18:45:55 UTC 2017
On 20 July 2017 at 01:38, Miguel Angel Vico <mvicomoya at nvidia.com> wrote:
>
>
> 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?
>
I'm inclined to keep them separate as well.
>>
>> > ---
>> > 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.
>
Indeed the const makes the compiler unhappy. It's fine as-is and if we
really want to we can drop it at a later stage.
It won't be an issue - API/ABI/other.
The updated patches look great, for the lot
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
I'd like to have these in 17.2 (branch points tomorrow/shortly after),
but I'll give these another day for people to skim though.
Thanks
Emil
More information about the mesa-dev
mailing list