[RFC wayland 1/9] wayland-egl: import libwayland-egl.so frontend library from Mesa

[Emil Velikov]

Hi Dan,

Thanks for the quick review/feedback.

On 15 September 2017 at 14:50, Daniel Stone <daniel at fooishbar.org> wrote:
> Hi Emil,
>
> On 15 September 2017 at 11:29, Emil Velikov <emil.l.velikov at gmail.com> 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 <daniels at collabora.com>
>
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