<div class="gmail_quote">On Fri, Aug 3, 2012 at 2:42 AM, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im">On Thu, 2 Aug 2012 20:42:17 -0600 Scott Moreau <<a href="mailto:oreaus@gmail.com">oreaus@gmail.com</a>> 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. </div>Hi Scott, the semantics sound good. Further comments below. </blockquote><div> Hi Pekka, Thanks for your comments. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > --- > 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; > + } </div></div>The above would simplify a bit, if you used just 'mode' (referring to my comments further down). <div class="im"> > + > + 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); </div>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. </blockquote><div> Yes, we will need to identify the output as per user definition. I like the idea of setting the title to the output name. </div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im"> > + > +err_free: > + free(output_width); > + output_width = NULL; > + free(output_height); > + output_height = NULL; </div>Missing x11_free_configured_output() call? Jump from the first goto err_free. </blockquote><div> 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. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im"> > +} > + > 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 }, </div>With size in config file, do we even need the --width and --height command line parameters anymore? </blockquote><div> Yes, I think we'll keep these around. When a user passes --width or --height, it will set all outputs to use this parameter. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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. </blockquote><div> Who said anything about getting rid of the hardcoded default size? </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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.</blockquote><div> I don't agree with this. I think the command line options should always take precedence. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> These command line options will not grow per-output versions nor handle relative positioning, either. </blockquote><div> They wont, but that's not a reason to ignore them. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im"> > { WESTON_OPTION_BOOLEAN, "fullscreen", 0, &fullscreen }, > - { WESTON_OPTION_INTEGER, "output-count", 0, &count }, > + { WESTON_OPTION_INTEGER, "output-count", 0, &count_arg }, </div>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. <div class="im"> > { 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 }, > + }; </div>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. </blockquote><div> Yes, we will use mode=<width>x<height>. I don't think we need to go the extra mile to support raw modelines though. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > + > + 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 </div></div>Thanks, pq </blockquote></div>