Hi Emil, On 15 September 2017 at 15:54, Emil Velikov <[email protected]> wrote: > On 15 September 2017 at 14:50, Daniel Stone <[email protected]> wrote: >> 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?
Yep! That would be good thanks. >> 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? Sure, that sounds good to me. >> 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. I'd really prefer to, to be honest; if you take, for instance, the build system changes, it's quite clear that those changes are separate from the imported files. >>> +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. I mean that, as written, wl_egl_window_v3 and wl_egl_window are identical, and written out twice. It makes it easier for them to accidentally drop out of sync; perhaps there's a way to reuse the definitions to avoid duplication of the most recent one. I guess the test does have checks against current though, so it's probably fine. >>> +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. Oh, because it's const. Ugh, that's incredibly nasty. :( I would suggest it's worth dropping the const; anyone modifying it is clearly doing something deeply stupid, and I don't like the gymnastics it introduces in the struct definition. (Though, when using it, why not just initialise surface as well, dropping all the other assignments to the malloced struct?) > 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? The problem is that we don't know the backend which will be used for a wl_egl_window, so the negotiation would have to be dynamic at the time of CreateWindowSurface/etc, which would require driver -> wl_egl_window callbacks etc. I'd rather not go down that path unless we have good reason for it. Cheers, Daniel _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
