[PATCH] compositor-x11: Allow output configuration from config file.
Scott Moreau
oreaus at gmail.com
Fri Aug 3 11:42:22 PDT 2012
On Fri, Aug 3, 2012 at 2:42 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> 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.
>
Hi Pekka,
Thanks for your comments.
>
> > ---
> > 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.
>
Yes, we will need to identify the output as per user definition. I like the
idea of setting the title to the output name.
>
> > +
> > +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.
>
I don't really like the way the clean-up is structured currently, hopefully
it will become simpler when it's reworked to accept mode instead of
width/height.
>
> > +}
> > +
> > 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?
>
Yes, I think we'll keep these around. When a user passes --width or
--height, it will set all outputs to use this parameter.
>
> 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.
>
Who said anything about getting rid of 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.
I don't agree with this. I think the command line options should always
take precedence.
> These command line options will
> not grow per-output versions nor handle relative positioning, either.
>
They wont, but that's not a reason to ignore them.
>
> > { 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.
>
Yes, we will use mode=<width>x<height>. I don't think we need to go the
extra mile to support raw modelines though.
>
> > +
> > + 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20120803/0d429a4f/attachment-0001.html>
More information about the wayland-devel
mailing list