[weston] compositor-x11: Move the x11 event handler to the display loop

Submitted by Derek Foreman on Dec. 12, 2014, 8:12 p.m.

Details

Message ID 1418415148-14554-1-git-send-email-derekf@osg.samsung.com
State Accepted
Commit 6deb09ef8a72164947cdfa5f2414e292c7672c9c
Headers show

Not browsing as part of any series.

Commit Message

Derek Foreman Dec. 12, 2014, 8:12 p.m.
While it conceptually makes sense to put the x11 event handler
in the compositor "input" loop, the input loop is actually
dispatched in the middle of the frame repaint.  When the
X11 event results in closing the compositor, this can cause
the current output to be destroyed just prior to trying to
process animations on it.

Closes bug: https://bugs.freedesktop.org/show_bug.cgi?id=81314

Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
---
 src/compositor-x11.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/compositor-x11.c b/src/compositor-x11.c
index a760f33..cf39ad7 100644
--- a/src/compositor-x11.c
+++ b/src/compositor-x11.c
@@ -1455,6 +1455,7 @@  x11_compositor_create(struct wl_display *display,
 	struct x11_compositor *c;
 	struct x11_output *output;
 	struct weston_config_section *section;
+	struct wl_event_loop *loop;
 	xcb_screen_iterator_t s;
 	int i, x = 0, output_count = 0;
 	int width, height, scale, count;
@@ -1580,8 +1581,9 @@  x11_compositor_create(struct wl_display *display,
 		x = pixman_region32_extents(&output->base.region)->x2;
 	}
 
+	loop = wl_display_get_event_loop(c->base.wl_display);
 	c->xcb_source =
-		wl_event_loop_add_fd(c->base.input_loop,
+		wl_event_loop_add_fd(loop,
 				     xcb_get_file_descriptor(c->conn),
 				     WL_EVENT_READABLE,
 				     x11_compositor_handle_event, c);

Comments

I just noticed that the follow patch *exactly* undoes commit 22ba60e

Is there any other reason that commit was necessary, or was it intended
to be cosmetic?

On 12/12/14 02:12 PM, Derek Foreman wrote:
> While it conceptually makes sense to put the x11 event handler
> in the compositor "input" loop, the input loop is actually
> dispatched in the middle of the frame repaint.  When the
> X11 event results in closing the compositor, this can cause
> the current output to be destroyed just prior to trying to
> process animations on it.
> 
> Closes bug: https://bugs.freedesktop.org/show_bug.cgi?id=81314
> 
> Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
> ---
>  src/compositor-x11.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/compositor-x11.c b/src/compositor-x11.c
> index a760f33..cf39ad7 100644
> --- a/src/compositor-x11.c
> +++ b/src/compositor-x11.c
> @@ -1455,6 +1455,7 @@ x11_compositor_create(struct wl_display *display,
>  	struct x11_compositor *c;
>  	struct x11_output *output;
>  	struct weston_config_section *section;
> +	struct wl_event_loop *loop;
>  	xcb_screen_iterator_t s;
>  	int i, x = 0, output_count = 0;
>  	int width, height, scale, count;
> @@ -1580,8 +1581,9 @@ x11_compositor_create(struct wl_display *display,
>  		x = pixman_region32_extents(&output->base.region)->x2;
>  	}
>  
> +	loop = wl_display_get_event_loop(c->base.wl_display);
>  	c->xcb_source =
> -		wl_event_loop_add_fd(c->base.input_loop,
> +		wl_event_loop_add_fd(loop,
>  				     xcb_get_file_descriptor(c->conn),
>  				     WL_EVENT_READABLE,
>  				     x11_compositor_handle_event, c);
>
On Fri, 12 Dec 2014 14:29:42 -0600
Derek Foreman <derekf@osg.samsung.com> wrote:

> I just noticed that the follow patch *exactly* undoes commit 22ba60e
> 
> Is there any other reason that commit was necessary, or was it intended
> to be cosmetic?

No, I don't think it was meant to be cosmetic.

There is some fancy input scheduling going on with the repaint loop, so
that input events would be sent as a burst to the clients when the
compositor is spinning in the repaint loop. The idea is that because
clients repaint only once per cycle, they don't need to be woken up due
to input any more often than that. Or that's how I understood it.

The problem here, I assume, is that the X11 fd is not only input, but
also output events, unlike DRM backend where the DRM fd is separate.

I would be inclined to just take this patch, because it fixes a bug and
the x11 backend is of lesser quality anyway.

But, I also wonder, if reading the evdev fds in DRM backend only once
per cycle could cause the kernel input buffers to overflow in some cases
(gaming mice?), leading to lost input events?


Thanks,
pq

> On 12/12/14 02:12 PM, Derek Foreman wrote:
> > While it conceptually makes sense to put the x11 event handler
> > in the compositor "input" loop, the input loop is actually
> > dispatched in the middle of the frame repaint.  When the
> > X11 event results in closing the compositor, this can cause
> > the current output to be destroyed just prior to trying to
> > process animations on it.
> > 
> > Closes bug: https://bugs.freedesktop.org/show_bug.cgi?id=81314
> > 
> > Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
> > ---
> >  src/compositor-x11.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/compositor-x11.c b/src/compositor-x11.c
> > index a760f33..cf39ad7 100644
> > --- a/src/compositor-x11.c
> > +++ b/src/compositor-x11.c
> > @@ -1455,6 +1455,7 @@ x11_compositor_create(struct wl_display *display,
> >  	struct x11_compositor *c;
> >  	struct x11_output *output;
> >  	struct weston_config_section *section;
> > +	struct wl_event_loop *loop;
> >  	xcb_screen_iterator_t s;
> >  	int i, x = 0, output_count = 0;
> >  	int width, height, scale, count;
> > @@ -1580,8 +1581,9 @@ x11_compositor_create(struct wl_display *display,
> >  		x = pixman_region32_extents(&output->base.region)->x2;
> >  	}
> >  
> > +	loop = wl_display_get_event_loop(c->base.wl_display);
> >  	c->xcb_source =
> > -		wl_event_loop_add_fd(c->base.input_loop,
> > +		wl_event_loop_add_fd(loop,
> >  				     xcb_get_file_descriptor(c->conn),
> >  				     WL_EVENT_READABLE,
> >  				     x11_compositor_handle_event, c);
> > 
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
On 19/12/14 06:40 AM, Pekka Paalanen wrote:
> On Fri, 12 Dec 2014 14:29:42 -0600
> Derek Foreman <derekf@osg.samsung.com> wrote:
> 
>> I just noticed that the follow patch *exactly* undoes commit 22ba60e
>>
>> Is there any other reason that commit was necessary, or was it intended
>> to be cosmetic?
> 
> No, I don't think it was meant to be cosmetic.
> 
> There is some fancy input scheduling going on with the repaint loop, so
> that input events would be sent as a burst to the clients when the
> compositor is spinning in the repaint loop. The idea is that because
> clients repaint only once per cycle, they don't need to be woken up due
> to input any more often than that. Or that's how I understood it.
> 
> The problem here, I assume, is that the X11 fd is not only input, but
> also output events, unlike DRM backend where the DRM fd is separate.
> 
> I would be inclined to just take this patch, because it fixes a bug and
> the x11 backend is of lesser quality anyway.

Hmmm, I think the only event that matters is XCB_CLIENT_MESSAGE (delete
window).  Can't I just handle that from an idle callback?

> But, I also wonder, if reading the evdev fds in DRM backend only once
> per cycle could cause the kernel input buffers to overflow in some cases
> (gaming mice?), leading to lost input events?

Well, I get a lot of dropped events with the pixman renderer and a
1000hz mouse if I start rotating a few windows, but I think that's not
going to go away unless input ends up in its own thread.

> 
> Thanks,
> pq
> 
>> On 12/12/14 02:12 PM, Derek Foreman wrote:
>>> While it conceptually makes sense to put the x11 event handler
>>> in the compositor "input" loop, the input loop is actually
>>> dispatched in the middle of the frame repaint.  When the
>>> X11 event results in closing the compositor, this can cause
>>> the current output to be destroyed just prior to trying to
>>> process animations on it.
>>>
>>> Closes bug: https://bugs.freedesktop.org/show_bug.cgi?id=81314
>>>
>>> Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
>>> ---
>>>  src/compositor-x11.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/compositor-x11.c b/src/compositor-x11.c
>>> index a760f33..cf39ad7 100644
>>> --- a/src/compositor-x11.c
>>> +++ b/src/compositor-x11.c
>>> @@ -1455,6 +1455,7 @@ x11_compositor_create(struct wl_display *display,
>>>  	struct x11_compositor *c;
>>>  	struct x11_output *output;
>>>  	struct weston_config_section *section;
>>> +	struct wl_event_loop *loop;
>>>  	xcb_screen_iterator_t s;
>>>  	int i, x = 0, output_count = 0;
>>>  	int width, height, scale, count;
>>> @@ -1580,8 +1581,9 @@ x11_compositor_create(struct wl_display *display,
>>>  		x = pixman_region32_extents(&output->base.region)->x2;
>>>  	}
>>>  
>>> +	loop = wl_display_get_event_loop(c->base.wl_display);
>>>  	c->xcb_source =
>>> -		wl_event_loop_add_fd(c->base.input_loop,
>>> +		wl_event_loop_add_fd(loop,
>>>  				     xcb_get_file_descriptor(c->conn),
>>>  				     WL_EVENT_READABLE,
>>>  				     x11_compositor_handle_event, c);
>>>
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
On Fri, 19 Dec 2014 08:08:33 -0600
Derek Foreman <derekf@osg.samsung.com> wrote:

> On 19/12/14 06:40 AM, Pekka Paalanen wrote:
> > On Fri, 12 Dec 2014 14:29:42 -0600
> > Derek Foreman <derekf@osg.samsung.com> wrote:
> > 
> >> I just noticed that the follow patch *exactly* undoes commit 22ba60e
> >>
> >> Is there any other reason that commit was necessary, or was it intended
> >> to be cosmetic?
> > 
> > No, I don't think it was meant to be cosmetic.
> > 
> > There is some fancy input scheduling going on with the repaint loop, so
> > that input events would be sent as a burst to the clients when the
> > compositor is spinning in the repaint loop. The idea is that because
> > clients repaint only once per cycle, they don't need to be woken up due
> > to input any more often than that. Or that's how I understood it.
> > 
> > The problem here, I assume, is that the X11 fd is not only input, but
> > also output events, unlike DRM backend where the DRM fd is separate.
> > 
> > I would be inclined to just take this patch, because it fixes a bug and
> > the x11 backend is of lesser quality anyway.
> 
> Hmmm, I think the only event that matters is XCB_CLIENT_MESSAGE (delete
> window).  Can't I just handle that from an idle callback?

I suppose, but is it worth it? :-)


Thanks,
pq
Hi,

On 19 December 2014 at 15:02, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> On Fri, 19 Dec 2014 08:08:33 -0600
> Derek Foreman <derekf@osg.samsung.com> wrote:
>> On 19/12/14 06:40 AM, Pekka Paalanen wrote:
>> > There is some fancy input scheduling going on with the repaint loop, so
>> > that input events would be sent as a burst to the clients when the
>> > compositor is spinning in the repaint loop. The idea is that because
>> > clients repaint only once per cycle, they don't need to be woken up due
>> > to input any more often than that. Or that's how I understood it.
>> >
>> > The problem here, I assume, is that the X11 fd is not only input, but
>> > also output events, unlike DRM backend where the DRM fd is separate.
>> >
>> > I would be inclined to just take this patch, because it fixes a bug and
>> > the x11 backend is of lesser quality anyway.
>>
>> Hmmm, I think the only event that matters is XCB_CLIENT_MESSAGE (delete
>> window).  Can't I just handle that from an idle callback?
>
> I suppose, but is it worth it? :-)

Yeah, I think it'd be worth doing so. Especially if we want to run
tests on the X11 backend, or rely on it not crashing ...

The idle-handler thing sounds good to me.

Cheers,
Daniel