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

[Duncan Roe]

Hi Emil,

On Mon, Sep 18, 2017 at 02:18:42PM +0100, Emil Velikov wrote:
> Hi Duncan,
>
> On 17 September 2017 at 09:04, Duncan Roe  wrote:
> > On Fri, Sep 15, 2017 at 11:29:19AM +0100, Emil Velikov wrote:
> >> From: Emil Velikov 
...

Agree this is orthogonal but just for the record
> >
> > I first packaged wayland-egl when it showed up as a dependency of VLC built 
> > with
> > all optional dependencies (and optional dependencies of dependencies ...). 
> > As
> > such, I have no idea when or even if wayland-egl gets used or how I would 
> > set
> > about testing whether a new release has an adverse effect on VLC.
> >
> Right, I've saw that and was worried how you tested the codepaths.

I found that I'd put this explanation in the slack-desc

 The Wayland display server protocol uses EGL. If you build qt5 with
 its optional dependency wayland and then build vlc then vlc will
 require wayland-egl.

I don't have a Wayland display server so can't test.

> AFAICT there's no backend driver or Mesa in SBo.

Slackware 14.2 provides Mesa-11.2.2. SBo only provides packages *not* in 
Slackware.

Cheers ... Duncan.
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Duncan Roe]

Hi Emil,

On Mon, Sep 18, 2017 at 02:18:42PM +0100, Emil Velikov wrote:
> Hi Duncan,
>
> On 17 September 2017 at 09:04, Duncan Roe  wrote:
> > On Fri, Sep 15, 2017 at 11:29:19AM +0100, Emil Velikov wrote:
> >> From: Emil Velikov 
...
>
> > I've made the updates locally, and see that it still builds
> > libwayland-egl.so.1.0.0. Shouldn't there be a version bump?
> >
> There is no difference in the sources or binary. Thus a version bump
> is a very bad idea.
>
Perhaps I didn't explain properly:

I built wayland-egl from code in Mesa-17.0.4. That built
libwayland-egl.so.1.0.0. I rebuilt wayland-egl from your code in Mesa-17.1.9.
That also built libwayland-egl.so.1.0.0.

Between the 2 revisions, the layout of struct wl_egl_window is changed to have
const intptr_t version inserted as its first element with struct wl_surface
*surface moved from first element to last.

The new .so builds to be the same size as the old one but cmp -l shows 1395
differing bytes,

Cheers ... Duncan.
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Duncan Roe]

On Fri, Sep 15, 2017 at 11:29:19AM +0100, Emil Velikov wrote:
> From: Emil Velikov 
...
>
> Takanari, Duncan,
> AFAICT the Renesas devices that you're using have an ImgTec IP inside
> them. Please check with Renesas and/or ImgTec, as one can build their
> driver as a Mesa component.
>
> As such you should be able to utilise the EGL/Wayland, EGL/Android and
> other features without having to write/maintain wrapper shims.

Hi Emil,

The only reason I'm involved is that I package wayland-egl for SBo.

SlackBuilds.org (SBo) provide additional packages ready to install on a system
running the Slackware Linux distribution.

I first packaged wayland-egl when it showed up as a dependency of VLC built with
all optional dependencies (and optional dependencies of dependencies ...). As
such, I have no idea when or even if wayland-egl gets used or how I would set
about testing whether a new release has an adverse effect on VLC.

Your email contains the checked-in Mesa code as of rev 17.1.9 / 17.2.0.

If I update my wayland-egl branch to this code and make a new Slackbuild, is
that likely to break anything?

I've made the updates locally, and see that it still builds
libwayland-egl.so.1.0.0. Shouldn't there be a version bump?

From checking header files it does seem to me that the layout of struct
wl_egl_window is indeed opaque to the outside world, but all the same wouldn't
it be better to be able to see what version of the library one had installed?

Cheers ... Duncan.
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Duncan Roe]

Hi Emil,

On Mon, Sep 18, 2017 at 02:18:42PM +0100, Emil Velikov wrote:
> Hi Duncan,
>
> On 17 September 2017 at 09:04, Duncan Roe  wrote:
> > On Fri, Sep 15, 2017 at 11:29:19AM +0100, Emil Velikov wrote:
> >> From: Emil Velikov 
...
>
> > From checking header files it does seem to me that the layout of struct
> > wl_egl_window is indeed opaque to the outside world, but all the same 
> > wouldn't
> > it be better to be able to see what version of the library one had 
> > installed?
> >
> Why? Can you please elaborate.
>
I searched /usr/include for includes of wayland-egl-priv.h. There weren't any
(as one would hope, since Mesa doesn't expose it).

I then searched /usr/include for declarations of struct wl_surface:

qt5/QtWaylandCompositor/5.7.1/QtWaylandCompositor/private/wayland-wayland-server-protocol.h
90 struct wl_subsurface;
91 struct wl_surface;
92 struct wl_touch;

It's declared but not defined. Code can only work with pointers to the struct
and not its elements, which is what I meant by saying it was "opaque".

Similar results for:
qt5/QtWaylandCompositor/5.7.1/QtWaylandCompositor/private/wayland-surface-extension-server-protocol.h:64
qt5/QtWaylandCompositor/5.7.1/QtWaylandCompositor/private/wayland-qtkey-extension-server-protocol.h:62
qt5/QtWaylandCompositor/5.7.1/QtWaylandCompositor/private/wayland-text-input-unstable-v2-server-protocol.h:51
qt5/QtWaylandCompositor/5.7.1/QtWaylandCompositor/private/wayland-xdg-shell-server-protocol.h:55
qt5/QtWaylandClient/5.7.1/QtWaylandClient/private/wayland-wayland-client-protocol.h:88
qt5/QtWaylandClient/5.7.1/QtWaylandClient/private/wayland-surface-extension-client-protocol.h:61
qt5/QtWaylandClient/5.7.1/QtWaylandClient/private/wayland-qtkey-extension-client-protocol.h:59
qt5/QtWaylandClient/5.7.1/QtWaylandClient/private/wayland-text-input-unstable-v2-client-protocol.h:48
qt5/QtWaylandClient/5.7.1/QtWaylandClient/private/wayland-xdg-shell-client-protocol.h:52
wayland-egl-core.h:37
wayland-server-protocol.h:91
wayland-client-protocol.h:88
vlc/plugins/vlc_vout_window.h:44

Cheers ... Duncan.
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Emil Velikov]

On 19 September 2017 at 06:24, Duncan Roe  wrote:
> Hi Emil,
>
> On Mon, Sep 18, 2017 at 02:18:42PM +0100, Emil Velikov wrote:
>> Hi Duncan,
>>
>> On 17 September 2017 at 09:04, Duncan Roe  wrote:
>> > On Fri, Sep 15, 2017 at 11:29:19AM +0100, Emil Velikov wrote:
>> >> From: Emil Velikov 
> ...
>>
>> > From checking header files it does seem to me that the layout of struct
>> > wl_egl_window is indeed opaque to the outside world, but all the same 
>> > wouldn't
>> > it be better to be able to see what version of the library one had 
>> > installed?
>> >
>> Why? Can you please elaborate.
>>
> I searched /usr/include for includes of wayland-egl-priv.h. There weren't any
> (as one would hope, since Mesa doesn't expose it).
>
> I then searched /usr/include for declarations of struct wl_surface:
>
> qt5/QtWaylandCompositor/5.7.1/QtWaylandCompositor/private/wayland-wayland-server-protocol.h
> 90 struct wl_subsurface;
> 91 struct wl_surface;
> 92 struct wl_touch;
>
> It's declared but not defined. Code can only work with pointers to the struct
> and not its elements, which is what I meant by saying it was "opaque".
>
True, wl_egl_window is opaque. Then again, I'm not sure why would you
bump the version "just in case".
Note that bumping ABI/DSO version without a genuine reason is not a good idea.

As mentioned in the previous email - the struct is internal backend
information. One that can be detected by the backend at runtime.

Thanks
Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Emil Velikov]

On 19 September 2017 at 05:34, Duncan Roe  wrote:
> Hi Emil,
>
> On Mon, Sep 18, 2017 at 02:18:42PM +0100, Emil Velikov wrote:
>> Hi Duncan,
>>
>> On 17 September 2017 at 09:04, Duncan Roe  wrote:
>> > On Fri, Sep 15, 2017 at 11:29:19AM +0100, Emil Velikov wrote:
>> >> From: Emil Velikov 
> ...
>>
>> > I've made the updates locally, and see that it still builds
>> > libwayland-egl.so.1.0.0. Shouldn't there be a version bump?
>> >
>> There is no difference in the sources or binary. Thus a version bump
>> is a very bad idea.
>>
> Perhaps I didn't explain properly:
>
> I built wayland-egl from code in Mesa-17.0.4. That built
> libwayland-egl.so.1.0.0. I rebuilt wayland-egl from your code in Mesa-17.1.9.
> That also built libwayland-egl.so.1.0.0.
>
> Between the 2 revisions, the layout of struct wl_egl_window is changed to have
> const intptr_t version inserted as its first element with struct wl_surface
> *surface moved from first element to last.
>
> The new .so builds to be the same size as the old one but cmp -l shows 1395
> differing bytes,
>
That's correct, yet the change is orthogonal to the import.

Even then this is a backend change, while a version bump indicates
frontend changes.
On the backend side one can easily detect the version at runtime see [1].

HTH
Emil

[1] https://cgit.freedesktop.org/mesa/mesa/log/?qt=author=miguel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Emil Velikov]

Hi Duncan,

On 17 September 2017 at 09:04, Duncan Roe  wrote:
> On Fri, Sep 15, 2017 at 11:29:19AM +0100, Emil Velikov wrote:
>> From: Emil Velikov 
> ...
>>
>> Takanari, Duncan,
>> AFAICT the Renesas devices that you're using have an ImgTec IP inside
>> them. Please check with Renesas and/or ImgTec, as one can build their
>> driver as a Mesa component.
>>
>> As such you should be able to utilise the EGL/Wayland, EGL/Android and
>> other features without having to write/maintain wrapper shims.
>
> Hi Emil,
>
> The only reason I'm involved is that I package wayland-egl for SBo.
>
> SlackBuilds.org (SBo) provide additional packages ready to install on a system
> running the Slackware Linux distribution.
>
> I first packaged wayland-egl when it showed up as a dependency of VLC built 
> with
> all optional dependencies (and optional dependencies of dependencies ...). As
> such, I have no idea when or even if wayland-egl gets used or how I would set
> about testing whether a new release has an adverse effect on VLC.
>
Right, I've saw that and was worried how you tested the codepaths.
AFAICT there's no backend driver or Mesa in SBo.
That's orthogonal topic, so let's not go there.

> Your email contains the checked-in Mesa code as of rev 17.1.9 / 17.2.0.
>
The commit mentions the exact commit SHA1, so Mesa versions do not
anything extra ;-)

> If I update my wayland-egl branch to this code and make a new Slackbuild, is
> that likely to break anything?
>
Not much more than it already is ;-)

> I've made the updates locally, and see that it still builds
> libwayland-egl.so.1.0.0. Shouldn't there be a version bump?
>
There is no difference in the sources or binary. Thus a version bump
is a very bad idea.

There's an open question (+ lots of data) on the version as listed in
the .pc file. See patch 2/9 [1]

> From checking header files it does seem to me that the layout of struct
> wl_egl_window is indeed opaque to the outside world, but all the same wouldn't
> it be better to be able to see what version of the library one had installed?
>
Why? Can you please elaborate.

Thanks
Emil

> Cheers ... Duncan.

[1] 
https://lists.freedesktop.org/archives/wayland-devel/2017-September/035021.html
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [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  wrote:
> Hi Emil,
>
> On 15 September 2017 at 15:54, Emil Velikov  wrote:
>> On 15 September 2017 at 14:50, Daniel Stone  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 
>>
>> 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
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org

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

[Daniel Stone]

Hi Emil,

On 15 September 2017 at 15:54, Emil Velikov  wrote:
> On 15 September 2017 at 14:50, Daniel Stone  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 
>
> 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
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [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  wrote:
> Hi Emil,
>
> On 15 September 2017 at 11:29, Emil Velikov  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 
>
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
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

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

[Daniel Stone]

Hi Emil,

On 15 September 2017 at 11:29, Emil Velikov  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

Apart from the below, this series, when squashed into a single patch
for import, is:
Reviewed-by: Daniel Stone 

I realise this is a straight copy/import, so sorry to pick on these, but:

> +#include  // offsetof
> +#include   // printf
> +
> +#include "wayland-egl-priv.h" // Current struct wl_egl_window implementation

C++ comments :(

> +/* GCC visibility */
> +#if defined(__GNUC__)
> +#define WL_EGL_EXPORT __attribute__ ((visibility("default")))
> +#else
> +#define WL_EGL_EXPORT
> +#endif

This bit can be deleted.

wayland-egl-priv.h could be renamed wayland-egl-backend.h, since it's
by definition no longer private. I'm also not a fan of importing dead
code and only wiring it up later.

> +#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?

wayland-egl-priv.h could be renamed wayland-egl-backend.h, since it's
by definition no longer private. I'm also not a fan of importing dead
code and only wiring it up later.

> diff --git a/egl/wayland-egl-symbols-check b/egl/wayland-egl-symbols-check
> new file mode 100755
> index 000..e7105ea
> --- /dev/null
> +++ b/egl/wayland-egl-symbols-check
> @@ -0,0 +1,16 @@
> +#!/bin/sh
> +
> +FUNCS=$(nm -D --defined-only ${1-.libs/libwayland-egl.so} | grep -o "T .*" | 
> cut -c 3- | while read func; do

Please take the input library name from the command line, so
build-system details are confined to just the build system.

> +WL_EGL_EXPORT void
> +wl_egl_window_resize(struct wl_egl_window *egl_window,

Reuse WL_EXPORT.

> +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?

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel