Hi Dan, Thanks for the quick review/feedback.
On 15 September 2017 at 14:50, Daniel Stone <[email protected]> wrote: > Hi Emil, > > On 15 September 2017 at 11:29, Emil Velikov <[email protected]> wrote: >> Currently we're in a bit of a pickle - both from vendor and user POV. > > This commit message is a bit long-winded. Could you please try to > compress it down to something like: > > Currently the client-facing libwayland-egl API is defined by a header > file shipped by Wayland, but the implementation is left to each > vendor. This can cause collisions when multiple implementations are > installed on the same system. Importing the implementation into > Wayland with a stable and versioned driver-facing ABI allows multiple > drivers etc etc > This sounds really nice. Should I keep the lengthy CC list and/or external repository reference? > Apart from the below, this series, when squashed into a single patch > for import, is: > Reviewed-by: Daniel Stone <[email protected]> > Err... let's not do that, please. We have three fundamentally different things here: - clean copy/import - integration - independent nitpicks Having clear reproducible steps sounds like a clear win IMHO. If you prefer I can address all the nitpics in Mesa first, and then do the import? > I realise this is a straight copy/import, so sorry to pick on these, but: > Ack, will address all the suggestions, but I'll kindly ask you to not fold/squash the series. >> +#ifdef __cplusplus >> +extern "C" { >> +#endif >> + >> +#define WL_EGL_WINDOW_VERSION 3 >> + >> +struct wl_egl_window { >> + const intptr_t version; >> + >> + int width; >> + int height; >> + int dx; >> + int dy; >> + >> + int attached_width; >> + int attached_height; >> + >> + void *private; >> + void (*resize_callback)(struct wl_egl_window *, void *); >> + void (*destroy_window_callback)(void *); >> + >> + struct wl_surface *surface; >> +}; > > I'd like to see this and the definitions of wayland-egl-abi-check.c > not be a complete copy of each other; perhaps in a separate private > header? > Can you elaborate - the believe all the structs (in wayland-egl-abi-check.c and this one) all vary in their own, small, subtle ways. >> +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) >> + return NULL; >> + >> + egl_window = malloc(sizeof *egl_window); >> + if (!egl_window) >> + return NULL; >> + >> + memcpy(egl_window, &_INIT_, sizeof *egl_window); > > The initialiser is weird; it should be static const if it remains, but > why not just use calloc and set it like the rest of the members below? > Using direct assignment for the version results in build failure. Thinking about it ... we might want to make that bidirectional like the ones in Vulkan (and Mesa EGL,GLX <> CL interop). Aka - frontend describes the max size/layout it knows and the backend min(foo.version, self.version). This way the frontend/backend can communicate the version they support, each one having appropriate code to handle older versions. If we opt for that, we want to bump the version to indicate the functionality change? Thanks Emil _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
