[PATCH] compositor-x11: Allow output configuration from config file.

Pekka Paalanen ppaalanen at gmail.com
Fri Aug 3 01:42:04 PDT 2012


On Thu,  2 Aug 2012 20:42:17 -0600
Scott Moreau <oreaus at gmail.com> wrote:

> This patch provides a way to define outputs for the x11 backend. It
> parses [output] sections and checks for 'name', 'width' and 'height'
> keys. The 'name' key must start with an 'X' to distinguish between
> drm output names. Command line options --width and --height supersede
> what is in the config file. When --output-count is passed, the number
> of outputs are limited or additional outputs added with default
> values.

Hi Scott,

the semantics sound good. Further comments below.

> ---
>  src/compositor-x11.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++----
>  weston.ini           |   5 ++
>  2 files changed, 132 insertions(+), 9 deletions(-)
> 
> diff --git a/src/compositor-x11.c b/src/compositor-x11.c
> index 4507463..992b68f 100644
> --- a/src/compositor-x11.c
> +++ b/src/compositor-x11.c
> @@ -50,6 +50,20 @@
>  #include "compositor.h"
>  #include "../shared/config-parser.h"
>  
> +static char *output_name;
> +static char *output_width;
> +static char *output_height;
> +static int width_arg;
> +static int height_arg;
> +static int count_arg;
> +static struct wl_list configured_output_list;
> +
> +struct x11_configured_output {
> +	char *name;
> +	int width, height;
> +	struct wl_list link;
> +};
> +
>  struct x11_compositor {
>  	struct weston_compositor	 base;
>  
> @@ -1033,9 +1047,20 @@ x11_restore(struct weston_compositor *ec)
>  }
>  
>  static void
> +x11_free_configured_output(struct x11_configured_output *output)
> +{
> +	free(output->name);
> +	free(output);
> +}
> +
> +static void
>  x11_destroy(struct weston_compositor *ec)
>  {
>  	struct x11_compositor *compositor = (struct x11_compositor *)ec;
> +	struct x11_configured_output *o, *n;
> +
> +	wl_list_for_each_safe(o, n, &configured_output_list, link)
> +		x11_free_configured_output(o);
>  
>  	wl_event_source_remove(compositor->xcb_source);
>  	x11_input_destroy(compositor);
> @@ -1055,8 +1080,9 @@ x11_compositor_create(struct wl_display *display,
>  		      int argc, char *argv[], const char *config_file)
>  {
>  	struct x11_compositor *c;
> +	struct x11_configured_output *o;
>  	xcb_screen_iterator_t s;
> -	int i, x;
> +	int i, x = 0, output_count = 0;
>  
>  	weston_log("initializing x11 backend\n");
>  
> @@ -1099,11 +1125,35 @@ x11_compositor_create(struct wl_display *display,
>  	if (x11_input_create(c, no_input) < 0)
>  		goto err_egl;
>  
> -	for (i = 0, x = 0; i < count; i++) {
> -		if (x11_compositor_create_output(c, x, 0, width, height,
> -						 fullscreen, no_input) < 0)
> -			goto err_x11_input;
> -		x += width;
> +	if (wl_list_empty(&configured_output_list))
> +		for (i = 0; i < count; i++) {
> +			if (x11_compositor_create_output(c, x, 0, width, height,
> +							 fullscreen, no_input) < 0)
> +				goto err_x11_input;
> +			x += width;
> +		}
> +	else {
> +		wl_list_for_each_reverse(o, &configured_output_list, link) {
> +			if (x11_compositor_create_output(c, x, 0,
> +							width_arg ? width_arg :
> +							o->width,
> +							height_arg ? height_arg :
> +							o->height,
> +							fullscreen, no_input) < 0)
> +				goto err_x11_input;
> +			x += width_arg ? width_arg : o->width;
> +			output_count++;
> +			if (count_arg && output_count >= count_arg)
> +				break;
> +		}
> +
> +		if (count > output_count)
> +			for (i = output_count; i < count; i++) {
> +				if (x11_compositor_create_output(c, x, 0, width, height,
> +								 fullscreen, no_input) < 0)
> +					goto err_x11_input;
> +				x += width;
> +			}
>  	}
>  
>  	c->xcb_source =
> @@ -1126,6 +1176,51 @@ err_free:
>  	return NULL;
>  }
>  
> +static void
> +output_section_done(void *data)
> +{
> +	struct x11_configured_output *output;
> +	int error = 0;
> +
> +	output = malloc(sizeof *output);
> +
> +	if (!output || !output_name || !output_width || !output_height) {
> +		free(output_name);
> +		output_name = NULL;
> +
> +		goto err_free;
> +	}
> +
> +	output->name = output_name;
> +
> +	if (sscanf(output_width, "%d", &output->width) != 1) {
> +		weston_log("Invalid width \"%s\" for output %s\n",
> +						output_width, output_name);
> +		error = 1;
> +	}
> +
> +	if (sscanf(output_height, "%d", &output->height) != 1) {
> +		weston_log("Invalid height \"%s\" for output %s\n",
> +						output_height, output_name);
> +		error = 1;
> +	}

The above would simplify a bit, if you used just 'mode' (referring to
my comments further down).

> +
> +	if (error) {
> +		x11_free_configured_output(output);
> +		goto err_free;
> +	} else if (output_name[0] != 'X') {
> +		weston_log("Invalid output name \"%s\"\n", output_name);
> +		x11_free_configured_output(output);
> +	} else
> +		wl_list_insert(&configured_output_list, &output->link);

I see you are not using output->name anywhere else, so you practically
discard it. I guess that left for follow-up patches, when you implement
the relative positioning directives?

Would be nice to see the output name in the X window title.

> +
> +err_free:
> +	free(output_width);
> +	output_width = NULL;
> +	free(output_height);
> +	output_height = NULL;

Missing x11_free_configured_output() call? Jump from the first goto
err_free.

> +}
> +
>  WL_EXPORT struct weston_compositor *
>  backend_init(struct wl_display *display, int argc, char *argv[],
>  	     const char *config_file)
> @@ -1134,15 +1229,38 @@ backend_init(struct wl_display *display, int argc, char *argv[],
>  	int no_input = 0;
>  
>  	const struct weston_option x11_options[] = {
> -		{ WESTON_OPTION_INTEGER, "width", 0, &width },
> -		{ WESTON_OPTION_INTEGER, "height", 0, &height },
> +		{ WESTON_OPTION_INTEGER, "width", 0, &width_arg },
> +		{ WESTON_OPTION_INTEGER, "height", 0, &height_arg },

With size in config file, do we even need the --width and --height
command line parameters anymore?

The x11 backend is just a testing backend, so I see no reason to avoid
having a config file. We can still have the hardcoded default size.

Oh, another though, maybe --width and --height should set only the
default size, and config would override that? Yes, it's a precedence
violation, but these two overriding the output config seems less
convenient than just setting defaults. These command line options will
not grow per-output versions nor handle relative positioning, either.

>  		{ WESTON_OPTION_BOOLEAN, "fullscreen", 0, &fullscreen },
> -		{ WESTON_OPTION_INTEGER, "output-count", 0, &count },
> +		{ WESTON_OPTION_INTEGER, "output-count", 0, &count_arg },

I could ask the same about output-count, since your patch takes care of
that based on what is configured in the file, but I think this could be
handy to keep.

>  		{ WESTON_OPTION_BOOLEAN, "no-input", 0, &no_input },
>  	};
>  
>  	parse_options(x11_options, ARRAY_LENGTH(x11_options), argc, argv);
>  
> +	if (width_arg)
> +		width = width_arg;
> +	if (height_arg)
> +		height = height_arg;
> +	if (count_arg)
> +		count = count_arg;
> +
> +	wl_list_init(&configured_output_list);
> +
> +	const struct config_key x11_config_keys[] = {
> +		{ "name", CONFIG_KEY_STRING, &output_name },
> +		{ "width", CONFIG_KEY_STRING, &output_width },
> +		{ "height", CONFIG_KEY_STRING, &output_height },
> +	};

Why 'width' and 'height' instead of just 'mode' like the DRM backend
uses?

This will make an [output] section sometimes use 'mode' and sometimes
'width' and 'height', which I think is confusing. Using only 'mode'
everywhere would be consistent within a config file. 'mode' with just
<width>x<height> is clear, and I might even go as far as saying that it
could accept even a complete modeline, and use only the active image
area, discarding other parameters. That would allow one to just copy a
DRM output section and change the name to have a working x11 output
section for testing.

> +
> +	const struct config_section config_section[] = {
> +		{ "output", x11_config_keys,
> +		ARRAY_LENGTH(x11_config_keys), output_section_done },
> +	};
> +
> +	parse_config_file(config_file, config_section,
> +				ARRAY_LENGTH(config_section), NULL);
> +
>  	return x11_compositor_create(display,
>  				     width, height, count, fullscreen,
>  				     no_input,
> diff --git a/weston.ini b/weston.ini
> index c2d6369..1de8e29 100644
> --- a/weston.ini
> +++ b/weston.ini
> @@ -41,3 +41,8 @@ duration=600
>  #[output]
>  #name=VGA1
>  #mode=173.00  1920 2048 2248 2576  1080 1083 1088 1120 -hsync +vsync
> +
> +#[output]
> +#name=X1
> +#width=1024
> +#height=768


Thanks,
pq


More information about the wayland-devel mailing list