Actually, why not just move "xorg_list_append(&xwl_output->link, &xwl_screen->output_list);" to xwl_output_create?
I can't tell if there's a reason it's in xwl_output_done, or if it's just an oversight. On Thu, May 14, 2015 at 9:37 AM, Dima Ryazanov <[email protected]> wrote: > Oh wow, I was playing around with outputs, and never realized > output_handle_done > was being called after any geometry change, not just after the output was > created. > > On Thu, May 14, 2015 at 2:58 AM, Marek Chalupa <[email protected]> > wrote: > >> output.done event can be sent even on some property change, not only >> when announcing the output. Therefore we must check if we already have it >> otherwise we may corrupt the list by adding it multiple times. >> >> This fixes bug when xwayland looped indefinitely in output.done handler >> and that can be reproduced following these steps (under X without >> multi-monitor setup): >> 1) run weston --output-count=2 >> 2) run xterm, move it so that half is on one output >> and half on the other >> 3) close second output, try run weston-terminal >> > > (I can only repro it after closing the first output, not the second one; > is that what you meant?) > > >> weston sends updated outputs which trigger this bug. >> >> Signed-off-by: Marek Chalupa <[email protected]> >> --- >> hw/xwayland/xwayland-output.c | 25 +++++++++++++++++++------ >> 1 file changed, 19 insertions(+), 6 deletions(-) >> >> diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c >> index 155cbc1..0c96e87 100644 >> --- a/hw/xwayland/xwayland-output.c >> +++ b/hw/xwayland/xwayland-output.c >> @@ -116,15 +116,28 @@ output_handle_mode(void *data, struct wl_output >> *wl_output, uint32_t flags, >> static void >> output_handle_done(void *data, struct wl_output *wl_output) >> { >> - struct xwl_output *xwl_output = data; >> + struct xwl_output *it, *xwl_output = data; >> struct xwl_screen *xwl_screen = xwl_output->xwl_screen; >> - int width, height; >> + int width = 0, height = 0, has_this_output = 0; >> + >> + xorg_list_for_each_entry(it, &xwl_screen->output_list, link) { >> + /* output done event is sent even when some property >> + * of output is changed. That means that we may already >> + * have this output. If it is true, we must not add it >> + * into the output_list otherwise we'll corrupt it */ >> + if (it == xwl_output) >> + has_this_output = 1; >> + >> + if (width < it->x + it->width) >> + width = it->x + it->width; >> + if (height < it->y + it->height) >> + height = it->y + it->height; >> + } >> >> - xorg_list_append(&xwl_output->link, &xwl_screen->output_list); >> + if (!has_this_output) { >> + xorg_list_append(&xwl_output->link, &xwl_screen->output_list); >> > > I think this line should also be moved here: > xwl_screen->expecting_event--; > > (It might not matter since it's only used by xwl_screen_init - but the > code seems to assume it would only be decremented once after the output is > created.) > > - width = 0; >> - height = 0; >> - xorg_list_for_each_entry(xwl_output, &xwl_screen->output_list, link) >> { >> + /* we did not check this output for new screen size, do it now */ >> if (width < xwl_output->x + xwl_output->width) >> width = xwl_output->x + xwl_output->width; >> if (height < xwl_output->y + xwl_output->height) >> -- >> 2.1.0 >> >> _______________________________________________ >> wayland-devel mailing list >> [email protected] >> http://lists.freedesktop.org/mailman/listinfo/wayland-devel >> > > Just a thought... You could instead: > - check if the output already exists > - add it if necessary > - update the width and height > > That way, the width/height calculation code won't be duplicated. (Though > it will require iterating over output_list twice.) Anyways, it's up to you. > >
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
