[PATCH weston v5 06/11] x11: port the x11 backend to the new init api
Pekka Paalanen
ppaalanen at gmail.com
Thu Apr 14 11:44:19 UTC 2016
On Wed, 13 Apr 2016 03:25:10 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:
> From: Benoit Gschwind <gschwind at gnu-log.net>
>
> Use a "well" defined structure to configure x11-backend and move configuration
> file parsing inside the weston compositor code.
>
> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
>
> ---
> v5:
> - Update to current trunk
> - Reformated for style consistency (e.g. spaces in "if(",
> linebreaks, etc.)
> - Move variable declarations to top of block
> - Rename header guard for consistency with other headers
> - Other misc. code style formatting fixes
> - Adjust code style to better match drm backend config
> - Version the config struct
> - Dropped use of bzero in favor of zalloc
> - Don't initialize vars already zalloc'd
> - Dropped weston_x11_backend_load in favor of more general
> load_backend_new routine
> - Dropped weston_x11_backend_config_outputs_clear and just free
> outputs in error handler for init routine
> - Dropped some temp variables for output configuration
> - Restore use of 'backend_init' as module entrypoint
> - Rename 'ths' to 'config' for backend config structs
> - Rename 'x11_options' to 'options' for consistency
> - Rename other variables for consistency with other code
> - Rename 'noutputs' to 'num_outputs'
Hi,
this patch needs a little rebasing. There is a small conflict with the
input event loop removal.
There are many small things to fix here.
>
> Makefile.am | 1 +
> src/compositor-drm.c | 5 +-
Note, compositor-drm change here.
> src/compositor-x11.c | 157 ++++++++++++++++++---------------------------------
> src/compositor-x11.h | 60 ++++++++++++++++++++
> src/main.c | 142 +++++++++++++++++++++++++++++++++++++++++++++-
> 5 files changed, 259 insertions(+), 106 deletions(-)
> create mode 100644 src/compositor-x11.h
>
> diff --git a/Makefile.am b/Makefile.am
> index f4cff4c..fde6280 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -73,6 +73,7 @@ weston_SOURCES = \
> src/compositor.c \
> src/compositor.h \
> src/compositor-drm.h \
> + src/compositor-x11.h \
> src/input.c \
> src/data-device.c \
> src/screenshooter.c \
I think the Makefile.am changes should mirror the same for
compositor-drm.h. This is missing installing compositor-x11.h and
listing it as x11-backend.so sources.
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index 21af671..aee42ea 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -3213,7 +3213,10 @@ backend_init(struct weston_compositor *compositor, int *argc, char *argv[],
> config_base->struct_size > sizeof(struct weston_drm_backend_config))
> return -1;
>
> - config = (struct weston_drm_backend_config *)config_base;
> + config = zalloc(sizeof(struct weston_drm_backend_config));
> + if (config == NULL)
> + return -1;
At this point one would initialize 'config' with all default values...
if there were any non-zero default values.
> + memcpy(config, config_base, config_base->struct_size);
So that memcpy will overwrite the passed in values, and the extra
values remain as defaults.
>
> b = drm_backend_create(compositor, config);
> if (b == NULL)
This hunk should be in a compositor-drm patch.
> diff --git a/src/compositor-x11.c b/src/compositor-x11.c
> index f1cb71b..f4b2f37 100644
> --- a/src/compositor-x11.c
> +++ b/src/compositor-x11.c
> @@ -50,21 +50,17 @@
> #include <xkbcommon/xkbcommon.h>
>
> #include "compositor.h"
> -#include "gl-renderer.h"
> -#include "pixman-renderer.h"
> +#include "compositor-x11.h"
> #include "shared/config-parser.h"
> #include "shared/helpers.h"
> #include "shared/image-loader.h"
> +#include "gl-renderer.h"
> +#include "pixman-renderer.h"
> #include "presentation-time-server-protocol.h"
> #include "linux-dmabuf.h"
>
> #define DEFAULT_AXIS_STEP_DISTANCE 10
>
> -static int option_width;
> -static int option_height;
> -static int option_scale;
> -static int option_count;
> -
Very nice to get rid of these globals.
> struct x11_backend {
> struct weston_backend base;
> struct weston_compositor *compositor;
> @@ -1566,24 +1562,15 @@ init_gl_renderer(struct x11_backend *b)
>
> return ret;
> }
> +
> static struct x11_backend *
> x11_backend_create(struct weston_compositor *compositor,
> - int fullscreen,
> - int no_input,
> - int use_pixman,
> - int *argc, char *argv[],
> - struct weston_config *config)
> + struct weston_x11_backend_config *config)
> {
> struct x11_backend *b;
> struct x11_output *output;
> - struct weston_config_section *section;
> - int i, x = 0, output_count = 0;
> - int width, height, scale, count;
> - const char *section_name;
> - char *name, *t, *mode;
> - uint32_t transform;
> -
> - weston_log("initializing x11 backend\n");
> + int x = 0;
> + unsigned i;
>
> b = zalloc(sizeof *b);
> if (b == NULL)
> @@ -1609,13 +1596,13 @@ x11_backend_create(struct weston_compositor *compositor,
> x11_backend_get_resources(b);
> x11_backend_get_wm_info(b);
>
> - if (!b->has_net_wm_state_fullscreen && fullscreen) {
> + if (!b->has_net_wm_state_fullscreen && config->fullscreen) {
> weston_log("Can not fullscreen without window manager support"
> "(need _NET_WM_STATE_FULLSCREEN)\n");
> - fullscreen = 0;
> + config->fullscreen = 0;
> }
>
> - b->use_pixman = use_pixman;
> + b->use_pixman = config->use_pixman;
> if (b->use_pixman) {
> if (pixman_renderer_init(compositor) < 0) {
> weston_log("Failed to initialize pixman renderer for X11 backend\n");
> @@ -1625,83 +1612,51 @@ x11_backend_create(struct weston_compositor *compositor,
> else if (init_gl_renderer(b) < 0) {
> goto err_xdisplay;
> }
> - weston_log("Using %s renderer\n", use_pixman ? "pixman" : "gl");
> + weston_log("Using %s renderer\n", config->use_pixman ? "pixman" : "gl");
>
> b->base.destroy = x11_destroy;
> b->base.restore = x11_restore;
>
> - if (x11_input_create(b, no_input) < 0) {
> + if (x11_input_create(b, config->no_input) < 0) {
> weston_log("Failed to create X11 input\n");
> goto err_renderer;
> }
>
> - width = option_width ? option_width : 1024;
> - height = option_height ? option_height : 640;
> - scale = option_scale ? option_scale : 1;
> - count = option_count ? option_count : 1;
> + for (i = 0; i < config->num_outputs; ++i) {
> + struct weston_x11_backend_output_config *output_iterator =
> + &config->outputs[i];
>
> - section = NULL;
> - while (weston_config_next_section(config,
> - §ion, §ion_name)) {
> - if (strcmp(section_name, "output") != 0)
> - continue;
> - weston_config_section_get_string(section, "name", &name, NULL);
> - if (name == NULL || name[0] != 'X') {
> - free(name);
> + if (output_iterator->name == NULL) {
> continue;
> }
>
> - weston_config_section_get_string(section,
> - "mode", &mode, "1024x600");
> - if (sscanf(mode, "%dx%d", &width, &height) != 2) {
> - weston_log("Invalid mode \"%s\" for output %s\n",
> - mode, name);
> - width = 1024;
> - height = 600;
> - }
> - free(mode);
> -
> - if (option_width)
> - width = option_width;
> - if (option_height)
> - height = option_height;
> -
> - weston_config_section_get_int(section, "scale", &scale, 1);
> - if (option_scale)
> - scale = option_scale;
> -
> - weston_config_section_get_string(section,
> - "transform", &t, "normal");
> - if (weston_parse_transform(t, &transform) < 0)
> - weston_log("Invalid transform \"%s\" for output %s\n",
> - t, name);
> - free(t);
> -
> - output = x11_backend_create_output(b, x, 0,
> - width, height,
> - fullscreen, no_input,
> - name, transform, scale);
> - free(name);
> - if (output == NULL) {
> - weston_log("Failed to create configured x11 output\n");
> - goto err_x11_input;
> + if (output_iterator->width < 1) {
> + weston_log("Invalid width \"%d\" for output %s\n",
> + output_iterator->width, output_iterator->name);
> + output_iterator->width = 1024;
> }
>
> - x = pixman_region32_extents(&output->base.region)->x2;
> -
> - output_count++;
> - if (option_count && output_count >= option_count)
> - break;
> - }
> + if (output_iterator->height < 1) {
> + weston_log("Invalid height \"%d\" for output %s\n",
> + output_iterator->height, output_iterator->name);
> + output_iterator->height = 600;
> + }
Invalid width or height would be reason enough to fail creating the
backend. The caller must provide valid values.
It is the caller's job to ensure the values are valid, and it does that
when parsing weston.ini and the command line options.
>
> - for (i = output_count; i < count; i++) {
> - output = x11_backend_create_output(b, x, 0, width, height,
> - fullscreen, no_input, NULL,
> - WL_OUTPUT_TRANSFORM_NORMAL, scale);
> + output = x11_backend_create_output(b,
> + x,
> + 0,
> + output_iterator->width,
> + output_iterator->height,
> + config->fullscreen,
> + config->no_input,
> + output_iterator->name,
> + output_iterator->transform,
> + output_iterator->scale);
> if (output == NULL) {
> - weston_log("Failed to create x11 output #%d\n", i);
> + weston_log("Failed to create configured x11 output\n");
> goto err_x11_input;
> }
> +
> x = pixman_region32_extents(&output->base.region)->x2;
> }
>
> @@ -1734,33 +1689,29 @@ err_free:
> }
>
> WL_EXPORT int
> -backend_init(struct weston_compositor *compositor, int *argc, char *argv[],
> - struct weston_config *config,
> +backend_init(struct weston_compositor *compositor,
> + int *argc, char *argv[],
> + struct weston_config *wc,
> struct weston_backend_config *config_base)
> {
> struct x11_backend *b;
> - int fullscreen = 0;
> - int no_input = 0;
> - int use_pixman = 0;
> -
> - const struct weston_option x11_options[] = {
> - { WESTON_OPTION_INTEGER, "width", 0, &option_width },
> - { WESTON_OPTION_INTEGER, "height", 0, &option_height },
> - { WESTON_OPTION_INTEGER, "scale", 0, &option_scale },
> - { WESTON_OPTION_BOOLEAN, "fullscreen", 'f', &fullscreen },
> - { WESTON_OPTION_INTEGER, "output-count", 0, &option_count },
> - { WESTON_OPTION_BOOLEAN, "no-input", 0, &no_input },
> - { WESTON_OPTION_BOOLEAN, "use-pixman", 0, &use_pixman },
> - };
> + struct weston_x11_backend_config *config;
> +
> + if (config_base == NULL ||
> + config_base->struct_version != 1 ||
> + config_base->struct_size > sizeof(struct weston_x11_backend_config)) {
> + weston_log("X11 backend config structure is invalid\n");
> + return -1;
> + }
>
> - parse_options(x11_options, ARRAY_LENGTH(x11_options), argc, argv);
> + config = zalloc(sizeof(struct weston_x11_backend_config));
> + if (config == NULL)
> + return -1;
> + memcpy(config, config_base, config_base->struct_size);
>
> - b = x11_backend_create(compositor,
> - fullscreen,
> - no_input,
> - use_pixman,
> - argc, argv, config);
> + b = x11_backend_create(compositor, config);
'config' is leaked.
> if (b == NULL)
> return -1;
> +
> return 0;
> }
> diff --git a/src/compositor-x11.h b/src/compositor-x11.h
> new file mode 100644
> index 0000000..18e3e01
> --- /dev/null
> +++ b/src/compositor-x11.h
> @@ -0,0 +1,60 @@
> +/*
> + * Copyright © 2016 Benoit Gschwind
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining
> + * a copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sublicense, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial
> + * portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#ifndef _WESTON_COMPOSITOR_X11_H
> +#define _WESTON_COMPOSITOR_X11_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include "compositor.h"
> +
> +struct weston_x11_backend_output_config {
> + int width;
> + int height;
> + char *name;
> + uint32_t transform;
> + int32_t scale;
> +};
> +
> +struct weston_x11_backend_config {
> + struct weston_backend_config base;
> +
> + bool fullscreen;
> + bool no_input;
> +
> + /** Whether to use the pixman renderer instead of the OpenGL ES renderer. */
> + bool use_pixman;
> +
> + uint32_t num_outputs;
> + struct weston_x11_backend_output_config *outputs;
> +};
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* SRC_COMPOSITOR_X11_H_ */
> diff --git a/src/main.c b/src/main.c
> index 807bd4d..a72a9d7 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -48,6 +48,7 @@
> #include "version.h"
>
> #include "compositor-drm.h"
> +#include "compositor-x11.h"
>
> static struct wl_list child_process_list;
> static struct weston_compositor *segv_compositor;
> @@ -672,6 +673,7 @@ load_backend_new(struct weston_compositor *compositor, const char *backend,
> return backend_init(compositor, NULL, NULL, NULL, config_base);
> }
>
> +
> struct drm_config {
> struct weston_drm_backend_config base;
> bool use_current_mode;
> @@ -763,16 +765,152 @@ load_drm_backend(struct weston_compositor *c, const char *backend,
> }
>
> static int
> +weston_x11_backend_config_append_output_config(struct weston_x11_backend_config *config,
> + struct weston_x11_backend_output_config *output_config) {
> + struct weston_x11_backend_output_config *new_outputs;
> +
> + new_outputs = realloc(config->outputs, (config->num_outputs+1) *
> + sizeof(struct weston_x11_backend_output_config));
> + if (new_outputs == NULL)
> + return -1;
> +
> + config->outputs = new_outputs;
> + config->outputs[(config->num_outputs)++] = *output_config;
> + return 0;
> +}
Realloc does not seem necessary. We get the number of outputs given to
us directly, might as well just allocate the exact number of elements to
begin with.
> +
> +static int
> +load_x11_backend(struct weston_compositor *c, char const * backend,
> + int *argc, char **argv, struct weston_config *wc)
> +{
> + struct weston_x11_backend_output_config default_output;
> + struct weston_x11_backend_config *config;
> + struct weston_config_section *section;
> + int ret = 0;
> + int option_width = 0;
> + int option_height = 0;
> + int option_scale = 0;
> + int option_count = 1;
> + int output_count = 0;
> + char const *section_name;
> + int i;
> + uint32_t j;
> +
> + config = zalloc(sizeof(struct weston_x11_backend_config));
> + if (config == NULL)
> + return -1;
> +
> + const struct weston_option options[] = {
> + { WESTON_OPTION_INTEGER, "width", 0, &option_width },
> + { WESTON_OPTION_INTEGER, "height", 0, &option_height },
> + { WESTON_OPTION_INTEGER, "scale", 0, &option_scale },
> + { WESTON_OPTION_BOOLEAN, "fullscreen", 'f', &config->fullscreen },
> + { WESTON_OPTION_INTEGER, "output-count", 0, &option_count },
> + { WESTON_OPTION_BOOLEAN, "no-input", 0, &config->no_input },
> + { WESTON_OPTION_BOOLEAN, "use-pixman", 0, &config->use_pixman },
> + };
Mixed code and decls.
> +
> + parse_options(options, ARRAY_LENGTH(options), argc, argv);
> +
> + section = NULL;
> + while (weston_config_next_section(wc, §ion, §ion_name)) {
> + struct weston_x11_backend_output_config current_output = { 0, };
> + char *t;
> + char *mode;
> +
> + if (strcmp(section_name, "output") != 0) {
> + continue;
> + }
> +
> + weston_config_section_get_string(section, "name", ¤t_output.name, NULL);
> + if (current_output.name == NULL || current_output.name[0] != 'X') {
> + free(current_output.name);
> + continue;
> + }
> +
> + weston_config_section_get_string(section, "mode", &mode, "1024x600");
> + if (sscanf(mode, "%dx%d", ¤t_output.width,
> + ¤t_output.height) != 2) {
> + weston_log("Invalid mode \"%s\" for output %s\n",
> + mode, current_output.name);
> + current_output.width = 1024;
> + current_output.height = 600;
> + }
> + free(mode);
> + if (current_output.width < 1)
> + current_output.width = 1024;
> + if (current_output.height < 1)
> + current_output.height = 600;
This looks like a change in behaviour by omitting option checks.
Previously, if option_width or option_height were set, they overruled
any weston.ini settings. However here, the ini settings get priority.
Changes in behaviour should be split out into separate patches if they
are desired, and we can discuss then if it's a good change.
> +
> + weston_config_section_get_int(section, "scale", ¤t_output.scale, 1);
> + if (option_scale)
> + current_output.scale = option_scale;
> +
> + weston_config_section_get_string(section,
> + "transform", &t, "normal");
> + if (weston_parse_transform(t, ¤t_output.transform) < 0)
> + weston_log("Invalid transform \"%s\" for output %s\n",
> + t, current_output.name);
> + free(t);
> +
> + if (weston_x11_backend_config_append_output_config(config, ¤t_output) < 0) {
> + ret = -1;
> + goto error;
> + }
> +
> + output_count++;
> + if (option_count && output_count >= option_count)
> + break;
> + }
> +
> + default_output.name = NULL;
> + default_output.width = option_width ? option_width : 1024;
> + default_output.height = option_height ? option_height : 600;
> + default_output.scale = option_scale ? option_scale : 1;
> + default_output.transform = WL_OUTPUT_TRANSFORM_NORMAL;
> +
> + for (i = output_count; i < option_count; i++) {
> + char name[16];
> + snprintf(name, 16, "screen%d", i);
> + default_output.name = strdup(name);
This would be much shorter with asprintf since you have to call strdup.
> +
> + if (weston_x11_backend_config_append_output_config(config, &default_output) < 0) {
> + ret = -1;
> + goto error;
> + }
> + }
> +
> + config->base.struct_version = 1;
> + config->base.struct_size = sizeof(struct weston_x11_backend_config);
> +
> + /* load the actual drm backend and configure it */
> + if (load_backend_new(c, backend,
> + (struct weston_backend_config *)config) < 0) {
> + ret = -1;
> + goto error;
> + }
> +
> + return ret;
> +
> +error:
> + for (j = 0; j < config->num_outputs; ++j)
> + free(config->outputs[j].name);
> + free(config->outputs);
> + free(config);
> + return ret;
> +}
> +
> +static int
> load_backend(struct weston_compositor *compositor, const char *backend,
> int *argc, char **argv, struct weston_config *config)
> {
> if (strstr(backend, "drm-backend.so"))
> return load_drm_backend(compositor, backend, argc, argv, config);
> + else if (strstr(backend, "x11-backend.so"))
> + return load_x11_backend(compositor, backend, argc, argv, config);
> #if 0
> else if (strstr(backend, "wayland-backend.so"))
> return load_wayland_backend(compositor, backend, argc, argv, config);
> - else if (strstr(backend, "x11-backend.so"))
> - return load_x11_backend(compositor, backend, argc, argv, config);
> else if (strstr(backend, "fbdev-backend.so"))
> return load_fbdev_backend(compositor, backend, argc, argv, config);
> else if (strstr(backend, "headless-backend.so"))
The patch has some style issues: mixed code and decls, too long lines,
missing an empty line between decls and code.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160414/0434801a/attachment-0001.sig>
More information about the wayland-devel
mailing list