[PATCH weston 1/3] compositor: prep for migration to new config system
Bryce Harrington
bryce at osg.samsung.com
Mon Aug 24 18:50:42 PDT 2015
On Thu, Aug 13, 2015 at 12:30:01PM +0200, Giulio Camuffo wrote:
> From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>
> Add new configuration argument to the backend_init() function, which
> will replace the current argc, argv, and config arguments.
>
> Next, each backend is converted individually, after which the unused
> arguments will be removed.
>
> Also add the skeleton to main.c for picking the right configuration path
> by the backend.
>
> Fix an error path that forgot to destroy weston_compositor.
This will be a nice cleanup. I like that we're being deliberate in
setting things up for a series of refactorings. That said, this
description seems to suggest several distinct changes are being done;
maybe that's ok and appropriate, but I'm finding it makes the review a
touch more involved.
The bit that fixes the weston_compositor destruction sounds like it may
be landable on its own, if it can be teased out without too much effort.
> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> ---
> src/compositor-drm.c | 3 +-
> src/compositor-fbdev.c | 3 +-
> src/compositor-headless.c | 3 +-
> src/compositor-rdp.c | 3 +-
> src/compositor-rpi.c | 3 +-
> src/compositor-wayland.c | 3 +-
> src/compositor-x11.c | 3 +-
> src/compositor.h | 21 ++++++++++++--
> src/main.c | 71 ++++++++++++++++++++++++++++++++++++++++-------
> 9 files changed, 93 insertions(+), 20 deletions(-)
>
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index 585169e..ce95d05 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -3182,7 +3182,8 @@ err_base:
>
> WL_EXPORT int
> backend_init(struct weston_compositor *compositor, int *argc, char *argv[],
> - struct weston_config *config)
> + struct weston_config *config,
> + struct weston_backend_config *config_base)
> {
> struct drm_backend *b;
> struct drm_parameters param = { 0, };
> diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
> index 051a381..1431621 100644
> --- a/src/compositor-fbdev.c
> +++ b/src/compositor-fbdev.c
> @@ -903,7 +903,8 @@ out_compositor:
>
> WL_EXPORT int
> backend_init(struct weston_compositor *compositor, int *argc, char *argv[],
> - struct weston_config *config)
> + struct weston_config *config,
> + struct weston_backend_config *config_base)
> {
> struct fbdev_backend *b;
> /* TODO: Ideally, available frame buffers should be enumerated using
> diff --git a/src/compositor-headless.c b/src/compositor-headless.c
> index dba21a6..ba0d8d7 100644
> --- a/src/compositor-headless.c
> +++ b/src/compositor-headless.c
> @@ -260,7 +260,8 @@ err_free:
> WL_EXPORT int
> backend_init(struct weston_compositor *compositor,
> int *argc, char *argv[],
> - struct weston_config *config)
> + struct weston_config *config,
> + struct weston_backend_config *config_base)
> {
> int width = 1024, height = 640;
> char *display_name = NULL;
> diff --git a/src/compositor-rdp.c b/src/compositor-rdp.c
> index c221eb9..36c22d8 100644
> --- a/src/compositor-rdp.c
> +++ b/src/compositor-rdp.c
> @@ -1269,7 +1269,8 @@ err_free_strings:
>
> WL_EXPORT int
> backend_init(struct weston_compositor *compositor, int *argc, char *argv[],
> - struct weston_config *wconfig)
> + struct weston_config *wconfig,
> + struct weston_backend_config *config_base)
> {
> struct rdp_backend *b;
> struct rdp_backend_config config;
> diff --git a/src/compositor-rpi.c b/src/compositor-rpi.c
> index 602cbee..f4b33ee 100644
> --- a/src/compositor-rpi.c
> +++ b/src/compositor-rpi.c
> @@ -551,7 +551,8 @@ out_compositor:
> WL_EXPORT int
> backend_init(struct weston_compositor *compositor,
> int *argc, char *argv[],
> - struct weston_config *config)
> + struct weston_config *config,
> + struct weston_backend_config *config_base)
> {
> const char *transform = "normal";
> struct rpi_backend *b;
> diff --git a/src/compositor-wayland.c b/src/compositor-wayland.c
> index f6c84d4..9788714 100644
> --- a/src/compositor-wayland.c
> +++ b/src/compositor-wayland.c
> @@ -2049,7 +2049,8 @@ wayland_backend_destroy(struct wayland_backend *b)
>
> WL_EXPORT int
> backend_init(struct weston_compositor *compositor, int *argc, char *argv[],
> - struct weston_config *config)
> + struct weston_config *config,
> + struct weston_backend_config *config_base)
> {
> struct wayland_backend *b;
> struct wayland_output *output;
> diff --git a/src/compositor-x11.c b/src/compositor-x11.c
> index 73ba783..d18595d 100644
> --- a/src/compositor-x11.c
> +++ b/src/compositor-x11.c
> @@ -1681,7 +1681,8 @@ err_free:
>
> WL_EXPORT int
> backend_init(struct weston_compositor *compositor, int *argc, char *argv[],
> - struct weston_config *config)
> + struct weston_config *config,
> + struct weston_backend_config *config_base)
> {
> struct x11_backend *b;
> int fullscreen = 0;
> diff --git a/src/compositor.h b/src/compositor.h
> index d1ca9ab..acf9016 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -607,9 +607,23 @@ enum weston_capability {
> WESTON_CAP_VIEW_CLIP_MASK = 0x0010,
> };
>
> +struct weston_backend_output_config {
> + uint32_t transform;
> + int32_t width;
> + int32_t height;
> + int scale;
> +};
> +
> struct weston_backend {
> - void (*destroy)(struct weston_compositor *ec);
> - void (*restore)(struct weston_compositor *ec);
> + void (*destroy)(struct weston_compositor *compositor);
> + void (*restore)(struct weston_compositor *compositor);
> + struct weston_output *
> + (*create_output)(struct weston_compositor *compositor,
> + const char *name,
> + struct weston_backend_output_config *config);
> +};
> +
> +struct weston_backend_config {
> };
So it looks like one conceptually distinct change is the stubbing in of
this new, empty weston_backend_config. That looks provably safe, and if
broken out as its own distinct patch I'd be comfortable landing it.
Another change adds structures weston_backend_output_config. I'm a bit
troubled that the name may be too confusingly similar to
weston_backend_config, but whatever. We'll also be getting
weston_wayland_backend_config in one of the other patches, and my brain
starts to ache a little. :-)
But I know finding unique names for all these interrelated bits can be a
bit tough. Maybe a couple sentences worth of docs for each struct would
help.
> struct weston_compositor {
> @@ -1560,7 +1574,8 @@ noop_renderer_init(struct weston_compositor *ec);
> int
> backend_init(struct weston_compositor *c,
> int *argc, char *argv[],
> - struct weston_config *config);
> + struct weston_config *config,
> + struct weston_backend_config *config_base);
> int
> module_init(struct weston_compositor *compositor,
> int *argc, char *argv[]);
> diff --git a/src/main.c b/src/main.c
> index a98570e..19124a9 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -629,6 +629,64 @@ handle_exit(struct weston_compositor *c)
> wl_display_terminate(c->wl_display);
> }
>
> +/* Temporary function to be removed when all backends are converted. */
> +static int
> +init_backend_old(struct weston_compositor *compositor, const char *backend,
> + int *argc, char **argv, struct weston_config *wc)
> +{
> + int (*backend_init)(struct weston_compositor *c,
> + int *argc, char *argv[],
> + struct weston_config *config,
> + struct weston_backend_config *config_base);
> +
> + backend_init = weston_load_module(backend, "backend_init");
> + if (!backend_init)
> + return -1;
> +
> + return backend_init(compositor, argc, argv, wc, NULL);
> +}
> +
> +/* Temporary function to be replaced by weston_compositor_init_backend(). */
> +static int
> +init_backend_new(struct weston_compositor *compositor, const char *backend,
> + struct weston_backend_config *config_base)
> +{
> + int (*backend_init)(struct weston_compositor *c,
> + int *argc, char *argv[],
> + struct weston_config *config,
> + struct weston_backend_config *config_base);
> +
> + backend_init = weston_load_module(backend, "backend_init");
> + if (!backend_init)
> + return -1;
> +
> + return backend_init(compositor, NULL, NULL, NULL, config_base);
> +}
Third change is to add init_backend_old and init_backend_new. While the
latter isn't used in this patch, it makes sense to add them together.
We have both 'backend_init' and 'init_backend', which look like
synonyms. I can see these names are just being carried along from
earlier history, but this refactoring might be an opportunity to come up
with less confusable names? We're also going to be getting
init_*_backend routines...
What is the difference between the module's backend_init call, and it's
init_<module>_backend call?
> +static int
> +init_backend(struct weston_compositor *compositor, const char *backend,
> + int *argc, char **argv, struct weston_config *config)
> +{
> +#if 0
> + if (strstr(backend, "drm-backend.so"))
> + return init_drm_backend(compositor, backend, argc, argv, config);
> + else if (strstr(backend, "wayland-backend.so"))
> + return init_wayland_backend(compositor, backend, argc, argv, config);
> + else if (strstr(backend, "x11-backend.so"))
> + return init_x11_backend(compositor, backend, argc, argv, config);
> + else if (strstr(backend, "fbdev-backend.so"))
> + return init_fbdev_backend(compositor, backend, argc, argv, config);
> + else if (strstr(backend, "headless-backend.so"))
> + return init_headless_backend(compositor, backend, argc, argv, config);
> + else if (strstr(backend, "rpi-backend.so"))
> + return init_rpi_backend(compositor, backend, argc, argv, config);
> + else if (strstr(backend, "rdp-backend.so"))
> + return init_rdp_backend(compositor, backend, argc, argv, config);
> +#endif
> + return init_backend_old(compositor, backend, argc, argv, config);
> +}
String comparison to find the backend function feels a bit inelegant...
But I suppose there aren't so many backends that startup performance is
at risk or anything.
> int main(int argc, char *argv[])
> {
> int ret = EXIT_FAILURE;
> @@ -636,9 +694,6 @@ int main(int argc, char *argv[])
> struct weston_compositor *ec;
> struct wl_event_source *signals[4];
> struct wl_event_loop *loop;
> - int (*backend_init)(struct weston_compositor *c,
> - int *argc, char *argv[],
> - struct weston_config *config);
> int i, fd;
> char *backend = NULL;
> char *shell = NULL;
> @@ -723,10 +778,6 @@ int main(int argc, char *argv[])
> backend = weston_choose_default_backend();
> }
>
> - backend_init = weston_load_module(backend, "backend_init");
> - if (!backend_init)
> - goto out_signals;
> -
> ec = weston_compositor_create(display, NULL);
> if (ec == NULL) {
> weston_log("fatal: failed to create compositor\n");
> @@ -735,11 +786,11 @@ int main(int argc, char *argv[])
>
> ec->config = config;
> if (weston_compositor_init_config(ec, config) < 0)
> - goto out_signals;
> + goto out;
>
> - if (backend_init(ec, &argc, argv, config) < 0) {
> + if (init_backend(ec, backend, &argc, argv, config) < 0) {
> weston_log("fatal: failed to create compositor backend\n");
> - goto out_signals;
> + goto out;
This bug fix would be fine to land on its own.
Bryce
> }
>
> catch_signals();
> --
> 2.5.0
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list