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

[Emil Velikov]

On 15 September 2017 at 16:21, Daniel Stone <daniel at fooishbar.org> wrote:
> Hi Emil,
>
> On 15 September 2017 at 15:54, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 15 September 2017 at 14:50, Daniel Stone <daniel at fooishbar.org> 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 <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?
>
> Sure, that sounds good to me.
>
Ack.

>>> 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.
>
Still. There's two things - one involves exactly zero brain cells,
while the needs a few.
Is there any way I can sell you against squashing them up, or your mind is set?

>>>> +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.
>
Right... that's perfectly intentional and correct as-is.
You want to have a local instance of each version. Those stay
immutable, forever.
This approach catches mistakes as wl_egl_window get modified, while
the test isn't updated.

Also, currently wl_egl_window_v3 and wl_egl_window are identical,
tomorrow it may be v4.


> 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?)
>
True implementation is a bit unpleasant to look at - I can make it tad better.

For dropping the constness, I have to disagree.
With Vulkan (and the Mesa GLX,EGL <> CL interop) a bidirectional
version field getting rather common.
Assuming that people always will do the right thing is not a great
idea. I think the series as a whole illustrates that ;-)

Having the "const" is trivial and makes people rethink what they're doing.

>> 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.
>
Good point, idea withdrawn.

Thanks
Emil