[PATCH] compositor: Add support for external back-ends
Krzysztof Konopko
kris at youview.com
Fri Sep 9 07:17:52 UTC 2016
On Thu, 2016-09-08 at 18:01 +0200, Quentin Glidic wrote:
> On 08/09/2016 16:45, Krzysztof Konopko wrote:
> > This commit adds ability to load external back-end implementations which is
> > required if they are not (and can not be) maintained within 'weston' tree.
> >
> > libweston-backend is introduced to provide common back-end functionality to
> > external implementations. Also few required functions are exported as
> > well and libweston-backend.so library is introduced.
> >
> > Signed-off-by: Krzysztof Konopko <kris at youview.com>
>
> Hi,
>
First of all, thanks for taking a look. Much appreciated.
> I think this is not a bad idea to have support for more backends.
> However, it must be clear which type of backend is needed, and why it
> cannot be in Weston repository. (It is not a requirement for the commit
> message, but as an informal addition for ML archiving.)
>
Yeah, I sent a cover letter but the enterprise-y SMTP server I use did
something weird while `git send-email` said it was OK. Finally it was
processed after some delay and it's on the ML now [1].
> A second point against this patch is that backend have a big rework
> coming soon, with Armin’s work during GSoC. We are waiting for the next
> release cycle to start merging his patches, as we are currently in
> Release Candidate state.
>
Ah, excellent! I'll hold off then and see what happens. I'm expecting
a lot of changes in Weston. Trying to keep track of it and I know I'll
have few more iterations with the back-ends I'm working with to adapt :)
> This leads to the third point: this patch seems to have been written
> against the pre-libweston code. You are re-introducing "struct
> weston_config" in the backend init function, but it was removed on
> purpose. If you want such custom backends, use the exact same init
> function (which I hope we will rename on the next cycle, to properly
> namespace it).
>
I didn't touch any of the existing APIs. That was my purpose to only
make additive changes so `weston_config` appeared one something which I
proposed as an alternative "init" function:
int
backend_init_ext(struct weston_compositor *compositor,
int *argc, char *argv[],
struct weston_config * config);
I needed extra arguments to let the external back-end do it own
(specific) configuration parsing. Maybe there's another way.
As explained in [1], ideally all back-ends (officially supported and
external) should use the same API which would need to be changed to move
back-end specific config parsing and initialisation into back-end
library (out of generic compositor library/executable).
But like you say, there are some changes on the horizon in this area in
the forthcoming cycle so something I'll need to revisit. Will keep an
eye on the changes here :)
> Now, a few inline comments for your next version (which should wait for
> Armin’s patches, or be built on top of them).
>
> > ---
> > Makefile.am | 63 +++++++++++++++++++++++++----------------------
> > compositor/main.c | 7 +++---
> > libweston/compositor.c | 16 ++++++++++++
> > libweston/compositor.h | 10 ++++++++
> > libweston/libinput-seat.c | 10 ++++----
> > libweston/libweston.pc.in | 2 +-
> > shared/config-parser.c | 2 +-
> > shared/option-parser.c | 4 ++-
> > 8 files changed, 73 insertions(+), 41 deletions(-)
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 1e63a58..d65646e 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -205,30 +205,6 @@ weston_SOURCES = \
> > compositor/main.c : $(top_builddir)/libweston/git-version.h
> > libweston/compositor.c : $(top_builddir)/libweston/git-version.h
> >
> > -noinst_LTLIBRARIES += \
> > - libsession-helper.la
> > -
> > -libsession_helper_la_SOURCES = \
> > - libweston/launcher-util.c \
> > - libweston/launcher-util.h \
> > - libweston/launcher-impl.h \
> > - libweston/weston-launch.h \
> > - libweston/launcher-weston-launch.c \
> > - libweston/launcher-direct.c
> > -libsession_helper_la_CFLAGS = $(AM_CFLAGS) $(LIBDRM_CFLAGS) $(PIXMAN_CFLAGS) $(COMPOSITOR_CFLAGS)
> > -libsession_helper_la_LIBADD = $(LIBDRM_LIBS)
> > -
> > -if ENABLE_DBUS
> > -if HAVE_SYSTEMD_LOGIN
> > -libsession_helper_la_SOURCES += \
> > - libweston/dbus.h \
> > - libweston/dbus.c \
> > - libweston/launcher-logind.c
> > -libsession_helper_la_CFLAGS += $(SYSTEMD_LOGIN_CFLAGS) $(DBUS_CFLAGS)
> > -libsession_helper_la_LIBADD += $(SYSTEMD_LOGIN_LIBS) $(DBUS_LIBS)
> > -endif
> > -endif
> > -
> > if HAVE_GIT_REPO
> > libweston/git-version.h : $(top_srcdir)/.git/logs/HEAD
> > $(AM_V_GEN)echo "#define BUILD_ID \"$(shell git --git-dir=$(top_srcdir)/.git describe --always --dirty) $(shell git --git-dir=$(top_srcdir)/.git log -1 --format='%s (%ci)')\"" > $@
> > @@ -285,8 +261,11 @@ libwestoninclude_HEADERS = \
> > libweston/compositor-rdp.h \
> > libweston/compositor-wayland.h \
> > libweston/compositor-x11.h \
> > + libweston/launcher-util.h \
> > + libweston/libinput-seat.h \
> > libweston/plugin-registry.h \
> > libweston/timeline-object.h \
> > + protocol/presentation-time-server-protocol.h \
>
> Why this header? It should not be installed. And it should be safe to
> use any version of this header with any version of libweston, thanks to
> libwayland design.
>
Yup, something I'll need to revisit. Probably I didn't understand well
enough the role of this header. I needed it to build rpi-backend out of
tree (as a PoC for my vendor specific back-end).
>
> > shared/matrix.h \
> > shared/config-parser.h \
> > shared/zalloc.h
> > @@ -337,7 +316,12 @@ x11_backend_la_SOURCES = \
> > shared/helpers.h
> > endif
> >
> > -INPUT_BACKEND_LIBS = $(LIBINPUT_BACKEND_LIBS)
> > +lib_LTLIBRARIES += \
> > + libweston-backend.la
> > +
> > +libweston_backend_la_CFLAGS = $(AM_CFLAGS) $(LIBDRM_CFLAGS) $(PIXMAN_CFLAGS) $(COMPOSITOR_CFLAGS)
> > +libweston_backend_la_LIBADD = $(LIBDRM_LIBS) $(LIBINPUT_BACKEND_LIBS) libshared.la
> > +
> > INPUT_BACKEND_SOURCES = \
> > libweston/libinput-seat.c \
> > libweston/libinput-seat.h \
> > @@ -345,16 +329,38 @@ INPUT_BACKEND_SOURCES = \
> > libweston/libinput-device.h \
> > shared/helpers.h
> >
> > +SESSION_HELPER_SOURCES = \
> > + libweston/launcher-util.c \
> > + libweston/launcher-util.h \
> > + libweston/launcher-impl.h \
> > + libweston/weston-launch.h \
> > + libweston/launcher-weston-launch.c \
> > + libweston/launcher-direct.c
> > +
> > +if ENABLE_DBUS
> > +if HAVE_SYSTEMD_LOGIN
> > +SESSION_HELPER_SOURCES += \
> > + libweston/dbus.h \
> > + libweston/dbus.c \
> > + libweston/launcher-logind.c
> > +libweston_backend_la_CFLAGS += $(SYSTEMD_LOGIN_CFLAGS) $(DBUS_CFLAGS)
> > +libweston_backend_la_LIBADD += $(SYSTEMD_LOGIN_LIBS) $(DBUS_LIBS)
> > +endif
> > +endif
> > +
> > +libweston_backend_la_SOURCES = \
> > + $(INPUT_BACKEND_SOURCES) \
> > + $(SESSION_HELPER_SOURCES)
> > +
>
> Since you move everything in this library, you should remove both
> INPUT_BACKEND_SOURCES and SESSION_HELPER_SOURCES entirely.
Yes.
> > if ENABLE_DRM_COMPOSITOR
> > libweston_module_LTLIBRARIES += drm-backend.la
> > drm_backend_la_LDFLAGS = -module -avoid-version
> > drm_backend_la_LIBADD = \
> > $(COMPOSITOR_LIBS) \
> > $(DRM_COMPOSITOR_LIBS) \
> > - $(INPUT_BACKEND_LIBS) \
> > libshared.la \
> > $(CLOCK_GETTIME_LIBS) \
> > - libsession-helper.la
> > + libweston-backend.la
> > drm_backend_la_CFLAGS = \
> > $(COMPOSITOR_CFLAGS) \
> > $(EGL_CFLAGS) \
> > @@ -416,8 +422,7 @@ fbdev_backend_la_LDFLAGS = -module -avoid-version
> > fbdev_backend_la_LIBADD = \
> > $(COMPOSITOR_LIBS) \
> > $(FBDEV_COMPOSITOR_LIBS) \
> > - $(INPUT_BACKEND_LIBS) \
> > - libsession-helper.la \
> > + libweston-backend.la \
> > libshared.la
> > fbdev_backend_la_CFLAGS = \
> > $(COMPOSITOR_CFLAGS) \
> > diff --git a/compositor/main.c b/compositor/main.c
> > index 0e5af5b..a6ab81e 100644
> > --- a/compositor/main.c
> > +++ b/compositor/main.c
> > @@ -1525,7 +1525,6 @@ load_wayland_backend(struct weston_compositor *c,
> > return ret;
> > }
> >
> > -
>
> Unrelated change.
>
Oops, good spot.
>
> > static int
> > load_backend(struct weston_compositor *compositor, const char *backend,
> > int *argc, char **argv, struct weston_config *config)
> > @@ -1542,9 +1541,9 @@ load_backend(struct weston_compositor *compositor, const char *backend,
> > return load_x11_backend(compositor, argc, argv, config);
> > else if (strstr(backend, "wayland-backend.so"))
> > return load_wayland_backend(compositor, argc, argv, config);
> > -
> > - weston_log("Error: unknown backend \"%s\"\n", backend);
> > - return -1;
> > + else
> > + return weston_compositor_load_backend_ext(compositor, backend,
> > + argc, argv, config);
>
> I would not allow Weston to load such a backend. But that means no
> possible testing…
Yeah, so this introduces an alternative API for loading external
back-end and, like you say, testing it becomes a problem. Ideally I
think all back-ends should use the same (unified) API which would be
tested with back-ends hosted within 'weston' project. I'll wait for the
next development cycle to see what changes land in this area and will
revisit it.
> I’ll let the others comment on that, but I would rather not allow Weston
> to load arbitrary backends. It’s up to each compositor to have its own
> whitelist.
> (Yes, Weston can load arbitrary modules, but that’s clear in the
> documentation, at least.)
>
I think the decision maker here is the system administrator/integrator
who decides which back-end is loaded through the config file or at build
time (`WESTON_NATIVE_BACKEND`). Need to have the "privilege" (at
integration time or run-time) to put the back-end library in certain
directory (eg. `/usr/lib/libweston-1`) and to change weston launch
configuration.
Maybe it's not obvious from the patch, but external back-end libraries
would only be loaded from the same directory where official back-ends
are installed.
>
> > }
> >
> > static char *
> > diff --git a/libweston/compositor.c b/libweston/compositor.c
> > index 47907bb..685ebbb 100644
> > --- a/libweston/compositor.c
> > +++ b/libweston/compositor.c
> > @@ -5104,6 +5104,22 @@ weston_compositor_load_backend(struct weston_compositor *compositor,
> > }
> >
> > WL_EXPORT int
> > +weston_compositor_load_backend_ext(struct weston_compositor *compositor,
> > + char *name, int *argc, char **argv,
> > + struct weston_config *wc)
> > +{
> > + int (*backend_init_ext)(struct weston_compositor *c,
> > + int *argc, char **argv,
> > + struct weston_config *wc);
> > +
> > + backend_init_ext = weston_load_module(name, "backend_init_ext");
> > + if (!backend_init_ext)
> > + return -1;
> > +
> > + return backend_init_ext(compositor, argc, argv, wc);
> > +}
> > +
>
> As I said, re-use the same init function, without argc/argc/config.
>
I needed argc/argc/config to let external back-end do their own config
parsing. But will revisit after the planned changes in this area.
>
> > +WL_EXPORT int
> > weston_compositor_load_xwayland(struct weston_compositor *compositor)
> > {
> > int (*module_init)(struct weston_compositor *ec,
> > diff --git a/libweston/compositor.h b/libweston/compositor.h
> > index 16db03b..1d94eb3 100644
> > --- a/libweston/compositor.h
> > +++ b/libweston/compositor.h
> > @@ -1568,6 +1568,12 @@ int
> > weston_compositor_load_backend(struct weston_compositor *compositor,
> > enum weston_compositor_backend backend,
> > struct weston_backend_config *config_base);
> > +
> > +int
> > +weston_compositor_load_backend_ext(struct weston_compositor *compositor,
> > + char *name, int *argc, char **argv,
> > + struct weston_config *wc);
> > +
> > void
> > weston_compositor_exit(struct weston_compositor *ec);
> > void *
> > @@ -1765,6 +1771,10 @@ int
> > backend_init(struct weston_compositor *c,
> > struct weston_backend_config *config_base);
> > int
> > +backend_init_ext(struct weston_compositor *compositor, int *argc, char *argv[],
> > + struct weston_config * config);
> > +
> > +int
> > module_init(struct weston_compositor *compositor,
> > int *argc, char *argv[]);
> >
> > diff --git a/libweston/libinput-seat.c b/libweston/libinput-seat.c
> > index 78a5fc4..97ca4e2 100644
> > --- a/libweston/libinput-seat.c
> > +++ b/libweston/libinput-seat.c
> > @@ -128,7 +128,7 @@ udev_seat_remove_devices(struct udev_seat *seat)
> > }
> > }
> >
> > -void
> > +WL_EXPORT void
> > udev_input_disable(struct udev_input *input)
> > {
> > if (input->suspended)
> > @@ -224,7 +224,7 @@ const struct libinput_interface libinput_interface = {
> > close_restricted,
> > };
> >
> > -int
> > +WL_EXPORT int
> > udev_input_enable(struct udev_input *input)
> > {
> > struct wl_event_loop *loop;
> > @@ -281,7 +281,7 @@ libinput_log_func(struct libinput *libinput,
> > weston_vlog(format, args);
> > }
> >
> > -int
> > +WL_EXPORT int
> > udev_input_init(struct udev_input *input, struct weston_compositor *c,
> > struct udev *udev, const char *seat_id,
> > udev_configure_device_t configure_device)
> > @@ -326,7 +326,7 @@ udev_input_init(struct udev_input *input, struct weston_compositor *c,
> > return udev_input_enable(input);
> > }
> >
> > -void
> > +WL_EXPORT void
> > udev_input_destroy(struct udev_input *input)
> > {
> > struct udev_seat *seat, *next;
> > @@ -403,7 +403,7 @@ udev_seat_destroy(struct udev_seat *seat)
> > free(seat);
> > }
> >
> > -struct udev_seat *
> > +WL_EXPORT struct udev_seat *
> > udev_seat_get_named(struct udev_input *input, const char *seat_name)
> > {
> > struct udev_seat *seat;
>
> If we make them public, we should properly namespace them too.
>
OK, will check that.
>
> > diff --git a/libweston/libweston.pc.in b/libweston/libweston.pc.in
> > index 2391003..30b411a 100644
> > --- a/libweston/libweston.pc.in
> > +++ b/libweston/libweston.pc.in
> > @@ -8,4 +8,4 @@ Description: Header files for libweston compositors development
> > Version: @WESTON_VERSION@
> > Requires.private: wayland-server pixman-1 xkbcommon
> > Cflags: -I${includedir}/libweston- at LIBWESTON_MAJOR@
> > -Libs: -L${libdir} -lweston- at LIBWESTON_MAJOR@
> > +Libs: -L${libdir} -lweston- at LIBWESTON_MAJOR@ -lweston-backend
>
> No. It is a different library, so make it have its own .pc file.
>
Yup, that's right. I admit I was a bit hesitant here as I wanted to
keep the patch as small as possible. But I totally agree, this deserves
own .pc.
>
> > diff --git a/shared/config-parser.c b/shared/config-parser.c
> > index e2b5fa2..e2b90aa 100644
> > --- a/shared/config-parser.c
> > +++ b/shared/config-parser.c
> > @@ -403,7 +403,7 @@ section_add_entry(struct weston_config_section *section,
> > return entry;
> > }
> >
> > -struct weston_config *
> > +WL_EXPORT struct weston_config *
> > weston_config_parse(const char *name)
> > {
> > FILE *fp;
> > diff --git a/shared/option-parser.c b/shared/option-parser.c
> > index eee7546..08c29e7 100644
> > --- a/shared/option-parser.c
> > +++ b/shared/option-parser.c
> > @@ -32,6 +32,8 @@
> > #include <assert.h>
> > #include <errno.h>
> >
> > +#include <wayland-util.h>
> > +
> > #include "config-parser.h"
> > #include "string-helpers.h"
> >
> > @@ -135,7 +137,7 @@ short_option_with_arg(const struct weston_option *options, int count, char *arg,
> > return 0;
> > }
> >
> > -int
> > +WL_EXPORT int
> > parse_options(const struct weston_option *options,
> > int count, int *argc, char *argv[])
> > {
>
> These two files changes are then not needed.
>
OK, will revisit this in the next development cycle.
Again, thanks a lot for taking a look and for all the comments. Will
watch this space and hopefully come back with something improved.
Cheers,
Kris
[1]
https://lists.freedesktop.org/archives/wayland-devel/2016-September/030998.html
This transmission contains information that may be confidential and contain personal views which are not necessarily those of YouView TV Ltd. YouView TV Ltd (Co No:7308805) is a limited liability company registered in England and Wales with its registered address at YouView TV Ltd, 3rd Floor, 10 Lower Thames Street, London, EC3R 6YT. For details see our web site at http://www.youview.com
More information about the wayland-devel
mailing list