Re: [waffle] [PATCH v3 4/4] wayland: resolve wayland-client symbols at runtime (via libdl)
On 26 March 2015 at 16:20, Chad Versace wrote: > Quoting Emil Velikov (2015-03-18 13:03:08) >> On 17 March 2015 at 19:54, Chad Versace wrote: >> > Hey Emil, >> > >> > The patches look good to me, though I haven't tested them yet. >> > I'm going to run the patches through a Piglit run and, if I >> > find no regressions, I'll push them. >> Great thanks. >> >> Looking at the next times on the top of my waffle todo: >> - Rework the waffle test - fold the test init and teardown within the >> waffle test hooks. >> Will allow us to use the waffle_init() waffle_teardown(), effectively >> "reloading" waffle for each test. We can even intentionally randomise >> the tests across threads, to ensure that the going from platfrom_foo >> to platform_bar does not cause problems. >> >> It will be quite some work so if I'd like your opinion on it before I >> crack on it. > > Do you mean calling waffle_init() and waffle_teardown() before and after > tests/functional/gl_basic.c:gl_basic_draw(), or something similar to > that? If so, that sounds like a good idea, especially the optional > randomization of tests across threads. That should provide some good > test coverage of Waffle handling (serially, though not concurrently) > multiple platforms in one process. > This is precisely what I was thinking/talking about. >> - Remove the xcb + libX11 link time dependency (rework the platforms >> to use libX11 alone). >> I'm leaning towards the latter, as it would simplify things. Although >> I'm a but uncertain if you chose the mixed design due to some >> technical reason ? > > Technical reasons did drive my decision. > > Originally, I tried to write Waffle's X11 code using only Xlib. After > trying for several days (I am no X expert) I gave up and moved to xcb, > which I got working after just a few hours. (I attribute my failure not > only to Xlib having an awful API but also to my lack of familiarity with > it). > > I was unable to go purely with xcb, though, because the native_display > parameter of eglGetDisplay must be an Xlib Display. Ironically, Mesa's > EGL makes exactly one Xlib call, XGetXCBConnection, and uses only xcb > afterwards. > > I think we have some strong precedent for avoiding Xlib: > - Mesa's EGL decided to use only xcb and not Xlib. > - Mesa's GLX DRI2 implementation is implemented with a mixture of > Xlib calls and rolling X protocol by hand. When keithp later wrote > Mesa's GLX DRI3 code, he decided to do it in pure xcb. > - If we have any questions or problems with xcb, the library's > source code is very readable. > > In summary, my technical reasons were: > - XCB's API is easier to understand (for me, at least). > - The XCB API is actively extended to support new X11 protocol. Xlib > is not, as far as I can tell. > - XCB's source is easy to understand. > - Other projects avoid Xlib too for similar reasons. > Before anything else, just want to say that I'm not against xcb in any way, but I was hoping to get a clear (either Xlib or xcb only) implementation for waffle. Considering we need Xlib to interact with libEGL (as pointed above) was hoping that we can go with it, and opt for a xcb only solution when possible. Seems like that won't be the case :-'( My main concern is that despite xcb's introduction quite some years ago, there are still places where it lacks functionality wrt Xlib. Mainly around interacting with glx. On the topic of that it's easier to understand the following reply from Keith [1] left me with the opposite impression. Yet It could be that the Xlib code is even harder/messier. And a final nail in the coffin (sort of speak) seems that the Valve guys have noticed some issues with the xcb-fication of glx/dri2 and are carrying patches that revert it. All that moaning done, I will shut up on the topic, and take a closer look into reworking the dependencies. Hopefully it will end up cleaner than expected. Cheers, Emil [1] "The problem is that reviewing this patch is *really hard*. The last time, I think I spent a solid couple of days thinking about this and making sure I'd caught all of the cases..." http://lists.x.org/archives/xorg-devel/2014-September/044063.html ___ waffle mailing list waffle@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH v3 4/4] wayland: resolve wayland-client symbols at runtime (via libdl)
Quoting Emil Velikov (2015-03-18 13:03:08) > On 17 March 2015 at 19:54, Chad Versace wrote: > > Hey Emil, > > > > The patches look good to me, though I haven't tested them yet. > > I'm going to run the patches through a Piglit run and, if I > > find no regressions, I'll push them. > Great thanks. > > Looking at the next times on the top of my waffle todo: > - Rework the waffle test - fold the test init and teardown within the > waffle test hooks. > Will allow us to use the waffle_init() waffle_teardown(), effectively > "reloading" waffle for each test. We can even intentionally randomise > the tests across threads, to ensure that the going from platfrom_foo > to platform_bar does not cause problems. > > It will be quite some work so if I'd like your opinion on it before I > crack on it. Do you mean calling waffle_init() and waffle_teardown() before and after tests/functional/gl_basic.c:gl_basic_draw(), or something similar to that? If so, that sounds like a good idea, especially the optional randomization of tests across threads. That should provide some good test coverage of Waffle handling (serially, though not concurrently) multiple platforms in one process. > - Remove the xcb + libX11 link time dependency (rework the platforms > to use libX11 alone). > I'm leaning towards the latter, as it would simplify things. Although > I'm a but uncertain if you chose the mixed design due to some > technical reason ? Technical reasons did drive my decision. Originally, I tried to write Waffle's X11 code using only Xlib. After trying for several days (I am no X expert) I gave up and moved to xcb, which I got working after just a few hours. (I attribute my failure not only to Xlib having an awful API but also to my lack of familiarity with it). I was unable to go purely with xcb, though, because the native_display parameter of eglGetDisplay must be an Xlib Display. Ironically, Mesa's EGL makes exactly one Xlib call, XGetXCBConnection, and uses only xcb afterwards. I think we have some strong precedent for avoiding Xlib: - Mesa's EGL decided to use only xcb and not Xlib. - Mesa's GLX DRI2 implementation is implemented with a mixture of Xlib calls and rolling X protocol by hand. When keithp later wrote Mesa's GLX DRI3 code, he decided to do it in pure xcb. - If we have any questions or problems with xcb, the library's source code is very readable. In summary, my technical reasons were: - XCB's API is easier to understand (for me, at least). - The XCB API is actively extended to support new X11 protocol. Xlib is not, as far as I can tell. - XCB's source is easy to understand. - Other projects avoid Xlib too for similar reasons. ___ waffle mailing list waffle@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH v3 4/4] wayland: resolve wayland-client symbols at runtime (via libdl)
On 17 March 2015 at 19:54, Chad Versace wrote: > Hey Emil, > > The patches look good to me, though I haven't tested them yet. > I'm going to run the patches through a Piglit run and, if I > find no regressions, I'll push them. Great thanks. Looking at the next times on the top of my waffle todo: - Rework the waffle test - fold the test init and teardown within the waffle test hooks. Will allow us to use the waffle_init() waffle_teardown(), effectively "reloading" waffle for each test. We can even intentionally randomise the tests across threads, to ensure that the going from platfrom_foo to platform_bar does not cause problems. It will be quite some work so if I'd like your opinion on it before I crack on it. - Remove the xcb + libX11 link time dependency (rework the platforms to use libX11 alone). I'm leaning towards the latter, as it would simplify things. Although I'm a but uncertain if you chose the mixed design due to some technical reason ? Cheers, Emil ___ waffle mailing list waffle@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/waffle
Re: [waffle] [PATCH v3 4/4] wayland: resolve wayland-client symbols at runtime (via libdl)
Hey Emil, The patches look good to me, though I haven't tested them yet. I'm going to run the patches through a Piglit run and, if I find no regressions, I'll push them. ___ waffle mailing list waffle@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/waffle
[waffle] [PATCH v3 4/4] wayland: resolve wayland-client symbols at runtime (via libdl)
By doing so one can use waffle's other platforms, without having
wayland installed/available on their system.
This patch is based on a similar work by the SDL devs.
v2:
- Remove implementation details from commit message.
- Follow SDL's approach.
v3:
- Include wayland-client.h only when needed (in the C files).
Signed-off-by: Emil Velikov
---
.../Modules/WafflePrintConfigurationSummary.cmake | 1 -
src/waffle/CMakeLists.txt | 6 +-
src/waffle/wayland/wayland_display.c | 2 +
src/waffle/wayland/wayland_platform.c | 6 ++
src/waffle/wayland/wayland_window.c| 2 +
src/waffle/wayland/wayland_wrapper.c | 115 +
src/waffle/wayland/wayland_wrapper.h | 94 +
7 files changed, 220 insertions(+), 6 deletions(-)
create mode 100644 src/waffle/wayland/wayland_wrapper.c
create mode 100644 src/waffle/wayland/wayland_wrapper.h
diff --git a/cmake/Modules/WafflePrintConfigurationSummary.cmake
b/cmake/Modules/WafflePrintConfigurationSummary.cmake
index 27aa8dc..1199ea3 100644
--- a/cmake/Modules/WafflePrintConfigurationSummary.cmake
+++ b/cmake/Modules/WafflePrintConfigurationSummary.cmake
@@ -60,7 +60,6 @@ if(waffle_has_glx)
endif()
if(waffle_has_wayland)
message("wayland-client_INCLUDE_DIRS: ${wayland-client_INCLUDE_DIRS}")
-message("wayland-client_LDFLAGS: ${wayland-client_LDFLAGS}")
message("wayland-egl_INCLUDE_DIRS:${wayland-egl_INCLUDE_DIRS}")
endif()
if(waffle_has_x11)
diff --git a/src/waffle/CMakeLists.txt b/src/waffle/CMakeLists.txt
index 4a473be..4f1d5c7 100644
--- a/src/waffle/CMakeLists.txt
+++ b/src/waffle/CMakeLists.txt
@@ -41,11 +41,6 @@ list(APPEND waffle_libdeps
)
if(waffle_on_linux)
-if(waffle_has_wayland)
-list(APPEND waffle_libdeps
-${wayland-client_LDFLAGS}
-)
-endif()
if(waffle_has_x11)
list(APPEND waffle_libdeps
${x11-xcb_LDFLAGS}
@@ -137,6 +132,7 @@ if(waffle_has_wayland)
wayland/wayland_display.c
wayland/wayland_platform.c
wayland/wayland_window.c
+wayland/wayland_wrapper.c
)
endif()
diff --git a/src/waffle/wayland/wayland_display.c
b/src/waffle/wayland/wayland_display.c
index e7c6e94..e2d59a8 100644
--- a/src/waffle/wayland/wayland_display.c
+++ b/src/waffle/wayland/wayland_display.c
@@ -28,6 +28,8 @@
#include
#include
+// The wrapper must be included before wayland-client.h
+#include "wayland_wrapper.h"
#include
#undef container_of
diff --git a/src/waffle/wayland/wayland_platform.c
b/src/waffle/wayland/wayland_platform.c
index 94c0010..2746ca1 100644
--- a/src/waffle/wayland/wayland_platform.c
+++ b/src/waffle/wayland/wayland_platform.c
@@ -43,6 +43,7 @@
#include "wayland_display.h"
#include "wayland_platform.h"
#include "wayland_window.h"
+#include "wayland_wrapper.h"
static const char *libwl_egl_filename = "libwayland-egl.so.1";
@@ -73,6 +74,7 @@ wayland_platform_destroy(struct wcore_platform *wc_self)
}
}
+ok &= wayland_wrapper_teardown();
ok &= wegl_platform_teardown(&self->wegl);
free(self);
return ok;
@@ -92,6 +94,10 @@ wayland_platform_create(void)
if (!ok)
goto error;
+ok = wayland_wrapper_init();
+if (!ok)
+goto error;
+
self->dl_wl_egl = dlopen(libwl_egl_filename, RTLD_LAZY | RTLD_LOCAL);
if (!self->dl_wl_egl) {
wcore_errorf(WAFFLE_ERROR_FATAL,
diff --git a/src/waffle/wayland/wayland_window.c
b/src/waffle/wayland/wayland_window.c
index 2cdf8e6..b5eeaa3 100644
--- a/src/waffle/wayland/wayland_window.c
+++ b/src/waffle/wayland/wayland_window.c
@@ -28,6 +28,8 @@
#include
#include
+// The wrapper must be included before wayland-(client|egl).h
+#include "wayland_wrapper.h"
#include
#undef container_of
diff --git a/src/waffle/wayland/wayland_wrapper.c
b/src/waffle/wayland/wayland_wrapper.c
new file mode 100644
index 000..6ffd5a9
--- /dev/null
+++ b/src/waffle/wayland/wayland_wrapper.c
@@ -0,0 +1,115 @@
+// Copyright 2015 Emil Velikov
+//
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are met:
+//
+// - Redistributions of source code must retain the above copyright notice,
this
+// list of conditions and the following disclaimer.
+//
+// - Redistributions in binary form must reproduce the above copyright notice,
+// this list of conditions and the following disclaimer in the documentation
+// and/or other materials provided with the distribution.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+// AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+// IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
ARE
+// DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOL
