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

[Emil Velikov]

On 20 July 2017 at 01:38, Miguel Angel Vico  wrote:
>
>
> On Wed, 19 Jul 2017 12:06:06 +0100
> Emil Velikov  wrote:
>
>> On 18 July 2017 at 21:49, Miguel A. Vico  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 
>> > Reviewed-by: James Jones 
>>
>> 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 
>> >  #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 
>> > +#include 
>> >
>> >  #include 
>> >  #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 @@ 

Re: [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  wrote:

> On 18 July 2017 at 21:49, Miguel A. Vico  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 
> > Reviewed-by: James Jones   
> 
> 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 
> >  #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 
> > +#include 
> >
> >  #include 
> >  #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
> 

Re: [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  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 
> Reviewed-by: James Jones 

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 
>  #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 
> +#include 
>
>  #include 
>  #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
___

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

[Miguel A. Vico]

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 
Reviewed-by: James Jones 
---
 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 
 #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));
+   }
+   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 
+#include 
 
 #include 
 #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);
+
egl_window->surface = surface;
egl_window->private = NULL;
egl_window->resize_callback = NULL;
@@ -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;
-   
+
return egl_window;
 }
 
-- 
2.12.2

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev