<div class="gmail_quote">On Fri, Aug 3, 2012 at 11:10 AM, Kristian Høgsberg <<a href="mailto:hoegsberg@gmail.com" target="_blank">hoegsberg@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, Aug 02, 2012 at 08:42:17PM -0600, Scott Moreau 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>Nice, that all sounds good. </blockquote><div> Hi Kristian, thanks for the review. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im"> > --- > 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; </div>We're using option_* for globals that we parse from the command line options. </blockquote><div> Noted. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > +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)) </div></div>I don't think you need this special case, just let wl_list_for_each_reverse() run through the empty list and fall through to the output_count case below. </blockquote><div> Yep, makes sense. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im"> > + 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) </div>You also don't need this condition, if we've created enoguh outputs, the loop below wont run. <div class="im"> > + for (i = output_count; i < count; i++) { > + if (x11_compositor_create_output(c, x, 0, width, height, > + fullscreen, no_input) < 0) </div></blockquote><div> Ah, yes. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im"> </div>I think no_input and fullscreen is something we should just store in x11_compositor instead of passing it around everywhere. But that's a different patch. </blockquote><div> For what it's worth, --fullscreen has no effect here. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > + 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>I think we should just do mode=600x400 like we do for the drm backend (but not support the mode line syntax, of course). </blockquote><div> Ok. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <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); </div>Won't this generate a warning if I have an entry for my LVDS or such in the config file? Maybe we should check for output_name[0] != 'X' early (in the top if-statement) and just silenty ignore it. </blockquote><div> Not as it stands currently, because the X output sections are expected to use width/height keys, and it will error before it hits this case. When it expects the same mode key as drm, this logic will be reworked. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im"> > + } else > + wl_list_insert(&configured_output_list, &output->link); </div>Use wl_list_insert(configured_output_list.prev, &output->link); to insert at the end of the list and avoid wl_list_for_each_reverse above. </blockquote><div> Yep. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > + > +err_free: > + free(output_width); > + output_width = NULL; > + free(output_height); > + output_height = NULL; > +} > + > 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 }, > { WESTON_OPTION_BOOLEAN, "fullscreen", 0, &fullscreen }, > - { WESTON_OPTION_INTEGER, "output-count", 0, &count }, > + { WESTON_OPTION_INTEGER, "output-count", 0, &count_arg }, > { 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; </div></div>Just move the default values up to the global variables (eg, static int option_width = 1024;) and the just pass option_width to x11_compositor_create() below. </blockquote><div> I thought that would work at first, but it would break the ability to have --width/--height supersede what is in the config file. I'll try to rethink it and come up with something better than this though. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> > + 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 }, > + }; > + > + 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 > -- > 1.7.11.2 > </div></div>> _______________________________________________ > wayland-devel mailing list > <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> </blockquote></div>