[PATCH xserver v2] xwayland: add envvar XWAYLAND_NO_GLAMOR
Not all compositors allow for customizing the Xwayland command line,
gnome-shell/mutter for example have the command line and path to
Xwayland binary hardcoded, which makes it harder for users to disable
glamor acceleration in Xwayland (glamor being used by default).
Add an environment variable XWAYLAND_NO_GLAMOR to disable glamor support
in Xwayland.
Signed-off-by: Olivier Fourdan
Reviewed-by: Eric Engestrom
---
v2: Fix typo in commit message, add r-b Eric Engestrom
hw/xwayland/xwayland-glamor.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index 65c3c00..ee30f81 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -552,6 +552,13 @@ Bool
xwl_glamor_init(struct xwl_screen *xwl_screen)
{
ScreenPtr screen = xwl_screen->screen;
+const char *no_glamor_env;
+
+no_glamor_env = getenv("XWAYLAND_NO_GLAMOR");
+if (no_glamor_env && *no_glamor_env != '0') {
+ErrorF("Disabling glamor and dri3 support, XWAYLAND_NO_GLAMOR is
set\n");
+return FALSE;
+}
if (xwl_screen->egl_context == EGL_NO_CONTEXT) {
ErrorF("Disabling glamor and dri3, EGL setup failed\n");
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] xwayland: add envvar XWAYLAND_NO_GLAMOR
Hi Eric, > > Not all compositors allow for customizing the Xwayland command line, > > gnome-shell/mutter for example have the command line and path to > > Xwayland binary hardcoded, which makes it harder for users to disable > > glamor acceleration in Xwayland (glamor being used by default). > > > > Add an environment variable XWAYLAND_NO_GLAMOR t odiable glamor support > > "to disable" I can't type... v2 with an updated commit message is on its way. > The change itself looks good to me. > Reviewed-by: Eric Engestrom Thanks! > As to whether it's a good idea to allow this, I'd say it is, but you > might want the opinion of someone who's more involved in this project :) Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[RFC PATCH xserver] xwayland: make sure client is not gone in sync callback
in XWayland, dri3_send_open_reply() is called from a sync callback, so
there is a possibility that the client might be gone when we get to the
callback eventually, which leads to a crash in _XSERVTransSendFd() from
WriteFdToClient() .
Check if clientGone has been set in the sync callback handler to avoid
this.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99149
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1416553
Signed-off-by: Olivier Fourdan
---
This seems to be a fairly rare occurence, but we do have bugs filed both
upstream and downstream for this.
I don't have any core file unfortunately so this is based solely on
the addresses returned by the crash handler, thus the "RFC" on this
patch...
hw/xwayland/xwayland-glamor.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index b3d0aab..65c3c00 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -435,9 +435,12 @@ static void
sync_callback(void *data, struct wl_callback *callback, uint32_t serial)
{
struct xwl_auth_state *state = data;
+ClientPtr client = state->client;
-dri3_send_open_reply(state->client, state->fd);
-AttendClient(state->client);
+if (!client->clientGone) {
+dri3_send_open_reply(client, state->fd);
+AttendClient(client);
+}
free(state);
wl_callback_destroy(callback);
}
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: xserver 1.19.2 call for patches
Hi Adam, > I'd like to do another 1.19 soonish, say middle of next week. If you've > got favorite patches you'd like to see included, please write their > sha1s on the back of a $50 bill and send them to me. > > Alternatively, reply to this email, or send a pull request. I would like to add more, if I may... Those are a couple of Xwayland bug fixes that might be beneficial in 1.19 as well: 8c9909a xwayland: Make sure we have a focus window fe5c340 xwayland: do not set checkRepeat on master kbd Thanks Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[RFC PATCH xserver] xwayland: RFC Disable glamor with an env var?
Hi all, I am seeing quite a few Xwayland crashes related to glamor. Various issues, could be with glamor itself or with the drivers (like the issues I witness with nv30), whatever. To investigate those issues (or even unblock affected users) it's often useful to disable glamor -even temporarily- but Xwayland doesn't use an xorg.conf and relies solely on the command line. Not all compositors allow for customizing the command line, gnome-shell or mutter for example have the command line and path to the Xwayland binary hardcoded, which makes it harder for users to disable glamor acceleration in Xwayland by adding -shm to the command line (glamor being used by default). Would adding an environment variable such as XWAYLAND_NO_GLAMOR (similar to what Xephyr does with e.g. XEPHYR_NO_SHM) make sense here? I tried and it works with the following patch added, by setting the environment variable XWAYLAND_NO_GLAMOR in a /etc/profile.d/ script on Fedora 25. Cheers, Olivier Olivier Fourdan (1): xwayland: add envvar XWAYLAND_NO_GLAMOR hw/xwayland/xwayland-glamor.c | 7 +++ 1 file changed, 7 insertions(+) -- 2.9.3 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: add envvar XWAYLAND_NO_GLAMOR
Not all compositors allow for customizing the Xwayland command line,
gnome-shell/mutter for example have the command line and path to
Xwayland binary hardcoded, which makes it harder for users to disable
glamor acceleration in Xwayland (glamor being used by default).
Add an environment variable XWAYLAND_NO_GLAMOR t odiable glamor support
in Xwayland.
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index b3d0aab..45de54f 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -549,6 +549,13 @@ Bool
xwl_glamor_init(struct xwl_screen *xwl_screen)
{
ScreenPtr screen = xwl_screen->screen;
+const char *no_glamor_env;
+
+no_glamor_env = getenv("XWAYLAND_NO_GLAMOR");
+if (no_glamor_env && *no_glamor_env != '0') {
+ErrorF("Disabling glamor and dri3 support, XWAYLAND_NO_GLAMOR is
set\n");
+return FALSE;
+}
if (xwl_screen->egl_context == EGL_NO_CONTEXT) {
ErrorF("Disabling glamor and dri3, EGL setup failed\n");
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: do not set checkRepeat on master kbd
If a key event is sent programmatically, the virtual core keyboard does
not have its dev->public.devicePrivate set and we can't get the Xwayland
seat, in which case we end up crashing in keyboard_check_repeat().
This is the case with "antimicro" which sends key events based on the
joystick buttons.
Adding the checkRepeat handler on the VCK is useless anyway, so remove
it and avoid the crash in keyboard_check_repeat() when trying to get the
Xwayland seat.
Bugzilla: https://bugzilla.redhat.com/1416244
Signed-off-by: Olivier Fourdan
---
v2: Avoid the crash by not setting the checkRepeat handler on the master
keyboard - For some reason, I was convinced it was needed when I sent
the patch for commit 239705a (xwayland: add a server sync before
repeating keys) but on further consideration, I don't see how it
could be, considering we cannot get to the xwl seat anyway.
Checked that removing it does not affect the key repeat mechanism
when the compositor is busy, it works equally well without this so
all is fine... My bad, thanks Peter!
hw/xwayland/xwayland-input.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index 2ca99d9..1f5d323 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -1027,8 +1027,6 @@ release_relative_pointer(struct xwl_seat *xwl_seat)
static void
init_keyboard(struct xwl_seat *xwl_seat)
{
-DeviceIntPtr master;
-
xwl_seat->wl_keyboard = wl_seat_get_keyboard(xwl_seat->seat);
wl_keyboard_add_listener(xwl_seat->wl_keyboard,
&keyboard_listener, xwl_seat);
@@ -1040,9 +1038,6 @@ init_keyboard(struct xwl_seat *xwl_seat)
}
EnableDevice(xwl_seat->keyboard, TRUE);
xwl_seat->keyboard->key->xkbInfo->checkRepeat = keyboard_check_repeat;
-master = GetMaster(xwl_seat->keyboard, MASTER_KEYBOARD);
-if (master)
-master->key->xkbInfo->checkRepeat = keyboard_check_repeat;
}
static void
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: Make sure we have a focus window
During the InitInput() phase, the wayland events get dequeued so we
can possibly end up calling dispatch_pointer_motion_event().
If this occurs before xwl_seat->focus_window is set, it leads to a NULL
pointer derefence and a segfault.
Check for xwl_seat->focus_window in both pointer_handle_frame() and
relative_pointer_handle_relative_motion() prior to calling
dispatch_pointer_motion_event() like it's done in
pointer_handle_motion().
Bugzilla: https://bugzilla.redhat.com/1410804
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-input.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index 8435da0..9c1581f 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -510,6 +510,9 @@ pointer_handle_frame(void *data, struct wl_pointer
*wl_pointer)
{
struct xwl_seat *xwl_seat = data;
+if (!xwl_seat->focus_window)
+return;
+
dispatch_pointer_motion_event(xwl_seat);
}
@@ -560,6 +563,9 @@ relative_pointer_handle_relative_motion(void *data,
xwl_seat->pending_pointer_event.dx_unaccel =
wl_fixed_to_double(dx_unaccelf);
xwl_seat->pending_pointer_event.dy_unaccel =
wl_fixed_to_double(dy_unaccelf);
+if (!xwl_seat->focus_window)
+return;
+
if (wl_proxy_get_version((struct wl_proxy *) xwl_seat->wl_pointer) < 5)
dispatch_pointer_motion_event(xwl_seat);
}
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] xwayland: Do not assume we have a seat
Hi Peter,
> what device is this called for? quick skim of the xwayland sources indicates
> that a device is only created if we have a seat, so I wonder if the repeat is
> called for the wrong device here?
The keyboard_check_repeat() handler is called directly from
AccessXRepeatKeyExpire() [1].
It's set in init_keyboard() [2] on the seat's keyboard and on the master
keyboard as well.
IIRC that was needed otherwise the keyboard_check_repeat() would not avoid
spurious key repeats in all cases when the compositor becomes "busy".
The device that causes the issue is the virtual core keyboard:
(gdb) bt
#0 keyboard_check_repeat (dev=0x3092040, xkbi=0x30932b0, key=111)
at xwayland-input.c:751
#1 0x005245e7 in AccessXRepeatKeyExpire (
timer=, now=, arg=0x3092040)
at xkbAccessX.c:321
#2 0x0058aa40 in DoTimer (timer=0x35fc3c0,
now=now@entry=68761551) at WaitFor.c:294
#3 0x0058aab8 in DoTimers (now=68761551) at WaitFor.c:308
#4 0x0058adf7 in check_timers () at WaitFor.c:151
#5 WaitForSomething (are_ready=) at WaitFor.c:217
#6 0x0055660a in Dispatch () at dispatch.c:422
#7 0x0055a858 in dix_main (argc=10, argv=0x7fffc3408528,
envp=) at main.c:287
#8 0x7fda223c7401 in __libc_start_main (main=0x423d80 ,
argc=10, argv=0x7fffc3408528, init=,
fini=, rtld_fini=,
stack_end=0x7fffc3408518) at ../csu/libc-start.c:289
#9 0x00423dba in _start ()
(gdb) p *dev
$2 = {public = {devicePrivate = 0x0,
processInputProc = 0x52c870 ,
realInputProc = 0x52c870 ,
enqueueInputProc = 0x55fa10 , on = 0},
next = 0x309fff0, startup = 1,
deviceProc = 0x54a820 , inited = 1, enabled = 1,
coreEvents = 1, deviceGrab = {grabTime = {months = 0,
milliseconds = 86921}, fromPassiveGrab = 0, implicitGrab = 0,
unused = 0x0, grab = 0x0, activatingKey = 0 '\000',
ActivateGrab = 0x566bd0 ,
DeactivateGrab = 0x566f90 , sync = {
frozen = 0, state = 0, other = 0x0, event = 0x3092400}},
type = 2, xinput_type = 0,
name = 0x30926a0 "Virtual core keyboard", id = 3, key = 0x30931c0,
valuator = 0x0, touch = 0x0, button = 0x0, focus = 0x309f380,
proximity = 0x0, kbdfeed = 0x3093240, ptrfeed = 0x0, intfeed = 0x0,
stringfeed = 0x0, bell = 0x0, leds = 0x0, xkb_interest = 0x3714540,
config_info = 0x0, unused_classes = 0x30926c0, saved_master_id = 0,
devPrivates = 0x30923d0, unwrapProc = 0x52ae60 ,
spriteInfo = 0x3092398, master = 0x0, lastSlave = 0x30a0650,
last = {valuators = {0 }, numValuators = 0,
slave = 0x30a0650, scroll = 0x0, num_touches = 0, touches = 0x0},
properties = {properties = 0x3092610, handlers = 0x3092670},
relative_transform = {m = {{1, 0, 0}, {0, 1, 0}, {0, 0, 1}}},
scale_and_transform = {m = {{1, 0, 0}, {0, 1, 0}, {0, 0, 1}}},
xtest_master_id = 0, idle_counter = 0x309f5d0}
Cheers,
Olivier
[1] https://cgit.freedesktop.org/xorg/xserver/tree/xkb/xkbAccessX.c#n312
[2]
https://cgit.freedesktop.org/xorg/xserver/tree/hw/xwayland/xwayland-input.c#n1021
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: Do not assume we have a seat
If a key event is sent programmatically, we may not have an Xwayland
seat associated with it, in which case we end up crashing in
keyboard_check_repeat().
This is the case with "antimicro" which sends key events based on the
joystick buttons.
Avoid the NULL pointer dereference by first checking if the xwl_seat is
non-NULL.
Bugzilla: https://bugzilla.redhat.com/1416244
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-input.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index cc83ef8..8435da0 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -742,10 +742,16 @@ static Bool
keyboard_check_repeat (DeviceIntPtr dev, XkbSrvInfoPtr xkbi, unsigned key)
{
struct xwl_seat *xwl_seat = dev->public.devicePrivate;
-struct xwl_screen *xwl_screen = xwl_seat->xwl_screen;
+struct xwl_screen *xwl_screen;
struct wl_callback *callback;
struct sync_pending *p;
+/* If we do not have an xwl_seat, it's not coming from the compositor */
+if (!xwl_seat)
+ return TRUE;
+
+xwl_screen = xwl_seat->xwl_screen;
+
/* Make sure we didn't miss a possible reply from the compositor */
xwl_sync_events (xwl_screen);
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: xserver 1.19.2 call for patches
Hi, > I'd like to do another 1.19 soonish, say middle of next week. If you've > got favorite patches you'd like to see included, please write their > sha1s on the back of a $50 bill and send them to me. > > Alternatively, reply to this email, or send a pull request. FWIW I reckon we should also pick Peter's patch to avoid a WriteToClient() from the input thread to the forthcoming 1.19.2 release: 1b12249 os: log a bug whenever WriteToClient is called from the input thread Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2 xserver] os: log a bug whenever WriteToClient is called from the input thread
> The input thread should generate events, not send them. Make it easier to
> find the instances where it's doing so.
>
> Signed-off-by: Peter Hutterer
> Tested-by: Olivier Fourdan
> ---
> Changes to v1:
> - add check for InputThreadInfo to avoid null-pointer dereference
> - remove leftover declaration for in_input_thread
>
> include/input.h | 1 +
> os/inputthread.c | 8
> os/io.c | 3 +++
> 3 files changed, 12 insertions(+)
>
> diff --git a/include/input.h b/include/input.h
> index bb58b22..6c9e45d 100644
> --- a/include/input.h
> +++ b/include/input.h
> @@ -722,6 +722,7 @@ extern _X_HIDDEN void input_constrain_cursor(DeviceIntPtr
> pDev, ScreenPtr screen
> extern _X_EXPORT void input_lock(void);
> extern _X_EXPORT void input_unlock(void);
> extern _X_EXPORT void input_force_unlock(void);
> +extern _X_EXPORT int in_input_thread(void);
>
> extern void InputThreadPreInit(void);
> extern void InputThreadInit(void);
> diff --git a/os/inputthread.c b/os/inputthread.c
> index 4400fba..e7159c7 100644
> --- a/os/inputthread.c
> +++ b/os/inputthread.c
> @@ -90,6 +90,13 @@ static pthread_mutex_t input_mutex;
> static Bool input_mutex_initialized;
> #endif
>
> +int
> +in_input_thread(void)
> +{
> +return inputThreadInfo &&
> + pthread_equal(pthread_self(), inputThreadInfo->thread);
> +}
> +
> void
> input_lock(void)
> {
> @@ -531,6 +538,7 @@ void input_force_unlock(void) {}
> void InputThreadPreInit(void) {}
> void InputThreadInit(void) {}
> void InputThreadFini(void) {}
> +int in_input_thread(void) { return 0; }
>
> int InputThreadRegisterDev(int fd,
> NotifyFdProcPtr readInputProc,
> diff --git a/os/io.c b/os/io.c
> index be85226..8aa51a1 100644
> --- a/os/io.c
> +++ b/os/io.c
> @@ -651,6 +651,9 @@ WriteToClient(ClientPtr who, int count, const void
> *__buf)
> int padBytes;
> const char *buf = __buf;
>
> +BUG_RETURN_VAL_MSG(in_input_thread(), 0,
> + " %s called from input thread *\n",
> __func__);
> +
> #ifdef DEBUG_COMMUNICATION
> Bool multicount = FALSE;
> #endif
Looks good to me :)
Reviewed-by: Olivier Fourdan
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [RFC xserver] os: log a bug whenever WriteToClient is called from the input thread
Hi, > > +extern inline int > > +in_input_thread(void); > > + > > maybe i am old fashion but this should be done with input.h, should it ? No, I don't think this is possible (in the middle of a stable release) nor even suitable, it's not a new API intended for drivers to use, it's purely internal to the Xserver to tell if it's safe to send data to clients (i.e. if we are on the main thread and not in the input thread) Cheers, Olivier. ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [RFC xserver] os: log a bug whenever WriteToClient is called from the input thread
Hi,
> > The input thread should generate events, not send them. Make it easier to
> > find the instances where it's doing so.
> >
> > Signed-off-by: Peter Hutterer
> > ---
> > include/input.h | 1 +
> > os/inputthread.c | 7 +++
> > os/io.c | 6 ++
> > 3 files changed, 14 insertions(+)
> >
> > diff --git a/include/input.h b/include/input.h
> > index bb58b22..6c9e45d 100644
> > --- a/include/input.h
> > +++ b/include/input.h
> > @@ -722,6 +722,7 @@ extern _X_HIDDEN void
> > input_constrain_cursor(DeviceIntPtr
> > pDev, ScreenPtr screen
> > extern _X_EXPORT void input_lock(void);
> > extern _X_EXPORT void input_unlock(void);
> > extern _X_EXPORT void input_force_unlock(void);
> > +extern _X_EXPORT int in_input_thread(void);
> >
> > extern void InputThreadPreInit(void);
> > extern void InputThreadInit(void);
> > diff --git a/os/inputthread.c b/os/inputthread.c
> > index 4400fba..933bc1c 100644
> > --- a/os/inputthread.c
> > +++ b/os/inputthread.c
> > @@ -90,6 +90,12 @@ static pthread_mutex_t input_mutex;
> > static Bool input_mutex_initialized;
> > #endif
> >
> > +int
> > +in_input_thread(void)
> > +{
> > +return pthread_equal(pthread_self(), inputThreadInfo->thread);
> > +}
> > +
> > void
> > input_lock(void)
> > {
> > @@ -531,6 +537,7 @@ void input_force_unlock(void) {}
> > void InputThreadPreInit(void) {}
> > void InputThreadInit(void) {}
> > void InputThreadFini(void) {}
> > +int in_input_thread(void) { return 0; }
> >
> > int InputThreadRegisterDev(int fd,
> > NotifyFdProcPtr readInputProc,
> > diff --git a/os/io.c b/os/io.c
> > index be85226..5494b30 100644
> > --- a/os/io.c
> > +++ b/os/io.c
> > @@ -643,6 +643,9 @@ SetCriticalOutputPending(void)
> > *this routine as int.
> > */
> >
> > +extern inline int
> > +in_input_thread(void);
> > +
> > int
> > WriteToClient(ClientPtr who, int count, const void *__buf)
> > {
> > @@ -651,6 +654,9 @@ WriteToClient(ClientPtr who, int count, const void
> > *__buf)
> > int padBytes;
> > const char *buf = __buf;
> >
> > +BUG_RETURN_VAL_MSG(in_input_thread(), 0,
> > + " %s called from input thread *\n",
> > __func__);
> > +
> > #ifdef DEBUG_COMMUNICATION
> > Bool multicount = FALSE;
> > #endif
> > --
> > 2.9.3
>
> It works a treat, brilliant - Within not even a second approaching the stylus
> to the tablet, Xorg was logging messages about calling WriteToClient() from
> the input thread, it's just awesome!
>
> Tested-by: Olivier Fourdan
>
> And if I'd dare, fwiw, it looks good to me.
>
> Reviewed-by: Olivier Fourdan
^^^
Err, nope! I take that back, sorry, looks like it's causing trouble to
Xwayland... it crashes.
> +int
> +in_input_thread(void)
> +{
> +return pthread_equal(pthread_self(), inputThreadInfo->thread);
> +}
If this gets called *before* the InputThread is even created, inputThreadInfo
is NULL.
I would add a
if (!inputThreadInfo)
return 0;
Otherwise Xwayland crashes at startup. With something like that added, it works
:)
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] configure: Xephyr, xfake and xfbdev require kdrive
Raise an error at configure time if one of the DDX that require kdrive is enabled but kdrive itself is not. That avoids the build to silently ignore Xephyr, Xfake and Xfbdev if --enable-kdrive is not set as well. Signed-off-by: Olivier Fourdan --- configure.ac | 11 +++ 1 file changed, 11 insertions(+) diff --git a/configure.ac b/configure.ac index e291b34..d6b8416 100644 --- a/configure.ac +++ b/configure.ac @@ -2457,6 +2457,17 @@ if test "$KDRIVE" = yes; then AC_SUBST([XEPHYR_LIBS]) AC_SUBST([XEPHYR_INCS]) +else +dnl Make sure none of the kdrive dependent servers are enabled if kdrive is not +if test "$XEPHYR" = yes; then +AC_MSG_ERROR([Xephyr requires kdrive.]) +fi +if test "$XFAKE" = yes; then +AC_MSG_ERROR([fake server requires kdrive.]) +fi +if test "$XFBDEV" = yes; then +AC_MSG_ERROR([framebuffer device server requires kdrive.]) +fi fi AC_SUBST([KDRIVE_INCS]) AC_SUBST([KDRIVE_PURE_INCS]) -- 2.9.3 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [RFC xserver] os: log a bug whenever WriteToClient is called from the input thread
> The input thread should generate events, not send them. Make it easier to
> find the instances where it's doing so.
>
> Signed-off-by: Peter Hutterer
> ---
> include/input.h | 1 +
> os/inputthread.c | 7 +++
> os/io.c | 6 ++
> 3 files changed, 14 insertions(+)
>
> diff --git a/include/input.h b/include/input.h
> index bb58b22..6c9e45d 100644
> --- a/include/input.h
> +++ b/include/input.h
> @@ -722,6 +722,7 @@ extern _X_HIDDEN void input_constrain_cursor(DeviceIntPtr
> pDev, ScreenPtr screen
> extern _X_EXPORT void input_lock(void);
> extern _X_EXPORT void input_unlock(void);
> extern _X_EXPORT void input_force_unlock(void);
> +extern _X_EXPORT int in_input_thread(void);
>
> extern void InputThreadPreInit(void);
> extern void InputThreadInit(void);
> diff --git a/os/inputthread.c b/os/inputthread.c
> index 4400fba..933bc1c 100644
> --- a/os/inputthread.c
> +++ b/os/inputthread.c
> @@ -90,6 +90,12 @@ static pthread_mutex_t input_mutex;
> static Bool input_mutex_initialized;
> #endif
>
> +int
> +in_input_thread(void)
> +{
> +return pthread_equal(pthread_self(), inputThreadInfo->thread);
> +}
> +
> void
> input_lock(void)
> {
> @@ -531,6 +537,7 @@ void input_force_unlock(void) {}
> void InputThreadPreInit(void) {}
> void InputThreadInit(void) {}
> void InputThreadFini(void) {}
> +int in_input_thread(void) { return 0; }
>
> int InputThreadRegisterDev(int fd,
> NotifyFdProcPtr readInputProc,
> diff --git a/os/io.c b/os/io.c
> index be85226..5494b30 100644
> --- a/os/io.c
> +++ b/os/io.c
> @@ -643,6 +643,9 @@ SetCriticalOutputPending(void)
> *this routine as int.
> */
>
> +extern inline int
> +in_input_thread(void);
> +
> int
> WriteToClient(ClientPtr who, int count, const void *__buf)
> {
> @@ -651,6 +654,9 @@ WriteToClient(ClientPtr who, int count, const void
> *__buf)
> int padBytes;
> const char *buf = __buf;
>
> +BUG_RETURN_VAL_MSG(in_input_thread(), 0,
> + " %s called from input thread *\n",
> __func__);
> +
> #ifdef DEBUG_COMMUNICATION
> Bool multicount = FALSE;
> #endif
> --
> 2.9.3
It works a treat, brilliant - Within not even a second approaching the stylus
to the tablet, Xorg was logging messages about calling WriteToClient() from the
input thread, it's just awesome!
Tested-by: Olivier Fourdan
And if I'd dare, fwiw, it looks good to me.
Reviewed-by: Olivier Fourdan
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH RFC xserver] os: Add a mutex to protect io buffers
Hi Alan, > >>> WriteToClient() can be called from XIChangeDeviceProperty() so from the > >>> InputThread which is a problem as it allocates and free the input and > >>> output buffers. > > > > That seems like a bug to me; the input thread isn't supposed to be > > directly interacting with clients. Instead, it should queue a suitable > > work proc and have the events delivered from there. This will ensure > > proper serialization with request processing so that the right serial > > numbers and other data are inserted. > > Should WriteToClient() have something like > assert(pthread_self() == mainThread) > added to help catch things like that? I very much like this idea, those bugs are a pain to trace... Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH RFC xserver] os: Add a mutex to protect io buffers
Hi Peter, > > > --- > > > RFC: This is probably sub-optimal and broken in many places, but it > > > seems to avoid the memory corruption (so far)... At least it's a > > > start, I guess. > > > > It's certainly an improvement in correctness, I just hate it. ;) > > > > One problem with this is there are some requests (GetImage in > > particular) whose reply is split up into multiple calls to > > WriteToClient, which means if an XICDP call happens on the input thread > > while GetImage is writing, the events will be interleaved with the > > reply data. In addition to corrupting the returned image, the client > > will almost certainly choke and die while attempting to process the > > next reply/event. > > > > We can paper over that by fixing the WriteToClient callers to > > (recursively) take the io lock as well. But doing so highlights a > > design issue with this approach. GetImage replies are slow because > > they're a lot of data, so now (if you hit the same XICDP path) you've > > made the input thread _block_ until the request completes, and the > > mouse will get stuck. > > > > What I'd like to see is the events built asynchronously and flushed > > from the main thread (possibly only if we can tell we're running on the > > input thread). > > unless I'm confused about the thread, it's intention was to replace the > sigio handler. So while it updates the pointer, it never actually *sends* > events, it merely generates them and shoves them into the EQ. The events are > then taken from there in the main thread and processed. From the backtraces I got, it does send them, see that backtrace I captured yesterday: #0 0x06f8b91f in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:58 #1 0x06f8d51a in __GI_abort () at abort.c:89 #2 0x005a156e in OsAbort () at utils.c:1355 #3 0x005a7163 in AbortServer () at log.c:877 #4 0x005a7f4d in FatalError (f=f@entry=0xaaf8ecf "%s:%d assertion '%s' failed\n") at log.c:1015 #5 0x0a974e36 in _kgem_submit (kgem=kgem@entry=0x9617000) at kgem.c:4134 #6 0x0a9b9f64 in kgem_submit (kgem=0x9617000) at kgem.h:382 #7 0x0a9b9f64 in sna_accel_flush (sna=sna@entry=0x850800 ) at sna_accel.c:17375 #8 0x0a9b9fec in sna_flush_callback (list=, user_data=0x850800 , call_data=) at sna_accel.c:17399 #9 0x0043c3e4 in _CallCallbacks (pcbl=0x36dc6b30, pcbl@entry=0x850800 , call_data=0x20, call_data@entry=0xb0bc150) at dixutils.c:737 #10 0x0059dbd3 in CallCallbacks (call_data=0xb0bc150, pcbl=0x850800 ) at ../include/callback.h:83 #11 0x0059dbd3 in FlushClient (who=who@entry=0xb0bc150, oc=oc@entry=0x9552800, __extraBuf=__extraBuf@entry=0x11027a60, extraCount=extraCount@entry=32) at io.c:809 #12 0x0059e13f in WriteToClient (who=who@entry=0xb0bc150, count=count@entry=32, __buf=__buf@entry=0x11027a60) at io.c:763 #13 0x00442572 in WriteEventsToClient (pClient=pClient@entry=0xb0bc150, count=, count@entry=1, events=events@entry=0x11027a60) at events.c:6000 #14 0x00442710 in TryClientEvents (client=0xb0bc150, dev=, pEvents=0x11027a60, count=1, mask=, filter=, grab=0x0) at events.c:2021 #15 0x00445e7a in DeliverEventToInputClients (dev=dev@entry=0xfc85870, inputclients=0xd5726f0, win=win@entry=0xb2fed40, events=events@entry=0x11027a60, count=count@entry=1, filter=filter@entry=16, grab=0x0, client_return=0x11027998, mask_return=0x11027994) at events.c:2170 #16 0x00446167 in DeliverEventToWindowMask (mask_return=0x11027994, client_return=0x11027998, grab=0x0, filter=16, count=1, events=0x11027a60, win=0xb2fed40, dev=0xfc85870) at events.c:2213 #17 0x00446167 in DeliverEventsToWindow (pDev=pDev@entry=0xfc85870, pWin=pWin@entry=0xb2fed40, pEvents=pEvents@entry=0x11027a60, count=count@entry=1, filter=filter@entry=16, grab=grab@entry=0x0) at events.c:2277 #18 0x005283b9 in SendEventToAllWindows (dev=dev@entry=0xfc85870, mask=16, ev=ev@entry=0x11027a60, count=count@entry=1) at exevents.c:3045 #19 0x00533d1f in send_property_event (dev=dev@entry=0xfc85870, property=, what=) at xiproperty.c:208 #20 0x005346d8 in XIChangeDeviceProperty (dev=0xfc85870, property=, type=, format=, mode=, len=, value=0x11027b50, sendevent=1) at xiproperty.c:802 #21 0x0fe1d039 in wcmUpdateSerial () at /usr/lib64/xorg/modules/input/wacom_drv.so #22 0x0fe131c2 in wcmSendEvents () at /usr/lib64/xorg/modules/input/wacom_drv.so #23 0x0fe14764 in wcmEvent () at /usr/lib64/xorg/modules/input/wacom_drv.so #24 0x0fe1a476 in usbParse () at /usr/lib64/xorg/modules/input/wacom_drv.so #25 0x0fe11feb in wcmReadPacket () at /usr/lib64/xorg/modules/input/wacom_drv.so #26 0x0fe12226 in wcmDevReadInput () at /usr/lib64/xorg/modules/input/wacom_drv.so #27 0x0059cc8c in InputReady (fd=32, xevents=1, data=0xfc94e90) at inputthread.c:173 #28 0x0059f27
[PATCH RFC xserver] os: Add a mutex to protect io buffers
WriteToClient() can be called from XIChangeDeviceProperty() so from the
InputThread which is a problem as it allocates and free the input and
output buffers.
Add a new mutex to protect accesses to the input and output buffers from
being accessed from different threads.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99164
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99887
Signed-off-by: Olivier Fourdan
---
RFC: This is probably sub-optimal and broken in many places, but it
seems to avoid the memory corruption (so far)... At least it's a
start, I guess.
os/io.c | 73 ++---
1 file changed, 66 insertions(+), 7 deletions(-)
diff --git a/os/io.c b/os/io.c
index be85226..b0e3825 100644
--- a/os/io.c
+++ b/os/io.c
@@ -107,6 +107,36 @@ static ConnectionInputPtr FreeInputs =
(ConnectionInputPtr) NULL;
static ConnectionOutputPtr FreeOutputs = (ConnectionOutputPtr) NULL;
static OsCommPtr AvailableInput = (OsCommPtr) NULL;
+/* iobuf_mutex code copied from inputthread.c */
+#ifdef PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP
+static pthread_mutex_t iobuf_mutex = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
+#else
+static pthread_mutex_t iobuf_mutex;
+static Bool iobuf_mutex_initialized;
+#endif
+
+static void
+iobuf_lock(void)
+{
+#ifndef PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP
+if (!iobuf_mutex_initialized) {
+pthread_mutexattr_t mutex_attr;
+
+iobuf_mutex_initialized = TRUE;
+pthread_mutexattr_init(&mutex_attr);
+pthread_mutexattr_settype(&mutex_attr, PTHREAD_MUTEX_RECURSIVE);
+pthread_mutex_init(&iobuf_mutex, &mutex_attr);
+}
+#endif
+pthread_mutex_lock(&iobuf_mutex);
+}
+
+static void
+iobuf_unlock(void)
+{
+pthread_mutex_unlock(&iobuf_mutex);
+}
+
#define get_req_len(req,cli) ((cli)->swapped ? \
lswaps((req)->length) : (req)->length)
@@ -203,6 +233,7 @@ YieldControlDeath(void)
static void
NextAvailableInput(OsCommPtr oc)
{
+iobuf_lock();
if (AvailableInput) {
if (AvailableInput != oc) {
ConnectionInputPtr aci = AvailableInput->input;
@@ -219,6 +250,7 @@ NextAvailableInput(OsCommPtr oc)
}
AvailableInput = NULL;
}
+iobuf_unlock();
}
int
@@ -237,12 +269,14 @@ ReadRequestFromClient(ClientPtr client)
/* make sure we have an input buffer */
+iobuf_lock();
if (!oci) {
if ((oci = FreeInputs)) {
FreeInputs = oci->next;
}
else if (!(oci = AllocateInputBuffer())) {
YieldControlDeath();
+iobuf_unlock();
return -1;
}
oc->input = oci;
@@ -313,6 +347,7 @@ ReadRequestFromClient(ClientPtr client)
*/
oci->ignoreBytes = needed - gotnow;
oci->lenLastReq = gotnow;
+iobuf_unlock();
return needed;
}
if ((gotnow == 0) || ((oci->bufptr - oci->buffer + needed) >
oci->size)) {
@@ -346,6 +381,7 @@ ReadRequestFromClient(ClientPtr client)
* used to happen
*/
YieldControlDeath();
+iobuf_unlock();
return -1;
}
result = _XSERVTransRead(oc->trans_conn, oci->buffer + oci->bufcnt,
@@ -358,10 +394,12 @@ ReadRequestFromClient(ClientPtr client)
#endif
{
YieldControlNoInput(fd);
+iobuf_unlock();
return 0;
}
}
YieldControlDeath();
+iobuf_unlock();
return -1;
}
oci->bufcnt += result;
@@ -395,6 +433,7 @@ ReadRequestFromClient(ClientPtr client)
if (gotnow < needed) {
/* Still don't have enough; punt. */
YieldControlNoInput(fd);
+iobuf_unlock();
return 0;
}
}
@@ -447,6 +486,8 @@ ReadRequestFromClient(ClientPtr client)
client->req_len -= bytes_to_int32(sizeof(xBigReq) - sizeof(xReq));
}
client->requestBuffer = (void *) oci->bufptr;
+iobuf_unlock();
+
#ifdef DEBUG_COMMUNICATION
{
xReq *req = client->requestBuffer;
@@ -498,12 +539,14 @@ InsertFakeRequest(ClientPtr client, char *data, int count)
int gotnow, moveup;
NextAvailableInput(oc);
-
+iobuf_lock();
if (!oci) {
if ((oci = FreeInputs))
FreeInputs = oci->next;
-else if (!(oci = AllocateInputBuffer()))
+else if (!(oci = AllocateInputBuffer())) {
+iobuf_unlock();
return FALSE;
+}
oc->input = oci;
}
oci->bufptr += oci->lenLastReq;
@@ -513,8 +556,10 @@ InsertFakeRequest(ClientPtr client, char *data, int count)
char *ibuf;
ibuf = (char *) realloc(oci->buffer, gotnow + count);
-
Re: [PATCH xserver] Xi: Hold the input lock when sending events
Hi, > Otherwise the output buffer will be accessed from different threads and > cause all sorts of nasty and painful memory corruptions. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99164 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99887 > Signed-off-by: Olivier Fourdan > --- > Xi/xiproperty.c | 2 ++ > 1 file changed, 2 insertions(+) That doesn't fix the issue, unfortunately. But I am convinced the problem comes from calling eventually WriteToClients() from the InputThread, which can lead to various memory corruption either from the input or output buffers or from the CallCallbacks() as seen in some of the backtraces. So I'm tempted to put all that code within its own mutex so it's protected from concurrent access. I have a patch for and the first tests seems to show it avoids the crash, I've been testing it successfully for several minutes whereas before it would crash whithin seconds with my reproducer steps. Of course it's not a proof, but... I'll post that patch as an RFC... Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] Xi: Hold the input lock when sending events
Otherwise the output buffer will be accessed from different threads and cause all sorts of nasty and painful memory corruptions. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99164 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99887 Signed-off-by: Olivier Fourdan --- Xi/xiproperty.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Xi/xiproperty.c b/Xi/xiproperty.c index b7a1f59..a68b4b3 100644 --- a/Xi/xiproperty.c +++ b/Xi/xiproperty.c @@ -203,10 +203,12 @@ send_property_event(DeviceIntPtr dev, Atom property, int what) .what = what }; +input_lock(); SendEventToAllWindows(dev, DevicePropertyNotifyMask, (xEvent *) &event, 1); SendEventToAllWindows(dev, GetEventFilter(dev, (xEvent *) &xi2), (xEvent *) &xi2, 1); +input_unlock(); } static int -- 2.9.3 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: xserver 1.19.2 call for patches
Hi Michel, - Original Message - > On 20/02/17 06:44 PM, Olivier Fourdan wrote: > > - Original Message - > >> I'd like to do another 1.19 soonish, say middle of next week. > > > > That would be great if we could get to the bottom of the random > > crashes that affect 1.19.x first. > > > > Good news is we now have a core file from downstream bug > > https://bugzilla.redhat.com/show_bug.cgi?id=1424644 > > > > I'm struggling to find the problem so if anyone else has any idea, > > I'm all hear. > > I also got stuck analyzing the valgrind logs in > https://bugs.freedesktop.org/show_bug.cgi?id=99164 . I have a similar bug here: https://bugs.freedesktop.org/show_bug.cgi?id=99887 > One thing that confuses me in the valgrind output below is: > CloseDownConnection (connection.c:919) seems to refer to the > FreeOsBuffers call (specifically, the free(oco) in there). However, > FreeOsBuffers is defined in io.c, so why does the valgrind output look > as if FreeOsBuffers was inlined into CloseDownConnection? Is there some > kind of LTO going on, or are there any patches affecting this applied on > top of upstream? No, we don't have patches on top of the xserver that would explain that... Probably a valgrind or a gcc issue. But I think I might be onto something, I strongly suspect this is a threading issue. I posted some more logs in bug 99887, and will send a possible patch shortly. Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: xserver 1.19.2 call for patches
- Original Message - > I'd like to do another 1.19 soonish, say middle of next week. That would be great if we could get to the bottom of the random crashes that affect 1.19.x first. Good news is we now have a core file from downstream bug https://bugzilla.redhat.com/show_bug.cgi?id=1424644 I'm struggling to find the problem so if anyone else has any idea, I'm all hear. So far what I did is to reduce the values of BUFSIZE and BUFWATERMARK in os/io.c in the hope to increase the chances to reach that portion of code, and run x11perf and gtkperf to load the server, running Xephyr in valgrind but could not see any memory corruption being reported by valgrind... Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] os: remove unused define MAX_TIMES_PER
Remove leftover from commit e10ba9e, MAX_TIMES_PER is not used anymore. Signed-off-by: Olivier Fourdan --- os/io.c | 1 - 1 file changed, 1 deletion(-) diff --git a/os/io.c b/os/io.c index 5ba1e86..be85226 100644 --- a/os/io.c +++ b/os/io.c @@ -116,7 +116,6 @@ static OsCommPtr AvailableInput = (OsCommPtr) NULL; lswapl(((xBigReq *)(req))->length) : \ ((xBigReq *)(req))->length) -#define MAX_TIMES_PER 10 #define BUFSIZE 16384 #define BUFWATERMARK 32768 -- 2.9.3 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] Revert "xwayland: bump wayland-protocols version to 1.7"
This reverts commit 371ff0c969a38a0013688391bbd7375bc7b6f933. --- I'm really sorry, I got confused, pointer contraints does not require 1.7 and 1.1 was enough... While it's not a major issue as 1.7 is a released version, we still can revert this commit as it's not mandatory. Sorry again! configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index e291b34..4dcf8b5 100644 --- a/configure.ac +++ b/configure.ac @@ -2475,7 +2475,7 @@ AM_CONDITIONAL(XFAKESERVER, [test "x$KDRIVE" = xyes && test "x$XFAKE" = xyes]) dnl Xwayland DDX -XWAYLANDMODULES="wayland-client >= 1.3.0 wayland-protocols >= 1.7 $LIBDRM epoxy" +XWAYLANDMODULES="wayland-client >= 1.3.0 wayland-protocols >= 1.1 $LIBDRM epoxy" if test "x$XF86VIDMODE" = xyes; then XWAYLANDMODULES="$XWAYLANDMODULES $VIDMODEPROTO" fi -- 2.9.3 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: xserver 1.19.2 call for patches
Hey Adam, > I'd like to do another 1.19 soonish, say middle of next week. If you've > got favorite patches you'd like to see included, please write their > sha1s on the back of a $50 bill and send them to me. > > Alternatively, reply to this email, or send a pull request. For Xwayland, I'd say: 058809c xwayland: Apply output rotation for screen size afeace2 xwayland: CRTC should support all rotations 1c78bec xwayland: Add hack for FWXGA resolution #99574 and in glamor (affecting Xwayland) 8646398 glamor: Two pass won't work on memory pixmaps Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: bump wayland-protocols version to 1.7
Xwayland support for pointer locking in confinement requires wayland-protocols version 1.7 or later. Update the required version in configure.ac to match the minimal required version of wayland-protocols. Signed-off-by: Olivier Fourdan --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 4dcf8b5..e291b34 100644 --- a/configure.ac +++ b/configure.ac @@ -2475,7 +2475,7 @@ AM_CONDITIONAL(XFAKESERVER, [test "x$KDRIVE" = xyes && test "x$XFAKE" = xyes]) dnl Xwayland DDX -XWAYLANDMODULES="wayland-client >= 1.3.0 wayland-protocols >= 1.1 $LIBDRM epoxy" +XWAYLANDMODULES="wayland-client >= 1.3.0 wayland-protocols >= 1.7 $LIBDRM epoxy" if test "x$XF86VIDMODE" = xyes; then XWAYLANDMODULES="$XWAYLANDMODULES $VIDMODEPROTO" fi -- 2.9.3 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver 2/2 v4] xwayland: Apply output rotation for screen size
Hey Adam, - Original Message - > On Wed, 2017-02-08 at 09:23 +0100, Olivier Fourdan wrote: > > Previously, we would swap the width/height of the Xwayland output based > > on the output rotation, so that the overall screen size would match the > > actual rotation of each output. > > This series makes sense, but I'm a little lost on which versions of > each patch are intended at this point. I assume it's this and v3 of > 1/2? > > (In general for short serieses I'd prefer a complete resend than trying > to sort out revisions within the thread.) Yeah... It's all confusing! Sorry! Those are the two patches: https://patchwork.freedesktop.org/patch/137446/ https://patchwork.freedesktop.org/patch/137635/ Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 2/2 v4] xwayland: Apply output rotation for screen size
Previously, we would swap the width/height of the Xwayland output based
on the output rotation, so that the overall screen size would match the
actual rotation of each output.
Problem is the RandR's ConstrainCursorHarder() handler will also apply
the output rotation, meaning that when the output is rotated, the
pointer will be constrained within the wrong dimension.
Moreover, XRandR assumes the original output width/height are unchanged
when the output is rotated, so by changing the Xwayland output width and
height based on rotation, Xwayland causes XRandr to report the wrong
output sizes (an output of size 1024x768 rotated left or right should
remain 1024x768, not 768x1024).
So to avoid this issue and keep things consistent between Wayland and
Xwayland outputs, leave the actual width/height unchanged but apply the
rotation when computing the screen size. This fixes both the output size
being wrong in "xrandr -q" and the pointer being constrained in the
wrong dimension with rotated with weston.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99663
Signed-off-by: Olivier Fourdan
---
v3: Split the patch in two as there two issues. The second issue being
the pointer events not being reported when a rotation is applied
this is because of the RR ConstrainCursorHarder() handler that we
should not need in Xwayland.
v4: Do not disable the ConstrainCursorHarder() handler set by
RRScreenInit(), simply leave the oupout size unchanged and apply
the rotation only when needed to compute the overall screen size...
hw/xwayland/xwayland-output.c | 28
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
index bdf270a..7d6c7b0 100644
--- a/hw/xwayland/xwayland-output.c
+++ b/hw/xwayland/xwayland-output.c
@@ -108,14 +108,8 @@ output_handle_mode(void *data, struct wl_output
*wl_output, uint32_t flags,
if (!(flags & WL_OUTPUT_MODE_CURRENT))
return;
-if (xwl_output->rotation & (RR_Rotate_0 | RR_Rotate_180)) {
-xwl_output->width = width;
-xwl_output->height = height;
-} else {
-xwl_output->width = height;
-xwl_output->height = width;
-}
-
+xwl_output->width = width;
+xwl_output->height = height;
xwl_output->refresh = refresh;
}
@@ -123,11 +117,21 @@ static inline void
output_get_new_size(struct xwl_output *xwl_output,
int *height, int *width)
{
-if (*width < xwl_output->x + xwl_output->width)
-*width = xwl_output->x + xwl_output->width;
+int output_width, output_height;
+
+if (xwl_output->rotation & (RR_Rotate_0 | RR_Rotate_180)) {
+output_width = xwl_output->width;
+output_height = xwl_output->height;
+} else {
+output_width = xwl_output->height;
+output_height = xwl_output->width;
+}
+
+if (*width < xwl_output->x + output_width)
+*width = xwl_output->x + output_width;
-if (*height < xwl_output->y + xwl_output->height)
-*height = xwl_output->y + xwl_output->height;
+if (*height < xwl_output->y + output_height)
+*height = xwl_output->y + output_height;
}
/* Approximate some kind of mmpd (m.m. per dot) of the screen given the outputs
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] xwayland: replace hardcoded function name with __func__ in error msg
Hey Peter, > > LGTM - Yo get rid of two \n along the way, but I think there were not > > needed in the first place so: > > oops. no, they're neeed so I added them back (and also to the instance where > it was missing). thanks Are they really needed? I looked at ErrorF() in the source tree and there are plenty of cases where there is no \n at the end, so I looked at LogVMessageVerb() where ErrorF() should end up, and it seemed to me it would ad it if missing: https://cgit.freedesktop.org/xorg/xserver/tree/os/log.c#n702 Anyway, it's no big deal :) Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver v3 2/2] xwayland: Disable RR ConstrainCursorHarder()
RRScreenInit() will install its own ConstrainCursorHarder() handler that
will apply pointer constraints depending on the RR config, including the
current rotation.
In the case of Xwayland, the output size is already swapped when
rotated, so the default ConstrainCursorHarder() will set the wrong
limits on the pointer.
Disable the ConstrainCursorHarder() handler set by RRScreenInit() to
avoid the issue.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99663
Signed-off-by: Olivier Fourdan
---
v3: Split the patch in two as there two issues. The second issue being
the pointer events not being reported when a rotation is applied
this is because of the RR ConstrainCursorHarder() handler that we
should not need in Xwayland.
hw/xwayland/xwayland-output.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
index bdf270a..613662b 100644
--- a/hw/xwayland/xwayland-output.c
+++ b/hw/xwayland/xwayland-output.c
@@ -339,14 +339,22 @@ xwl_randr_set_config(ScreenPtr pScreen,
Bool
xwl_screen_init_output(struct xwl_screen *xwl_screen)
{
+ScreenPtr pScreen = xwl_screen->screen;
+ConstrainCursorHarderProcPtr ConstrainCursorHarder;
rrScrPrivPtr rp;
-if (!RRScreenInit(xwl_screen->screen))
+/* Save current ConstrainCursorHarder handler, if any */
+ConstrainCursorHarder = pScreen->ConstrainCursorHarder;
+
+if (!RRScreenInit(pScreen))
return FALSE;
-RRScreenSetSizeRange(xwl_screen->screen, 320, 200, 8192, 8192);
+/* Restore previous ConstrainCursorHarder handler */
+pScreen->ConstrainCursorHarder = ConstrainCursorHarder;
+
+RRScreenSetSizeRange(pScreen, 320, 200, 8192, 8192);
-rp = rrGetScrPriv(xwl_screen->screen);
+rp = rrGetScrPriv(pScreen);
rp->rrGetInfo = xwl_randr_get_info;
rp->rrSetConfig = xwl_randr_set_config;
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver v3 1/2] xwayland: CRTC should support all rotations
If the Wayland compositor sets a rotation on the output, Xwayland
translates the transformation as an xrandr rotation for the given
output.
However, if the rotation is not supported by the CRTC, this is not
a valid setup and xrandr queries will fail.
Pretend we support all rotations and reflections so that the
configuration remains a valid xrandr setup.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99663
Signed-off-by: Olivier Fourdan
---
v2: We don't need to support all rotatons and reflections, just one
(RR_Rotate_0) and use it in the current config for rrGetInfo()
v3: Split the patch in two as there two issues. One being the config
set by Xwayland when there is a transformation not supported by the
CRTC, this is this patch.
hw/xwayland/xwayland-output.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
index f3ce763..bdf270a 100644
--- a/hw/xwayland/xwayland-output.c
+++ b/hw/xwayland/xwayland-output.c
@@ -31,6 +31,12 @@
#include
#define DEFAULT_DPI 96
+#define ALL_ROTATIONS (RR_Rotate_0 | \
+ RR_Rotate_90 | \
+ RR_Rotate_180 | \
+ RR_Rotate_270 | \
+ RR_Reflect_X | \
+ RR_Reflect_Y)
static Rotation
wl_transform_to_xrandr(enum wl_output_transform transform)
@@ -266,6 +272,7 @@ xwl_output_create(struct xwl_screen *xwl_screen, uint32_t
id)
ErrorF("Failed creating RandR CRTC\n");
goto err;
}
+RRCrtcSetRotations (xwl_output->randr_crtc, ALL_ROTATIONS);
xwl_output->randr_output = RROutputCreate(xwl_screen->screen, name,
strlen(name), xwl_output);
@@ -317,7 +324,7 @@ xwl_output_remove(struct xwl_output *xwl_output)
static Bool
xwl_randr_get_info(ScreenPtr pScreen, Rotation * rotations)
{
-*rotations = 0;
+*rotations = ALL_ROTATIONS;
return TRUE;
}
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] xwayland: replace hardcoded function name with __func__ in error msg
> Signed-off-by: Peter Hutterer
> ---
> hw/xwayland/xwayland-input.c | 4 ++--
> hw/xwayland/xwayland-output.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index 580df09..8d8bcbd 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -854,7 +854,7 @@ touch_handle_down(void *data, struct wl_touch *wl_touch,
>
> xwl_touch = calloc(1, sizeof *xwl_touch);
> if (xwl_touch == NULL) {
> -ErrorF("touch_handle_down ENOMEM");
> +ErrorF("%s ENOMEM", __func__);
> return;
> }
>
> @@ -1125,7 +1125,7 @@ create_input_device(struct xwl_screen *xwl_screen,
> uint32_t id, uint32_t version
>
> xwl_seat = calloc(1, sizeof *xwl_seat);
> if (xwl_seat == NULL) {
> -ErrorF("create_input ENOMEM\n");
> +ErrorF("%s ENOMEM", __func__);
> return;
> }
>
> diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
> index f3ce763..17715e2 100644
> --- a/hw/xwayland/xwayland-output.c
> +++ b/hw/xwayland/xwayland-output.c
> @@ -244,7 +244,7 @@ xwl_output_create(struct xwl_screen *xwl_screen, uint32_t
> id)
>
> xwl_output = calloc(1, sizeof *xwl_output);
> if (xwl_output == NULL) {
> -ErrorF("create_output ENOMEM\n");
> +ErrorF("%s ENOMEM", __func__);
> return NULL;
> }
>
> --
> 2.9.3
LGTM - Yo get rid of two \n along the way, but I think there were not needed in
the first place so:
Reviewed-by: Olivier Fourdan
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver v2] xwayland: Support RR_Rotate_0 in rrGetInfo()
If the Wayland compositor sets a rotation on the output, Xwayland
translates the transformation as an xrandr rotation for the given
output.
However, if the rotation is not supported, this is not a valid setup
and xrandr will fail.
Make sure we support the default rotation (RR_Rotate_0) and use it in
the current config for rrGetInfo() so that the configuration remains a
valid xrandr setup.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99663
Signed-off-by: Olivier Fourdan
---
v2: We don't need to support all rotatons and reflections, just one
(RR_Rotate_0) and use it in the current config for rrGetInfo()
hw/xwayland/xwayland-output.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
index f3ce763..51d5820 100644
--- a/hw/xwayland/xwayland-output.c
+++ b/hw/xwayland/xwayland-output.c
@@ -317,7 +317,15 @@ xwl_output_remove(struct xwl_output *xwl_output)
static Bool
xwl_randr_get_info(ScreenPtr pScreen, Rotation * rotations)
{
-*rotations = 0;
+RRScreenSizePtr pSize;
+
+*rotations = RR_Rotate_0;
+
+pSize = RRRegisterSize(pScreen, pScreen->width,
+pScreen->height,
+pScreen->mmWidth,
+pScreen->mmHeight);
+RRSetCurrentConfig(pScreen, RR_Rotate_0, 0, pSize);
return TRUE;
}
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: Pretend we support all transformations
If the Wayland compositor sets a rotation on the output, Xwayland
translates the transformation as an xrandr rotation.
However, if the rotations is not supported, this is not a valid setup
and xrandr will fail.
Pretend we support all rotations and transformations (as this may be set
via Wayland output protocol) so that the waylandsetup remains a valid
xrandr setup.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99663
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-output.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
index f3ce763..e923808 100644
--- a/hw/xwayland/xwayland-output.c
+++ b/hw/xwayland/xwayland-output.c
@@ -317,7 +317,20 @@ xwl_output_remove(struct xwl_output *xwl_output)
static Bool
xwl_randr_get_info(ScreenPtr pScreen, Rotation * rotations)
{
-*rotations = 0;
+RRScreenSizePtr pSize;
+
+*rotations = RR_Rotate_0 | \
+ RR_Rotate_90 | \
+ RR_Rotate_180 | \
+ RR_Rotate_270 | \
+ RR_Reflect_X | \
+ RR_Reflect_Y;
+
+pSize = RRRegisterSize(pScreen, pScreen->width,
+pScreen->height,
+pScreen->mmWidth,
+pScreen->mmHeight);
+RRSetCurrentConfig(pScreen, RR_Rotate_0, 0, pSize);
return TRUE;
}
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xwayland] xwayland: Add hack for FWXGA resolution #99574
> For some applications (like fullscreen games) it matters for XRandr
> resolution to be correctly set and equal to root window resolution.
>
> In XServer there is already hack for this, adapted it for XWayland.
>
> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=99574
>
> Signed-off-by: Svitozar Cherepii
> ---
> hw/xwayland/xwayland-cvt.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/hw/xwayland/xwayland-cvt.c b/hw/xwayland/xwayland-cvt.c
> index 9655e104e..8564fdbae 100644
> --- a/hw/xwayland/xwayland-cvt.c
> +++ b/hw/xwayland/xwayland-cvt.c
> @@ -296,6 +296,13 @@ xwayland_cvt(int HDisplay, int VDisplay, float VRefresh,
> Bool Reduced,
> if (Interlaced)
> modeinfo.modeFlags |= RR_Interlace;
>
> +/* FWXGA hack adapted from hw/xfree86/modes/xf86EdidModes.c, because you
> can't say 1366 */
> +if (HDisplay == 1366 && VDisplay == 768) {
> + modeinfo.width = 1366;
> + modeinfo.hSyncStart--;
> + modeinfo.hSyncEnd--;
> +}
> +
> snprintf(name, sizeof name, "%dx%d",
> modeinfo.width, modeinfo.height);
> modeinfo.nameLength = strlen(name);
> --
> 2.11.0
LGTM.
Acked-by: Olivier Fourdan
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] xwayland: Support horizontal resolutions not divisible by 8 #99574
Hi
> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=99574
>
> Signed-off-by: Svitozar Cherepii
> ---
> hw/xwayland/xwayland-cvt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/xwayland/xwayland-cvt.c b/hw/xwayland/xwayland-cvt.c
> index 9655e104e..d6ff305c7 100644
> --- a/hw/xwayland/xwayland-cvt.c
> +++ b/hw/xwayland/xwayland-cvt.c
> @@ -101,7 +101,7 @@ xwayland_cvt(int HDisplay, int VDisplay, float VRefresh,
> Bool Reduced,
> VFieldRate = VRefresh;
>
> /* 2. Horizontal pixels */
> -HDisplayRnd = HDisplay - (HDisplay % CVT_H_GRANULARITY);
> +HDisplayRnd = HDisplay;
>
> /* 3. Determine left and right borders */
> if (Margins) {
> --
> 2.11.0
I don't think the patch is correct, the code is to calculate a CVT mode and
1366x768 is not compliant.
So why does it actually matter? The root window has the correct size (1366x768)
whereas the CVT is used to compute the mode, which is not used actually in
Xwayland, apart for returning a mode in both xrandr and the "fake" vidmode
extension support in Xwayland.
So I wonder, is it just about the mode name in xrandr? If so, then it would be
better to change the snprintf() at the end of xwayland_cvt() to to get
"1366x768" as the mode name without changing the actual CVT computation.
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: glamor woes with nouveau
Hey! So I was wrong (again), this is not glamor_composite_choose_shader() failed which is the problem, the rendering issue comes from glamor_poly_fill_rect_gl() (the plain ackground is garbled, not the actual content, if that makes any sense). If I force glamor_poly_fill_rect_bail() from glamor_poly_fill_rect() rendering is fine. It is also worth noting that the glsl version on this particular hardware is 120, so we are using glamor_facet_polyfillrect_120... I also noticed that not showing the GtkEntry box avoids the issue, but I can't make sense of that. Still digging, meanwhile if someone else has an idea :) Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH v2 xserver] glamor: Two pass won't work on memory pixmaps
When selecting "CA_TWO_PASS" in glamor_composite_clipped_region() when
the hardware does not support "GL_ARB_blend_func_extended", we call
glamor_composite_choose_shader() twice in a row, which in turn calls
glamor_pixmap_ensure_fbo().
On memory pixmaps, the first call will set the FBO and the second one
will fail an assertion in glamor_upload_picture_to_texture() because
the FBO is already set.
Bail out earlier when the mask pixmap is in memory and the hardware
capabilities would require to use two pass, so that the assertion is not
failed and the rendering is correct.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99346
Signed-off-by: Olivier Fourdan
---
v2: Bail out only on the specific case which would lead to the assertion
failure otherwise
glamor/glamor_render.c | 4
1 file changed, 4 insertions(+)
diff --git a/glamor/glamor_render.c b/glamor/glamor_render.c
index e04dd21..52f073d 100644
--- a/glamor/glamor_render.c
+++ b/glamor/glamor_render.c
@@ -1494,6 +1494,10 @@ glamor_composite_clipped_region(CARD8 op,
ca_state = CA_DUAL_BLEND;
} else {
if (op == PictOpOver) {
+if (glamor_pixmap_is_memory(mask_pixmap)) {
+glamor_fallback("two pass not supported on memory
pximaps\n");
+goto out;
+}
ca_state = CA_TWO_PASS;
op = PictOpOutReverse;
}
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] glamor: keep gl_fbo and fbo consistent
> There is a fundamental logical problem with this patch though, because > glamor_upload_picture_to_texture() does: > > assert(glamor_pixmap_is_memory(pixmap)); > > which is basically priv->type == GLAMOR_MEMORY; > > So glamor_upload_picture_to_texture() clearly expects a pixmap of type > GLAMOR_MEMORY which my patch avoids, so the patch is clearly not right... I > mean, it avoids the assert() failure but it disables the entire logic. Oh, I reckon this it pretty obvious though... Sorry I did not spot this before, took me a while to figure that, but this is because of CA_TWO_PASS (that's even obious from the backtrace frame #5 in bug 99346 [0]) In glamor_composite_with_shader() [1] we call glamor_composite_choose_shader() twice in a row when CA_TWO_PASS is set [2], the first call works, but the second reaches the assertion failure set by commit ee7ca67 [3]. This is because glamor_composite_choose_shader() calls glamor_upload_picture_to_texture() which in turn calls glamor_pixmap_ensure_fbo() which sets the fbo on the first call, which cause the assert to be reached on the second. So I suspect commit ee7ca67 [3] and commit 1bed5ef [4] can't work with CA_TWO_PASS Cheers, Olivier [0] https://bugs.freedesktop.org/show_bug.cgi?id=99346#c0 [1] https://cgit.freedesktop.org/xorg/xserver/tree/glamor/glamor_render.c#n1112 [2] https://cgit.freedesktop.org/xorg/xserver/tree/glamor/glamor_render.c#n1142 [3] https://cgit.freedesktop.org/xorg/xserver/commit/?id=ee7ca67 [4] https://cgit.freedesktop.org/xorg/xserver/commit/?id=1bed5ef ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] glamor: keep gl_fbo and fbo consistent
Hi
> If the pixmap type is neither GLAMOR_TEXTURE_ONLY nor GLAMOR_TEXTURE_DRM
> we might have the fbo field set but the gl_fbo still set to the default
> GLAMOR_FBO_UNATTACHED, which later may fail an assert in
> glamor_upload_picture_to_texture().
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99346
> Signed-off-by: Olivier Fourdan
> ---
> glamor/glamor_render.c | 8
> glamor/glamor_utils.h | 4
> 2 files changed, 12 insertions(+)
>
> diff --git a/glamor/glamor_render.c b/glamor/glamor_render.c
> index e04dd21..a9ab971 100644
> --- a/glamor/glamor_render.c
> +++ b/glamor/glamor_render.c
> @@ -916,6 +916,10 @@ glamor_composite_choose_shader(CARD8 op,
> }
> if (source_pixmap_priv->gl_fbo == GLAMOR_FBO_UNATTACHED) {
> #ifdef GLAMOR_PIXMAP_DYNAMIC_UPLOAD
> +if (!GLAMOR_PIXMAP_PRIV_HAS_TEXTURE(source_pixmap_priv)) {
> +glamor_fallback("no texture in source\n");
> +goto fail;
> +}
> source_needs_upload = TRUE;
> #else
> glamor_fallback("no texture in source\n");
> @@ -932,6 +936,10 @@ glamor_composite_choose_shader(CARD8 op,
> }
> if (mask_pixmap_priv->gl_fbo == GLAMOR_FBO_UNATTACHED) {
> #ifdef GLAMOR_PIXMAP_DYNAMIC_UPLOAD
> +if (!GLAMOR_PIXMAP_PRIV_HAS_TEXTURE(mask_pixmap_priv)) {
> +glamor_fallback("no texture in mask\n");
> +goto fail;
> +}
> mask_needs_upload = TRUE;
> #else
> glamor_fallback("no texture in mask\n");
> diff --git a/glamor/glamor_utils.h b/glamor/glamor_utils.h
> index 6b88527..636c095 100644
> --- a/glamor/glamor_utils.h
> +++ b/glamor/glamor_utils.h
> @@ -585,6 +585,10 @@
>
> #define GLAMOR_PIXMAP_PRIV_HAS_FBO(pixmap_priv)(pixmap_priv->gl_fbo ==
> GLAMOR_FBO_NORMAL)
>
> +#define GLAMOR_PIXMAP_PRIV_HAS_TEXTURE(pixmap_priv) (
> \
> + pixmap_priv->type == GLAMOR_TEXTURE_DRM \
> + || pixmap_priv->type == GLAMOR_TEXTURE_ONLY)
> +
> /**
> * Borrow from uxa.
> */
> --
> 2.9.3
There is a fundamental logical problem with this patch though, because
glamor_upload_picture_to_texture() does:
assert(glamor_pixmap_is_memory(pixmap));
which is basically priv->type == GLAMOR_MEMORY;
So glamor_upload_picture_to_texture() clearly expects a pixmap of type
GLAMOR_MEMORY which my patch avoids, so the patch is clearly not right... I
mean, it avoids the assert() failure but it disables the entire logic.
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] glamor: keep gl_fbo and fbo consistent
If the pixmap type is neither GLAMOR_TEXTURE_ONLY nor GLAMOR_TEXTURE_DRM
we might have the fbo field set but the gl_fbo still set to the default
GLAMOR_FBO_UNATTACHED, which later may fail an assert in
glamor_upload_picture_to_texture().
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99346
Signed-off-by: Olivier Fourdan
---
glamor/glamor_render.c | 8
glamor/glamor_utils.h | 4
2 files changed, 12 insertions(+)
diff --git a/glamor/glamor_render.c b/glamor/glamor_render.c
index e04dd21..a9ab971 100644
--- a/glamor/glamor_render.c
+++ b/glamor/glamor_render.c
@@ -916,6 +916,10 @@ glamor_composite_choose_shader(CARD8 op,
}
if (source_pixmap_priv->gl_fbo == GLAMOR_FBO_UNATTACHED) {
#ifdef GLAMOR_PIXMAP_DYNAMIC_UPLOAD
+if (!GLAMOR_PIXMAP_PRIV_HAS_TEXTURE(source_pixmap_priv)) {
+glamor_fallback("no texture in source\n");
+goto fail;
+}
source_needs_upload = TRUE;
#else
glamor_fallback("no texture in source\n");
@@ -932,6 +936,10 @@ glamor_composite_choose_shader(CARD8 op,
}
if (mask_pixmap_priv->gl_fbo == GLAMOR_FBO_UNATTACHED) {
#ifdef GLAMOR_PIXMAP_DYNAMIC_UPLOAD
+if (!GLAMOR_PIXMAP_PRIV_HAS_TEXTURE(mask_pixmap_priv)) {
+glamor_fallback("no texture in mask\n");
+goto fail;
+}
mask_needs_upload = TRUE;
#else
glamor_fallback("no texture in mask\n");
diff --git a/glamor/glamor_utils.h b/glamor/glamor_utils.h
index 6b88527..636c095 100644
--- a/glamor/glamor_utils.h
+++ b/glamor/glamor_utils.h
@@ -585,6 +585,10 @@
#define GLAMOR_PIXMAP_PRIV_HAS_FBO(pixmap_priv)(pixmap_priv->gl_fbo ==
GLAMOR_FBO_NORMAL)
+#define GLAMOR_PIXMAP_PRIV_HAS_TEXTURE(pixmap_priv) ( \
+ pixmap_priv->type == GLAMOR_TEXTURE_DRM \
+ || pixmap_priv->type == GLAMOR_TEXTURE_ONLY)
+
/**
* Borrow from uxa.
*/
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver v2 5/5] xwayland: use _XWAYLAND_ALLOW_COMMITS property
Hi Pekka, - Original Message - > here is an update on the Weston side: > https://lists.freedesktop.org/archives/wayland-devel/2017-January/032712.html > > The related Weston patches series has shrunk from 24 to 3 patches as > lots of them have been merged. The stuff about _XWAYLAND_ALLOW_COMMITS > is still pending. > > Given the development on Weston side, would you demand implementing > NET_WM_SYNC_REQUEST support in Weston before deciding whether to merge > _XWAYLAND_ALLOW_COMMITS support in Xwayland, or is the rationale in the > remaining Weston patches enough to justify it already? I meant to reply your previous email but didn't quite finish it, sorry... What I meant to say there is NET_WM_SYNC and _XWAYLAND_ALLOW_COMMITS are two different things and I don't think you can reach the same goals using NET_WM_SYNC_REQUEST. The goal of NET_WM_SYNC is to make sure the window manager is not flooding the client with configure requests while the user resizes the window. Without NET_WM_SYNC, you can easily see the client window/repaint lagging behind when resizing even with an X11 compositor - That's quite different from what _XWAYLAND_ALLOW_COMMITS is meant for. Not all apps use NET_WM_SYNC, actually few do (mostly gtk and qt based apps) when considering the large number of X11 apps available, so you cannot rely on NET_WM_SYNC being available, whereas having _XWAYLAND_ALLOW_COMMITS in Xwayland make it available for all X11 clients running on Wayland with a compositor taking advantage of it. So, *IMHO* _XWAYLAND_ALLOW_COMMITS should not depend on weston implementing NET_WM_SYNC_REQUEST. Sorry if I caused some confusion. > The window jumping I talked about no longer happens, due to reordering > the patch series, but I believe that is just the race being won rather > than lost most of the time and that the race which > _XWAYLAND_ALLOW_COMMITS will remove still exists. Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: glamor woes with nouveau
Hi, On 18 January 2017 at 10:27, Olivier Fourdan wrote: >> >> This is where it gets even more confusing, I cannot reproduce the >> issue using "Xephyr -glamor"... Not even with Xephyr running in >> Xwayland. > > And by that I mean, rendering is fine and I don't get the errors > "glamor_composite_choose_shader: Unsupported source picture format" > when using "Xephyr -glamor" > >> I can reproduce on Xwayland though (I mean, running another Xwayland >> server in a Wayland session), I can capture an apitrace of that, but >> the apitrace looks incomplete and cannot be replayed (no context). (sorry for the spamming!!) And that got me thinking... Could it be that Xwayland exposes some different visuals that gtk+ would pick and that would fail with glamor? xdpyinfo shows differences in the visuals available between Xephyr and Xwayland. Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: glamor woes with nouveau
On 18 January 2017 at 10:10, Olivier Fourdan wrote: > Hi Eric > > On 17 January 2017 at 21:25, Eric Anholt wrote: >> >> It sounds like nouveau is failing at the getteximage/texsubimage cycle >> in the fallback path. X sometimes finds bugs in Mesa than our testcases >> miss, because it frequently reads/writes subsets of a texture. In >> comparison, testcases tend to use solid colors, window system >> renderbuffers instead of textures, and the whole renderbuffer instead of >> a subset. >> >> I'd recommend getting an apitrace of the rendering under Xephyr -glamor, >> if possible. Then you can use apitrace to dump images of each draw >> call on both intel and nouveau, and identify which draw call/texture >> upload was the one that failed. > > This is where it gets even more confusing, I cannot reproduce the > issue using "Xephyr -glamor"... Not even with Xephyr running in > Xwayland. And by that I mean, rendering is fine and I don't get the errors "glamor_composite_choose_shader: Unsupported source picture format" when using "Xephyr -glamor" > I can reproduce on Xwayland though (I mean, running another Xwayland > server in a Wayland session), I can capture an apitrace of that, but > the apitrace looks incomplete and cannot be replayed (no context). > > Cheers, > Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: glamor woes with nouveau
Hi Eric On 17 January 2017 at 21:25, Eric Anholt wrote: > > It sounds like nouveau is failing at the getteximage/texsubimage cycle > in the fallback path. X sometimes finds bugs in Mesa than our testcases > miss, because it frequently reads/writes subsets of a texture. In > comparison, testcases tend to use solid colors, window system > renderbuffers instead of textures, and the whole renderbuffer instead of > a subset. > > I'd recommend getting an apitrace of the rendering under Xephyr -glamor, > if possible. Then you can use apitrace to dump images of each draw > call on both intel and nouveau, and identify which draw call/texture > upload was the one that failed. This is where it gets even more confusing, I cannot reproduce the issue using "Xephyr -glamor"... Not even with Xephyr running in Xwayland. I can reproduce on Xwayland though (I mean, running another Xwayland server in a Wayland session), I can capture an apitrace of that, but the apitrace looks incomplete and cannot be replayed (no context). Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
glamor woes with nouveau
Hi all, I am trying to investigate an issue with Xwayland that affects pixmap rendering only some hardware with nouveau (basically, older hardware). Initally, I started looking into this because of reported black icons in gtk2 (X11/Xwayland) apps in GNOME and RH bugzilla, and discovered that one possible common factor is the use on nouveau: https://bugzilla.redhat.com/show_bug.cgi?id=1411447 https://bugzilla.gnome.org/show_bug.cgi?id=776255 While trying to reproduce, I got a much worse problem where sometimes, the entire window would appear completely garbled. I initially thought of an issue in meda DRI driver, so I filed bug 99400 for this, but I am now wondering if this could be an issue with glamor instead: https://bugs.freedesktop.org/show_bug.cgi?id=99400 Disabling glamor in Xwayland makes the problem go away, whereas Mesa with debug enabled doesn't seem to complain much Running Xwayland with GLAMOR_DEBUG=3 shows a few messages, most noticeably: glamor_composite_choose_shader: Unsupported source picture format. glamor_composite_with_shader: glamor_composite_choose_shader failed glamor_composite: from picts 0x25c8050:0x267ff60 48x48 / 0x25c3940:0x267ff60 48 x 48 (f,f) to pict 0x2586890:0x2606e30 523x300 (f) 523x300 would match the size of the window where the entire background is (apparently) random. When the errors occur, we would end up in the fallback code of glamor_composite() in glamor/glamor_render.c But then I tried the same on intel hardware an get the exact same glamor messages, and yet the rendering is fine on intel, so I am a bit confused, could that be an issue with the fallback code in glamor_composite() or is it that the source pixmap being copied is actually garbled? Any hint on how I could investigate that further? Thanks Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver v2 5/5] xwayland: use _XWAYLAND_ALLOW_COMMITS property
Hi, - Original Message - > On Fri, 2016-12-09 at 14:24 +0200, Pekka Paalanen wrote: > > From: Pekka Paalanen > > > > The X11 window manager (XWM) of a Wayland compositor can use the > > _XWAYLAND_ALLOW_COMMITS property to control when Xwayland sends > > wl_surface.commit requests. If the property is not set, the behaviour > > remains what it was. > > I'm still thinking over whether I like this or whether I'd rather have > this keyed off the netwm sync props. (Did we think that was a thing > that could work? Forgive me, I'm freshly back from holidays.) [...] Maybe it's two different goals. If I understand correctly, this patch was initially intended for the initial map of the window. FWIW I spent quite a few days before the holidays experimenting with this, trying to see if that could be used to help with the black border issue reported in mutter with server-side decorations [1] on applications implementing the NET_WM_SYNC extended protocol [2]. Unfortunately, I didn't have much success in doing so. Freezing the commit in either meta_window_actor_freeze() or meta_window_actor_pre_paint() and re-allowing the commit in meta_window_actor_thaw() or meta_window_actor_post_paint() didn't help at all with the visible black border during resize, and made the resize more sluggish because while the commit is frozen, the surface is not resized but can still be moved so resizing from the left or top looks awful, from a user stand point. I think I could trace the black border issue in mutter back to meta_window_actor_update_opaque_region(), but I was not able to use the _XWAYLAND_ALLOW_COMMITS property to fix or even improve the problem, so I am not even sure _XWAYLAND_ALLOW_COMMITS is the solution for the problem I am trying to fix - Granted, I am not an expert in this area of mutter, but I start to wonder if _XWAYLAND_ALLOW_COMMITS is the solution to the specific issue I am trying to solve. So, we might eventually need both, something like _XWAYLAND_ALLOW_COMMITS for the initial map and also have Xwayland monitoring NET_WM_SYNC properties. Cheers, Olivier [1] https://bugzilla.gnome.org/show_bug.cgi?id=767212 [2] http://fishsoup.net/misc/wm-spec-synchronization.html ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: Null pointer deref in FlushAllOutput with 1.19-rc1 ?
Hi Keith, > Olivier Fourdan writes: > > >> FlushAllOutput() in /usr/src/debug/xorg-server-20160929/os/io.c:612 > >> Dispatch() in /usr/src/debug/xorg-server-20160929/dix/dispatch.c:3491 > >> dix_main() in /usr/src/debug/xorg-server-20160929/dix/main.c:296 > > I have a theory about how this is happening -- events may be delivered > during client shutdown but after CloseDownClient removed the client from > the output_pending queue. Moving this call until after clientGone is > set, and then making output_pending_mark check that flag before queueing > it will avoid that problem. > > A patch has been sent to the list, any idea how we can test this? Unfortunately, I suspect the fix is not complete, as we still see similar bugs being reported against 1.19.0: https://bugzilla.redhat.com/show_bug.cgi?id=1402515 https://bugzilla.redhat.com/show_bug.cgi?id=1402158 https://bugzilla.redhat.com/show_bug.cgi?id=1399773 I'm sorry to be the one bringing the bad news... Maybe it's a different issue but it looks quite similar, I reckon. Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [RFC xserver 0/3] Allow XWM to control Xwayland commits
Hey On 5 December 2016 at 22:43, Adam Jackson wrote: > (apologies for being so slow to get to this thread, this is great stuff) > > On Mon, 2016-11-28 at 15:47 +0200, Pekka Paalanen wrote: > [...] > > > Mind, I am mostly thinking this in Weston XWM terms, which draws the > > window decorations through X11 like a normal window manager. > > That's fine. Honestly I'd prefer a world where the wayland server was > not also the window manager, you should be able to run twm if you want > (and have it work about as well as, say, twm on win32). That'd need an > x11 compat protocol to really handle well, but again, okay. I think > that's _better_ than the current model, where mutter just magically > knows to get X geometry info through this side channel and therefore > has to worry about races, instead of Xwayland always presenting a > logically consistent view of its world to the wayland server. > Putting my /other desktop/ swimming suit here, I would love to see such a model, it would greatly help with a wider Wayland adoption imho. Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: startx(1) low hanging fruit buglet for > 1 year in bugzilla
Hi, > Esteemed X11 developers, > > may I direct your attention to a shell scripting buglet in startx(1), as > reported in https://bugs.freedesktop.org/show_bug.cgi?id=91811 which > sits there for more than a year now? Fix included. This is very low > hanging fruit for improving the code. If the fix can be improved, please > let me know. Thanks for your time and effort! Problem is patches in bugzilla are unlikely to be reviewed, even less when they are inline in the bug description. Best is to prepare a patch (on top of git master, either with git format-patch or directly with git send-email, not forgetting to add the "signed-off-by:" and referencing the "bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91811";) and post it to xorg-devel mailing list so it can be reviewed and picked by the maintainers. See this page for more details on how to submit patches: https://www.x.org/wiki/Development/Documentation/SubmittingPatches/ Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [RFC xserver 2/3] xwayland: fix 'buffer' may be used uninitialized warning
> From: Pekka Paalanen > > Fix the following warning due to --disable-glamor: > > CC Xwayland-xwayland.o > In file included from /home/pq/local/include/wayland-client.h:40:0, > from xwayland.h:35, > from xwayland.c:26: > xwayland.c: In function ‘block_handler’: > /home/pq/local/include/wayland-client-protocol.h:3446:2: warning: ‘buffer’ > may be used uninitialized in this function [-Wmaybe-uninitialized] > wl_proxy_marshal((struct wl_proxy *) wl_surface, > ^ > xwayland.c:466:23: note: ‘buffer’ was declared here > struct wl_buffer *buffer; >^ > > Signed-off-by: Pekka Paalanen > --- > hw/xwayland/xwayland.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c > index c2ac4b3..9d79554 100644 > --- a/hw/xwayland/xwayland.c > +++ b/hw/xwayland/xwayland.c > @@ -474,8 +474,8 @@ xwl_window_post_damage(struct xwl_window *xwl_window) > #if GLAMOR_HAS_GBM > if (xwl_screen->glamor) > buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap); > +else > #endif > -if (!xwl_screen->glamor) > buffer = xwl_shm_pixmap_get_wl_buffer(pixmap); > > wl_surface_attach(xwl_window->surface, buffer, 0, 0); > -- > 2.7.3 > Reviewed-by: Olivier Fourdan Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [RFC xserver 1/3] xwayland: refactor into xwl_window_post_damage()
> From: Pekka Paalanen
>
> Refactor xwl_screen_post_damage() and split the window specific code
> into a new function xwl_window_post_damage().
>
> This is a pure refactoring, there are no behavioral changes. An assert
> is added to xwl_window_post_damage() to ensure frame callbacks are not
> leaked if a future patch changes the call.
>
> Signed-off-by: Pekka Paalanen
> ---
> hw/xwayland/xwayland.c | 56
> +-
> 1 file changed, 33 insertions(+), 23 deletions(-)
>
> diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
> index 9e1ecf8..c2ac4b3 100644
> --- a/hw/xwayland/xwayland.c
> +++ b/hw/xwayland/xwayland.c
> @@ -458,44 +458,54 @@ static const struct wl_callback_listener frame_listener
> = {
> };
>
> static void
> -xwl_screen_post_damage(struct xwl_screen *xwl_screen)
> +xwl_window_post_damage(struct xwl_window *xwl_window)
> {
> -struct xwl_window *xwl_window, *next_xwl_window;
> +struct xwl_screen *xwl_screen = xwl_window->xwl_screen;
> RegionPtr region;
> BoxPtr box;
> struct wl_buffer *buffer;
> PixmapPtr pixmap;
>
> -xorg_list_for_each_entry_safe(xwl_window, next_xwl_window,
> - &xwl_screen->damage_window_list,
> link_damage) {
> -/* If we're waiting on a frame callback from the server,
> - * don't attach a new buffer. */
> -if (xwl_window->frame_callback)
> -continue;
> +assert(!xwl_window->frame_callback);
>
> -region = DamageRegion(xwl_window->damage);
> -pixmap = (*xwl_screen->screen->GetWindowPixmap)
> (xwl_window->window);
> +region = DamageRegion(xwl_window->damage);
> +pixmap = (*xwl_screen->screen->GetWindowPixmap) (xwl_window->window);
>
> #if GLAMOR_HAS_GBM
> -if (xwl_screen->glamor)
> -buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap);
> +if (xwl_screen->glamor)
> +buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap);
> #endif
> -if (!xwl_screen->glamor)
> -buffer = xwl_shm_pixmap_get_wl_buffer(pixmap);
> +if (!xwl_screen->glamor)
> +buffer = xwl_shm_pixmap_get_wl_buffer(pixmap);
>
> -wl_surface_attach(xwl_window->surface, buffer, 0, 0);
> +wl_surface_attach(xwl_window->surface, buffer, 0, 0);
>
> -box = RegionExtents(region);
> -wl_surface_damage(xwl_window->surface, box->x1, box->y1,
> - box->x2 - box->x1, box->y2 - box->y1);
> +box = RegionExtents(region);
> +wl_surface_damage(xwl_window->surface, box->x1, box->y1,
> +box->x2 - box->x1, box->y2 - box->y1);
>
> -xwl_window->frame_callback = wl_surface_frame(xwl_window->surface);
> -wl_callback_add_listener(xwl_window->frame_callback,
> &frame_listener, xwl_window);
> +xwl_window->frame_callback = wl_surface_frame(xwl_window->surface);
> +wl_callback_add_listener(xwl_window->frame_callback, &frame_listener,
> xwl_window);
>
> -wl_surface_commit(xwl_window->surface);
> -DamageEmpty(xwl_window->damage);
> +wl_surface_commit(xwl_window->surface);
> +DamageEmpty(xwl_window->damage);
>
> -xorg_list_del(&xwl_window->link_damage);
> +xorg_list_del(&xwl_window->link_damage);
> +}
> +
> +static void
> +xwl_screen_post_damage(struct xwl_screen *xwl_screen)
> +{
> +struct xwl_window *xwl_window, *next_xwl_window;
> +
> + xorg_list_for_each_entry_safe(xwl_window, next_xwl_window,
> + &xwl_screen->damage_window_list,
> link_damage) {
> +/* If we're waiting on a frame callback from the server,
> + * don't attach a new buffer. */
> +if (xwl_window->frame_callback)
> +continue;
> +
> +xwl_window_post_damage(xwl_window);
> }
> }
>
> --
> 2.7.3
Reviewed-by: Olivier Fourdan
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [RFC xserver 3/3] Xwayland: use _XWAYLAND_ALLOW_COMMITS property
Hi Pekka, > [...] > Patches 1 and 2 OTOH would be ready for merging on my behalf. Yes, I think the two first patches are fine. > Olivier asked about _NET_WM_SYNC_REQUEST - do you want me to fully > implement the basic sync protocol too before accepting this series? I was hoping/wondering if using _XWAYLAND_ALLOW_COMMITS could help the X11 window manager/X11 compositor to better sync the updates from the client with the refresh of the server side decorations (which includes drop shadows) to reduce the artefacts with server side decorations and drop shadows when resizing apps implementing the _NET_WM_SYNC_REQUEST protocol under Xwayland But if anything, imho, it's up to the X11 window manager/X11 compositor to use both mechanism when dealing with Xwayland, since the window manager/X11 compositor "knows" about both properties and when it's best to paint. So FWIW, I don't think it's necessary to add _NET_WM_SYNC_REQUEST to Xwayland, when using _XWAYLAND_ALLOW_COMMITS. I'll play with mutter and _XWAYLAND_ALLOW_COMMITS to see how that goes :) > [...] > I expect the property to be written only from the XWM. Should I verify > that somehow? Can I even identify the XWM here? I guess any client (or even user playing with xprop [1]) could add/change the property to any toplevel X11 window and Xwayland would stop committing the surfaces, but what would be the point of doing that? Cheers, Olivier [1] "xprop -id ... -format _XWAYLAND_ALLOW_COMMITS 32c -set _XWAYLAND_ALLOW_COMMITS 0" brings the update to a halt :) ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[Pull request]: couple of fixes for xwayland
The following changes since commit d6a6e1d6abb110ff00ad31b94cd29d92ca7c71a5: Xi: when creating a new master device, update barries for all clients (2016-11-30 12:05:34 +1000) are available in the git repository at: git://people.freedesktop.org/~ofourdan/xserver xwayland for you to fetch changes up to e1d30075c923f96a375895d74ea12a3c92a640c6: configure: Enable glamor when building just Xwayland (2016-11-30 09:47:43 +0100) Adam Jackson (1): configure: Enable glamor when building just Xwayland Olivier Fourdan (2): glamor: restore vfunc handlers on init failure xwayland: Fix use after free of cursors configure.ac | 5 + glamor/glamor.c | 11 --- hw/xwayland/xwayland-input.c | 17 + 3 files changed, 22 insertions(+), 11 deletions(-) ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: Fix use after free of cursors
Sometimes, Xwayland will try to use a cursor that has just been freed,
leading to a crash when trying to access that cursor data either in
miPointerUpdateSprite() or AnimCurTimerNotify().
CheckMotion() updates the pointer's cursor based on which xwindow
XYToWindow() returns, and Xwayland implements its own xwl_xy_to_window()
to fake a crossing to the root window when the pointer has left the
Wayland surface but is still within the xwindow.
But after an xwindow is unrealized, the last xwindow used to match the
xwindows is cleared so two consecutive calls to xwl_xy_to_window() may
not return the same xwindow.
To avoid this issue, update the last_xwindow based on enter and leave
notifications instead of xwl_xy_to_window(), and check if the xwindow
found by the regular miXYToWindow() is a child of the known last
xwindow, so that multiple consecutive calls to xwl_xy_to_window()
return the same xwindow, being either the one found by miXYToWindow()
or the root window.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1385258
Signed-off-by: Olivier Fourdan
Tested-by: Vít Ondruch
Tested-by: Satish Balay
Reviewed-by: Jonas Ådahl
---
hw/xwayland/xwayland-input.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index 3bc7110..580df09 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -312,6 +312,9 @@ pointer_handle_enter(void *data, struct wl_pointer *pointer,
dx = xwl_seat->focus_window->window->drawable.x;
dy = xwl_seat->focus_window->window->drawable.y;
+/* We just entered a new xwindow, forget about the old last xwindow */
+xwl_seat->last_xwindow = NullWindow;
+
master = GetMaster(dev, POINTER_OR_FLOAT);
(*pScreen->SetCursorPosition) (dev, pScreen, dx + sx, dy + sy, TRUE);
@@ -360,8 +363,14 @@ pointer_handle_leave(void *data, struct wl_pointer
*pointer,
xwl_seat->xwl_screen->serial = serial;
-xwl_seat->focus_window = NULL;
-CheckMotion(NULL, GetMaster(dev, POINTER_OR_FLOAT));
+/* The pointer has left a known xwindow, save it for a possible match
+ * in sprite_check_lost_focus()
+ */
+if (xwl_seat->focus_window) {
+xwl_seat->last_xwindow = xwl_seat->focus_window->window;
+xwl_seat->focus_window = NULL;
+CheckMotion(NULL, GetMaster(dev, POINTER_OR_FLOAT));
+}
}
static void
@@ -1252,10 +1261,10 @@ sprite_check_lost_focus(SpritePtr sprite, WindowPtr
window)
*/
if (master->lastSlave == xwl_seat->pointer &&
xwl_seat->focus_window == NULL &&
-xwl_seat->last_xwindow == window)
+xwl_seat->last_xwindow != NullWindow &&
+IsParent(xwl_seat->last_xwindow, window))
return TRUE;
-xwl_seat->last_xwindow = window;
return FALSE;
}
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver v3] xwayland: Fix use after free of cursors
Hi, > > Sometimes, Xwayland will try to use a cursor that has just been freed, > > leading to a crash when trying to access that cursor data either in > > miPointerUpdateSprite() or AnimCurTimerNotify(). > > > > CheckMotion() updates the pointer's cursor based on which xwindow > > XYToWindow() returns, and Xwayland implements its own xwl_xy_to_window() > > to fake a crossing to the root window when the pointer has left the > > Wayland surface but is still within the xwindow. > > > > But after an xwindow is unrealized, the last xwindow used to match the > > xwindows is cleared so two consecutive calls to xwl_xy_to_window() may > > not return the same xwindow. > > > > To avoid this issue, update the last_xwindow based on enter and leave > > notifications instead of xwl_xy_to_window(), and check if the xwindow > > found by the regular miXYToWindow() is a child of the known last > > xwindow, so that multiple consecutive calls to xwl_xy_to_window() > > return the same xwindow, being either the one found by miXYToWindow() > > or the root window. > > > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1385258 > > Signed-off-by: Olivier Fourdan > > Tested-by: Vít Ondruch > Tested-by: Satish Balay I have added this patch to the Fedora 25 package for xorg-x11-server-Xwayland-1.19.0 a week or so ago and haven't spotted any new report of a similar crash in Xwayland since then, so I am quite confident this is the right fix for the issue. Cheers, Olivier. ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [RFC xserver 0/3] Allow XWM to control Xwayland commits
Hi Pekka, > this is probably the first time I'm sending patches for the xserver, so > pointing out API misuse, coding style issues etc. would be appreciated. The > last patch also has some XXX comments with questions. > > The first patch, refactoring, is not absolutely necessary (anymore - I used > to > have a use for it while brewing this), but I think it makes things look > nicer. > > The fundamental problem is that Xwayland and the Wayland compositor (window > manager) are racing, particularly when windows are being mapped. This can > lead > to glitches. The specific glitch I bumped into is described at > https://phabricator.freedesktop.org/T7622 . > > The proposed solution allows the WM to temporarily prohibit commits. > > It would be awkward for Weston to postpone mapping certain windows since it > would leak quite much Xwayland into the heart of the window manager. A > Wayland > compositor also cannot ignore commits, because Xwayland will wait for frame > callbacks until it commits again. To not get stuck, one would need to fire > frame callbacks outside of their normal path. It seems much simpler to just > control Xwayland's commits, which also ensures that the first commit will > carry > fully drawn window decorations too. > > This series is the Xwayland side of the fix to T7622. The Weston side is > still > brewing, but I was able to test that the Xwayland code works. I've taken a look at https://phabricator.freedesktop.org/T7622 but I am not sure how that proposal would play with apps that implement the _NET_WM_SYNC protocol, using _NET_WM_SYNC_REQUEST and sync counters as described in EMWH [1] GNOME (mutter,gtk3) go a bit farther even and use an extended version of this protocol described by Owen [2]. I suspect these make the de-syncronization with the Xwayland toplevel decorated by the X window manager even more visible under Wayland, as the repaint is scheduled according to the app content and not sync'ed with Xwayland and repaint of the shadows by the XWM. Would your proposal help there? Cheers, Olivier [1] https://specifications.freedesktop.org/wm-spec/wm-spec-latest.html#idm140200472538288 [2] http://fishsoup.net/misc/wm-spec-synchronization.html [3] https://bugzilla.gnome.org/show_bug.cgi?id=767212 [4] https://bugs.freedesktop.org/show_bug.cgi?id=81275 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver v3] xwayland: Fix use after free of cursors
> Sometimes, Xwayland will try to use a cursor that has just been freed,
> leading to a crash when trying to access that cursor data either in
> miPointerUpdateSprite() or AnimCurTimerNotify().
>
> CheckMotion() updates the pointer's cursor based on which xwindow
> XYToWindow() returns, and Xwayland implements its own xwl_xy_to_window()
> to fake a crossing to the root window when the pointer has left the
> Wayland surface but is still within the xwindow.
>
> But after an xwindow is unrealized, the last xwindow used to match the
> xwindows is cleared so two consecutive calls to xwl_xy_to_window() may
> not return the same xwindow.
>
> To avoid this issue, update the last_xwindow based on enter and leave
> notifications instead of xwl_xy_to_window(), and check if the xwindow
> found by the regular miXYToWindow() is a child of the known last
> xwindow, so that multiple consecutive calls to xwl_xy_to_window()
> return the same xwindow, being either the one found by miXYToWindow()
> or the root window.
>
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1385258
> Signed-off-by: Olivier Fourdan
Tested-by: Vít Ondruch
Tested-by: Satish Balay
> ---
> v2: Issue was caused by inconsistent returned value from
> xwl_xy_to_window() after a window is unrealized, fix the
> issue to update the last_xwindow on enter/leave notify handlers.
> Fix was succesfully tested in:
> https://bugzilla.redhat.com/show_bug.cgi?id=1387281
> v3: Same patch, reword the commit messag to make it shorter and clearer
> (apparently patchwork did not like the long message much...)
>
> hw/xwayland/xwayland-input.c | 17 +
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index 0526122..681bc9d 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -312,6 +312,9 @@ pointer_handle_enter(void *data, struct wl_pointer
> *pointer,
> dx = xwl_seat->focus_window->window->drawable.x;
> dy = xwl_seat->focus_window->window->drawable.y;
>
> +/* We just entered a new xwindow, forget about the old last xwindow */
> +xwl_seat->last_xwindow = NullWindow;
> +
> master = GetMaster(dev, POINTER_OR_FLOAT);
> (*pScreen->SetCursorPosition) (dev, pScreen, dx + sx, dy + sy, TRUE);
>
> @@ -360,8 +363,14 @@ pointer_handle_leave(void *data, struct wl_pointer
> *pointer,
>
> xwl_seat->xwl_screen->serial = serial;
>
> -xwl_seat->focus_window = NULL;
> -CheckMotion(NULL, GetMaster(dev, POINTER_OR_FLOAT));
> +/* The pointer has left a known xwindow, save it for a possible match
> + * in sprite_check_lost_focus()
> + */
> +if (xwl_seat->focus_window) {
> +xwl_seat->last_xwindow = xwl_seat->focus_window->window;
> +xwl_seat->focus_window = NULL;
> +CheckMotion(NULL, GetMaster(dev, POINTER_OR_FLOAT));
> +}
> }
>
> static void
> @@ -1256,10 +1265,10 @@ sprite_check_lost_focus(SpritePtr sprite, WindowPtr
> window)
> */
> if (master->lastSlave == xwl_seat->pointer &&
> xwl_seat->focus_window == NULL &&
> -xwl_seat->last_xwindow == window)
> +xwl_seat->last_xwindow != NullWindow &&
> +IsParent (xwl_seat->last_xwindow, window))
> return TRUE;
>
> -xwl_seat->last_xwindow = window;
> return FALSE;
> }
>
> --
> 2.9.3
>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver v3] xwayland: Fix use after free of cursors
Sometimes, Xwayland will try to use a cursor that has just been freed,
leading to a crash when trying to access that cursor data either in
miPointerUpdateSprite() or AnimCurTimerNotify().
CheckMotion() updates the pointer's cursor based on which xwindow
XYToWindow() returns, and Xwayland implements its own xwl_xy_to_window()
to fake a crossing to the root window when the pointer has left the
Wayland surface but is still within the xwindow.
But after an xwindow is unrealized, the last xwindow used to match the
xwindows is cleared so two consecutive calls to xwl_xy_to_window() may
not return the same xwindow.
To avoid this issue, update the last_xwindow based on enter and leave
notifications instead of xwl_xy_to_window(), and check if the xwindow
found by the regular miXYToWindow() is a child of the known last
xwindow, so that multiple consecutive calls to xwl_xy_to_window()
return the same xwindow, being either the one found by miXYToWindow()
or the root window.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1385258
Signed-off-by: Olivier Fourdan
---
v2: Issue was caused by inconsistent returned value from
xwl_xy_to_window() after a window is unrealized, fix the
issue to update the last_xwindow on enter/leave notify handlers.
Fix was succesfully tested in:
https://bugzilla.redhat.com/show_bug.cgi?id=1387281
v3: Same patch, reword the commit messag to make it shorter and clearer
(apparently patchwork did not like the long message much...)
hw/xwayland/xwayland-input.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index 0526122..681bc9d 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -312,6 +312,9 @@ pointer_handle_enter(void *data, struct wl_pointer *pointer,
dx = xwl_seat->focus_window->window->drawable.x;
dy = xwl_seat->focus_window->window->drawable.y;
+/* We just entered a new xwindow, forget about the old last xwindow */
+xwl_seat->last_xwindow = NullWindow;
+
master = GetMaster(dev, POINTER_OR_FLOAT);
(*pScreen->SetCursorPosition) (dev, pScreen, dx + sx, dy + sy, TRUE);
@@ -360,8 +363,14 @@ pointer_handle_leave(void *data, struct wl_pointer
*pointer,
xwl_seat->xwl_screen->serial = serial;
-xwl_seat->focus_window = NULL;
-CheckMotion(NULL, GetMaster(dev, POINTER_OR_FLOAT));
+/* The pointer has left a known xwindow, save it for a possible match
+ * in sprite_check_lost_focus()
+ */
+if (xwl_seat->focus_window) {
+xwl_seat->last_xwindow = xwl_seat->focus_window->window;
+xwl_seat->focus_window = NULL;
+CheckMotion(NULL, GetMaster(dev, POINTER_OR_FLOAT));
+}
}
static void
@@ -1256,10 +1265,10 @@ sprite_check_lost_focus(SpritePtr sprite, WindowPtr
window)
*/
if (master->lastSlave == xwl_seat->pointer &&
xwl_seat->focus_window == NULL &&
-xwl_seat->last_xwindow == window)
+xwl_seat->last_xwindow != NullWindow &&
+IsParent (xwl_seat->last_xwindow, window))
return TRUE;
-xwl_seat->last_xwindow = window;
return FALSE;
}
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver v2] xwayland: Fix use after free of cursors
Sometimes, Xwayland will try to use a cursor that has just been freed,
leading to a crash when trying to access that cursor data either in
miPointerUpdateSprite() or elsewhere.
This issue is very random and hard to reproduce.
Typical backtraces include:
miPointerUpdateSprite () at mipointer.c
mieqProcessInputEvents () at mieq.c
ProcessInputEvents () at xwayland-input.c
Dispatch () at dispatch.c
dix_main () at main.c
or
miPointerUpdateSprite () at mipointer.c
mieqProcessInputEvents () at mieq.c
keyboard_handle_modifiers () at xwayland-input.c
wl_closure_invoke () at src/connection.c
dispatch_event () at src/wayland-client.c
dispatch_queue () at src/wayland-client.c
wl_display_dispatch_queue_pending () at src/wayland-client.c
wl_display_dispatch_pending () at src/wayland-client.c
xwl_read_events () at xwayland.c
WaitForSomething () at WaitFor.c
Dispatch () at dispatch.c
dix_main () at main.c
or
AnimCurTimerNotify () at animcur.c
DoTimer () at WaitFor.c
DoTimers () at WaitFor.c
check_timers () at WaitFor.c
WaitForSomething () at WaitFor.c
Dispatch () at dispatch.c
dix_main () at main.c
CheckMotion() updates the pointer's cursor based on which window
XYToWindow() returns, and Xwayland implements its own xwl_xy_to_window()
to fake a crossing to the root window when the pointer has left the
Wayland surface but is still within the xwindow.
But Xwayland's xwl_xy_to_window() may not return consistent windows as
it updates the last_xwindow if no match is found.
When an xwindow is unrealized, the last_xwindow and focus_window can be
cleared, which will prevent a match in xwl_xy_to_window() whereas
another call to xwl_xy_to_window() will match the wrong window because
of the previous miss:
* xwl_unrealize_window() clears the seat's focus_window and
last_xwindow.
* xwl_xy_to_window() is called, the last_xwindow (unset) does not match
the window returned by miXYToWindow()
* xwl_xy_to_window() returns the window from miXYToWindow() and updates
the last_xwindow.
* xwl_xy_to_window() is called again, last_xwindow now matches, and the
root window is returned.
So two consecutive calls to xwl_xy_to_window() may not return the same
xwindow.
To avoid this issue, update the last_xwindow in enter and leave notify
handlers only, and check in xwl_xy_to_window() if the window found by
the regular miXYToWindow() is a child of the last_xwindow, so that
multiple calls to xwl_xy_to_window() return the same window, being
either the one found by miXYToWindow() or the root window.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1385258
Signed-off-by: Olivier Fourdan
---
v2: Issue was caused by inconsistent returned value from
xwl_xy_to_window() after a window is unrealized, fix the
issue to update the last_xwindow on enter/leave notify handlers.
Fix was succesfully tested in:
https://bugzilla.redhat.com/show_bug.cgi?id=1387281
hw/xwayland/xwayland-input.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index 0526122..681bc9d 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -312,6 +312,9 @@ pointer_handle_enter(void *data, struct wl_pointer *pointer,
dx = xwl_seat->focus_window->window->drawable.x;
dy = xwl_seat->focus_window->window->drawable.y;
+/* We just entered a new xwindow, forget about the old last xwindow */
+xwl_seat->last_xwindow = NullWindow;
+
master = GetMaster(dev, POINTER_OR_FLOAT);
(*pScreen->SetCursorPosition) (dev, pScreen, dx + sx, dy + sy, TRUE);
@@ -360,8 +363,14 @@ pointer_handle_leave(void *data, struct wl_pointer
*pointer,
xwl_seat->xwl_screen->serial = serial;
-xwl_seat->focus_window = NULL;
-CheckMotion(NULL, GetMaster(dev, POINTER_OR_FLOAT));
+/* The pointer has left a known xwindow, save it for a possible match
+ * in sprite_check_lost_focus()
+ */
+if (xwl_seat->focus_window) {
+xwl_seat->last_xwindow = xwl_seat->focus_window->window;
+xwl_seat->focus_window = NULL;
+CheckMotion(NULL, GetMaster(dev, POINTER_OR_FLOAT));
+}
}
static void
@@ -1256,10 +1265,10 @@ sprite_check_lost_focus(SpritePtr sprite, WindowPtr
window)
*/
if (master->lastSlave == xwl_seat->pointer &&
xwl_seat->focus_window == NULL &&
-xwl_seat->last_xwindow == window)
+xwl_seat->last_xwindow != NullWindow &&
+IsParent (xwl_seat->last_xwindow, window))
return TRUE;
-xwl_seat->last_xwindow = window;
return FALSE;
}
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] xwayland: Fix use after free of cursors
> Sometimes, Xwayland will try to use a cursor that has just been freed, > leading to a crash when trying to access that cursor data either in > miPointerUpdateSprite() or elsewhere. > [...] Right, I think I have to withdraw this patch, because it probably doesn't fix the issue... I still believe the problem finds its root in xwl_xy_to_window() but I still haven't found an appropriate fix. Sorry about that, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: Fix use after free of cursors
Sometimes, Xwayland will try to use a cursor that has just been freed, leading to a crash when trying to access that cursor data either in miPointerUpdateSprite() or elsewhere. This issue is very random and hard to reproduce. Typical backtraces include: miPointerUpdateSprite () at mipointer.c mieqProcessInputEvents () at mieq.c ProcessInputEvents () at xwayland-input.c Dispatch () at dispatch.c dix_main () at main.c or miPointerUpdateSprite () at mipointer.c mieqProcessInputEvents () at mieq.c keyboard_handle_modifiers () at xwayland-input.c wl_closure_invoke () at src/connection.c dispatch_event () at src/wayland-client.c dispatch_queue () at src/wayland-client.c wl_display_dispatch_queue_pending () at src/wayland-client.c wl_display_dispatch_pending () at src/wayland-client.c xwl_read_events () at xwayland.c WaitForSomething () at WaitFor.c Dispatch () at dispatch.c dix_main () at main.c or AnimCurTimerNotify () at animcur.c DoTimer () at WaitFor.c DoTimers () at WaitFor.c check_timers () at WaitFor.c WaitForSomething () at WaitFor.c Dispatch () at dispatch.c dix_main () at main.c CheckMotion() would update the pointer's cursor only when the sprite windows differ before and after calling XYToWindow(), but Xwayland implements its own xwl_xy_to_window() which would fake a crossing to the root window if the pointer has left the Wayland surface but is still within the Xwindow, which confuses CheckMotion(). Typically, after the cursors have been freed from CloseDownClient(), if the pointer's sprite window is already the root window, and Xwayland's xwl_xy_to_window() fakes a transition to the root window as well, the previous and new sprite windows are already identical and CheckMotion() will not call PostNewCursor() and thus not invoke miPointerDisplayCursor() that would have updated the pointer's cursor. Any further attempt to update the pointer using that cursor will lead to a crash. To avoid this issue, modify Xwayland's own xwl_xy_to_window() to avoid returning the root window if the sprite window is already the root window, so that the logic in CheckMotion() is preserved. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1385258 Signed-off-by: Olivier Fourdan --- hw/xwayland/xwayland-input.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c index 0526122..88c12f5 100644 --- a/hw/xwayland/xwayland-input.c +++ b/hw/xwayland/xwayland-input.c @@ -1256,7 +1256,8 @@ sprite_check_lost_focus(SpritePtr sprite, WindowPtr window) */ if (master->lastSlave == xwl_seat->pointer && xwl_seat->focus_window == NULL && -xwl_seat->last_xwindow == window) +xwl_seat->last_xwindow == window && +sprite->win != sprite->spriteTrace[0]) return TRUE; xwl_seat->last_xwindow = window; -- 2.9.3 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Xwayland crash using a free cursor in 1.19
Hi Just a quick heads up, I have been trying to investigate random crashes in Xwayland, something that occurs very randomly and really hard to reproduce. It appears that, sometimes, Xwayland will try to use a pCursor that has just been freed, leading to a crash when trying to use that cursor in miPointerUpdateSprite(). Initially, I thought it might be related to the use of miPointerInvalidateSprite() in Xwayland that sets the pPointer->pSpriteCursor to an invalid value thus forcing the update of the cursor, because each time the crash would occur, pPointer->pSpriteCursor would be 0x1 (the invalid value set in miPointerInvalidateSprite()). Every time, the pPointer->pCursor was freed, with both bits = 0x0 and refcnt = 0. A typical backtrace would look like: miPointerUpdateSprite () at mipointer.c:465 mieqProcessInputEvents () at mieq.c:559 ProcessInputEvents () at xwayland-input.c:1229 Dispatch () at dispatch.c:408 dix_main () at main.c:287 As seen in: https://bugzilla.redhat.com/show_bug.cgi?id=1388976 https://bugzilla.redhat.com/show_bug.cgi?id=1393158 https://bugzilla.redhat.com/show_bug.cgi?id=1385258 (Daniel's been posting in the last one) But I also had another similar crash but with a different code path: miPointerUpdateSprite () at mipointer.c:476 mieqProcessInputEvents () at mieq.c:563 keyboard_handle_modifiers () at xwayland-input.c:677 wl_closure_invoke () at src/connection.c:935 dispatch_event () at src/wayland-client.c:1310 dispatch_queue () at src/wayland-client.c:1456 wl_display_dispatch_queue_pending () at src/wayland-client.c:1698 wl_display_dispatch_pending () at src/wayland-client.c:1761 xwl_read_events () at xwayland.c:565 WaitForSomething () at WaitFor.c:224 Dispatch () at dispatch.c:412 dix_main () at main.c:287 Which makes me think that the use of miPointerInvalidateSprite() is not necessarily the problem. Instrumenting the code and checking if any device would still be using the cursor when freed from FreeCursor(), or checking if a master's cursor refcnt is 0 from mieqProcessInputEvents, I can see that the cursor is freed from CloseDownClient(), i.e. after an X client is gone. So in some rare cases, CloseDownClient() will free the associated cursor(s) and immediately after, mieqProcessInputEvents() will invoke miPointerUpdateSprite() trying to use that (freed) cursor to update the pointer, and crash. Pulling the strings from there, I see that the cursor should be updated in miPointerDisplayCursor() which is called from CheckMotion() (via PostNewCursor()) but only if the new sprite window is different from the old one, and the new one comes from XYToWindow(). Xwayland implements its own XYToWindow() that will return the root window when transitioning from an Xwayland window to a native Wayland surface (so that the X clients get a LeaveNotify event in this case, otherwise they might not be able to tell the cursor has left the X window, that was one of my patches). So now I suspect the problem actually lies in xwl_xy_to_window() which somehow defeats the logic in CheckMotion(), but I don't know how to fix that issue yet... Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: Remove MIPOINTER() definition
Not needed anymore now that mipointer exposes an API for that,
miPointerInvalidateSprite()
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-input.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index 7ec3b1a..0526122 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -35,12 +35,6 @@
#include
#include
-/* Copied from mipointer.c */
-#define MIPOINTER(dev) \
-(IsFloating(dev) ? \
-(miPointerPtr)dixLookupPrivate(&(dev)->devPrivates, miPointerPrivKey):
\
-(miPointerPtr)dixLookupPrivate(&(GetMaster(dev,
MASTER_POINTER))->devPrivates, miPointerPrivKey))
-
struct sync_pending {
struct xorg_list l;
DeviceIntPtr pending_dev;
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xwayland 0/7] Implement zwp_tablet v2
Hey Jason, Carlos, > Okay... I had some remote confusion: > > $ git remote -v > jason https://github.com/wayland-tablet/xserver.git (fetch) > > I picked the most recent branch there as a starting point, but it's > sensibly older than that. I take back these patches, will look into > merging what's good of those with yours. I just started taking a quick look at these patches, I have one remark while at it, if I may, a couple of those patches (4/7 "xwayland: Handle wp_tablet events" and 5/7 "Handle tablet_tool events" for example) use hardcoded values that seem a tad obscure (at least to me) or maybe hardware related, would be nice to give some explanation of where those come from in the code itself. Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] glamor: restore vfunc handlers on init failure
In glamor_init(), if the minimum requirements are not met, glamor may fail after setting up its own CloseScreen() and DestroyPixmap() routines, leading to a crash when either of the two routines is called if glamor failed to complete its initialization, e.g: (EE) Backtrace: (EE) 0: Xwayland (OsSigHandler+0x29) (EE) 1: /lib64/libpthread.so.0 (__restore_rt+0x0) (EE) 2: Xwayland (glamor_sync_close+0x2a) (EE) 3: Xwayland (glamor_close_screen+0x52) (EE) 4: Xwayland (CursorCloseScreen+0x88) (EE) 5: Xwayland (AnimCurCloseScreen+0xa4) (EE) 6: Xwayland (present_close_screen+0x42) (EE) 7: Xwayland (dix_main+0x4f9) (EE) 8: /lib64/libc.so.6 (__libc_start_main+0xf1) (EE) 9: Xwayland (_start+0x2a) Restore the previous CloseScreen() and DestroyPixmap() vfunc handlers in case of failure when checking for the minimum requirements, so that if any of the requirement is not met we don't leave the CloseScreen() and DestroyPixmap() from glamor handlers in place. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1390018 Signed-off-by: Olivier Fourdan --- glamor/glamor.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/glamor/glamor.c b/glamor/glamor.c index b771832..c54cf3b 100644 --- a/glamor/glamor.c +++ b/glamor/glamor.c @@ -470,7 +470,7 @@ glamor_init(ScreenPtr screen, unsigned int flags) LogMessage(X_WARNING, "glamor%d: Failed to allocate screen private\n", screen->myNum); -goto fail; +goto free_glamor_private; } glamor_set_screen_private(screen, glamor_priv); @@ -480,7 +480,7 @@ glamor_init(ScreenPtr screen, unsigned int flags) LogMessage(X_WARNING, "glamor%d: Failed to allocate pixmap private\n", screen->myNum); -goto fail; +goto free_glamor_private; } if (!dixRegisterPrivateKey(&glamor_gc_private_key, PRIVATE_GC, @@ -488,7 +488,7 @@ glamor_init(ScreenPtr screen, unsigned int flags) LogMessage(X_WARNING, "glamor%d: Failed to allocate gc private\n", screen->myNum); -goto fail; +goto free_glamor_private; } glamor_priv->saved_procs.close_screen = screen->CloseScreen; @@ -731,6 +731,11 @@ glamor_init(ScreenPtr screen, unsigned int flags) return TRUE; fail: +/* Restore default CloseScreen and DestroyPixmap handlers */ +screen->CloseScreen = glamor_priv->saved_procs.close_screen; +screen->DestroyPixmap = glamor_priv->saved_procs.destroy_pixmap; + + free_glamor_private: free(glamor_priv); glamor_set_screen_private(screen, NULL); return FALSE; -- 2.9.3 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: Null pointer deref in FlushAllOutput with 1.19-rc1 ?
Hi
> > Multiple Fedora 25 users running 1.19-rc1 are reporting a backtrace
> > related to an InitFonts -> SendErrorToClient -> FlushAllOutput
> > call chain.
> >
> > Since there is no trivial reproducer this is somewhat hard to debug,
> > hence this mail. Anyone have a clue / hint ? See:
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=1382444
>
> Actually, I think we cannot really trust the symbols from Xorg's own
> generated backtrace, however, looking at the addresses, the sequence makes
> some more sense:
>
> FlushAllOutput() in /usr/src/debug/xorg-server-20160929/os/io.c:612
> Dispatch() in /usr/src/debug/xorg-server-20160929/dix/dispatch.c:3491
> dix_main() in /usr/src/debug/xorg-server-20160929/dix/main.c:296
>
> with /usr/src/debug/xorg-server-20160929/os/io.c:612
>
> 612 xorg_list_for_each_entry_safe(client, tmp, &output_pending_clients,
> output_pending) {
> 613 if (client->clientGone)
> 614 continue;
> 615 if (!client_is_ready(client)) {
> 616 oc = (OsCommPtr) client->osPrivate;
> 617 (void) FlushClient(client, oc, (char *) NULL, 0);
> 618 } else
> 619 NewOutputPending = TRUE;
> 620 }
>
> So it could be that output_pending_clients list got corrupted somehow.
>
> Not sure I can go much further than that with so little data, but if that
> rings a bell with someone else...
Some more reports all pointing to FlushAllOutput() with different backtraces,
e.g.:
#6 FlushClient at io.c:938
#7 WriteToClient at io.c:768
#8 WriteEventsToClient at events.c:6000
#9 present_send_complete_notify at present_event.c:172
#10 present_vblank_notify at present.c:213
#11 present_execute at present.c:771
#12 present_pixmap at present.c:963
#13 present_notify_msc at present.c:1014
#14 proc_present_notify_msc at present_request.c:174
#15 Dispatch at dispatch.c:469
or
#6 FlushClient at io.c:938
#7 WriteToClient at io.c:768
#8 ProcGetScreenSaver at dispatch.c:3163
#9 Dispatch at dispatch.c:469
#10 dix_main at main.c:287
with
792 int
793 FlushClient(ClientPtr who, OsCommPtr oc, const void *__extraBuf, int
extraCount)
794 {
...
936
937 if (oco->size > BUFWATERMARK) {
938 free(oco->buf); <== here
939 free(oco);
940 }
941 else {
942 oco->next = FreeOutputs;
943 FreeOutputs = oco;
944 }
The most important change I see affecting this code is the "Switch server to
poll" series, I am not sure how this can be related though.
Also, I don't see any change between xorg-server-20160929 and current git
master, so chances are this is still affecting current git code.
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[xserver PULL for 1.19] One more patch on top of Hans' pull request
[...] > Could we add https://patchwork.freedesktop.org/patch/117161/ to this as well? > > It (hopefully) fixes a random crash in Xwayland with touch devices. > > Or else I can prepare another pull request... The following changes since commit d13cb974426f7f1110b0bdb08c4ebb46ff8975f7: ddx: add new call to purge input devices that weren't added (2016-10-26 15:35:07 +1000) are available in the git repository at: git://people.freedesktop.org/~ofourdan/xserver xwayland for you to fetch changes up to 8c460a77cac6e403299ec81f60745c3e8fb37c01: xwayland: Activate and enable touch devices (2016-10-26 14:16:42 +0200) -------- Olivier Fourdan (1): xwayland: Activate and enable touch devices hw/xwayland/xwayland-input.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [xserver PULL for 1.19 ] Various small fixes for 1.19
Hi, - Original Message - > Hi Adam, Keith, > > Here is a pull-req with various small fixes > (all with at least 1 Reviewed-by) which I've collected > for merging into 1.19: > > The following changes since commit d13cb974426f7f1110b0bdb08c4ebb46ff8975f7: > >ddx: add new call to purge input devices that weren't added (2016-10-26 >15:35:07 +1000) > > are available in the git repository at: > >git://people.freedesktop.org/~jwrdegoede/xserver for-server-1.19 > > for you to fetch changes up to 7b135f5e7d79622c0b922de8ee827a2556504d8f: > >xwayland: Transform pointer enter event coordinates (2016-10-26 11:51:38 >+0200) > > > Hans de Goede (1): >xfree86: Xorg.wrap: Do not require root rights for cards with 0 >outputs > > Michel Dänzer (1): >DRI2: Sync radeonsi_pci_ids.h from Mesa > > Mihail Konev (2): >xwin: make glx optional again >modesetting: fix glamor ifdef > > Nikhil Mahale (1): >modesetting: unifdef MODESETTING_OUTPUT_SLAVE_SUPPORT > > Rui Matos (1): >xwayland: Transform pointer enter event coordinates > > configure.ac | 4 ++-- > hw/xfree86/dri2/pci_ids/radeonsi_pci_ids.h | 12 > hw/xfree86/drivers/modesetting/driver.c | 2 ++ > hw/xfree86/drivers/modesetting/drmmode_display.c | 2 -- > hw/xfree86/xorg-wrapper.c| 2 +- > hw/xwayland/xwayland-input.c | 5 - > 6 files changed, 21 insertions(+), 6 deletions(-) Could we add https://patchwork.freedesktop.org/patch/117161/ to this as well? It (hopefully) fixes a random crash in Xwayland with touch devices. Or else I can prepare another pull request... Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: Null pointer deref in FlushAllOutput with 1.19-rc1 ?
Hi,
> Multiple Fedora 25 users running 1.19-rc1 are reporting a backtrace
> related to an InitFonts -> SendErrorToClient -> FlushAllOutput
> call chain.
>
> Since there is no trivial reproducer this is somewhat hard to debug,
> hence this mail. Anyone have a clue / hint ? See:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1382444
Actually, I think we cannot really trust the symbols from Xorg's own generated
backtrace, however, looking at the addresses, the sequence makes some more
sense:
FlushAllOutput() in /usr/src/debug/xorg-server-20160929/os/io.c:612
Dispatch() in /usr/src/debug/xorg-server-20160929/dix/dispatch.c:3491
dix_main() in /usr/src/debug/xorg-server-20160929/dix/main.c:296
with /usr/src/debug/xorg-server-20160929/os/io.c:612
612 xorg_list_for_each_entry_safe(client, tmp, &output_pending_clients,
output_pending) {
613 if (client->clientGone)
614 continue;
615 if (!client_is_ready(client)) {
616 oc = (OsCommPtr) client->osPrivate;
617 (void) FlushClient(client, oc, (char *) NULL, 0);
618 } else
619 NewOutputPending = TRUE;
620 }
So it could be that output_pending_clients list got corrupted somehow.
Not sure I can go much further than that with so little data, but if that rings
a bell with someone else...
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: Activate and enable touch devices
On some random condition, a touch event may trigger a crash in Xwayland
in GetTouchEvents().
The (simplified) backtrace goes as follow:
(gdb) bt
#0 GetTouchEvents() at getevents.c:1892
#1 QueueTouchEvents() at getevents.c:1866
#2 xwl_touch_send_event() at xwayland-input.c:652
#5 wl_closure_invoke() from libwayland-client.so.0
#6 dispatch_event() from libwayland-client.so.0
#7 wl_display_dispatch_queue_pending() from libwayland-client.so.0
#8 xwl_read_events() at xwayland.c:483
#9 ospoll_wait() at ospoll.c:412
#10 WaitForSomething() at WaitFor.c:222
#11 Dispatch() at dispatch.c:412
#12 dix_main() at main.c:287
#13 __libc_start_main() at libc-start.c:289
#14 _start ()
The crash occurs when trying to access the sprite associated with the
touch device, which appears to be NULL. Reason being the device itself
is more a keyboard device than a touch device.
Moreover, it appears the device is neither enabled nor activated
(inited=0, enabled=0) which doesn't seem right, but matches the code in
init_touch() from xwayland-input.c which would enable the device if it
was previously existing and otherwise would create the device but not
activate it.
Make sure we do activate and enable touch devices just like we do for
other input devices such as keyboard and pointer.
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-input.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index 4d447a5..9a7123a 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -1056,12 +1056,13 @@ init_touch(struct xwl_seat *xwl_seat)
wl_touch_add_listener(xwl_seat->wl_touch,
&touch_listener, xwl_seat);
-if (xwl_seat->touch)
-EnableDevice(xwl_seat->touch, TRUE);
-else {
+if (xwl_seat->touch == NULL) {
xwl_seat->touch =
add_device(xwl_seat, "xwayland-touch", xwl_touch_proc);
+ActivateDevice(xwl_seat->touch, TRUE);
}
+EnableDevice(xwl_seat->touch, TRUE);
+
}
static void
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH v2 xserver] glamor: Fix pixmap offset for bitplane in glamor_copy_fbo_cpu
Commit cba28d5 - "glamor: Handle bitplane in glamor_copy_fbo_cpu"
introduced a regression as the computed pixmap offset would not match
the actual coordinates and write data elsewhere in memory causing a
segfault in fbBltOne().
Translate the pixmap coordinates so that the data is read and written at
the correct location.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97974
Signed-off-by: Olivier Fourdan
Reviewed-and-Tested-by: Michel Dänzer
---
v2: Fix typos in commit message
Reformat as per Michel's review and add Michel's R-b
glamor/glamor_copy.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/glamor/glamor_copy.c b/glamor/glamor_copy.c
index 8a329d2..3ca56fb 100644
--- a/glamor/glamor_copy.c
+++ b/glamor/glamor_copy.c
@@ -230,20 +230,22 @@ glamor_copy_cpu_fbo(DrawablePtr src,
goto bail;
}
+src_pix->drawable.x = -dst->x;
+src_pix->drawable.y = -dst->y;
+
fbGetDrawable(&src_pix->drawable, src_bits, src_stride, src_bpp,
src_xoff,
src_yoff);
if (src->bitsPerPixel > 1)
-fbCopyNto1(src, &src_pix->drawable, gc, box, nbox,
- dst_xoff + dx, dst_yoff + dy, reverse, upsidedown,
- bitplane, closure);
+fbCopyNto1(src, &src_pix->drawable, gc, box, nbox, dx, dy,
+ reverse, upsidedown, bitplane, closure);
else
-fbCopy1toN(src, &src_pix->drawable, gc, box, nbox,
- dst_xoff + dx, dst_yoff + dy, reverse, upsidedown,
- bitplane, closure);
+fbCopy1toN(src, &src_pix->drawable, gc, box, nbox, dx, dy,
+ reverse, upsidedown, bitplane, closure);
-glamor_upload_boxes(dst_pixmap, box, nbox, 0, 0, 0, 0,
-(uint8_t *) src_bits, src_stride * sizeof(FbBits));
+glamor_upload_boxes(dst_pixmap, box, nbox, src_xoff, src_yoff,
+dst_xoff, dst_yoff, (uint8_t *) src_bits,
+src_stride * sizeof(FbBits));
fbDestroyPixmap(src_pix);
} else {
fbGetDrawable(src, src_bits, src_stride, src_bpp, src_xoff, src_yoff);
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH RFC xserver] glamor: Fix pixmap offset for bitplane in glamor_copy_fbo_cpu
Commit cba28d5 - glamor: Handle bitplane in glamor_copy_fbo_cpu
instroduced a regression as the computed pixmap offset would not match
the qactual coordinates and write data elsewhere in memory causing a
segfault in fbBltOne().
Translate the pixmap coordinates so that the data is read and written at
the correct location.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97974
Signed-off-by: Olivier Fourdan
---
Note: This fixes the crash apparerntly and produces the expected output,
as tested with xterm's contectual menu, the check boxes are drawn
correctly and placed where they should be.
Yet, I am not familiar with this code so I have no idea if ths is
the correct fix for the problem, or even a fix at all.
But it avoids the crash and the regression here.
glamor/glamor_copy.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/glamor/glamor_copy.c b/glamor/glamor_copy.c
index 8a329d2..1fd863a 100644
--- a/glamor/glamor_copy.c
+++ b/glamor/glamor_copy.c
@@ -230,19 +230,22 @@ glamor_copy_cpu_fbo(DrawablePtr src,
goto bail;
}
+src_pix->drawable.x = - dst->x;
+src_pix->drawable.y = - dst->y;
+
fbGetDrawable(&src_pix->drawable, src_bits, src_stride, src_bpp,
src_xoff,
src_yoff);
if (src->bitsPerPixel > 1)
fbCopyNto1(src, &src_pix->drawable, gc, box, nbox,
- dst_xoff + dx, dst_yoff + dy, reverse, upsidedown,
+ dx, dy, reverse, upsidedown,
bitplane, closure);
else
fbCopy1toN(src, &src_pix->drawable, gc, box, nbox,
- dst_xoff + dx, dst_yoff + dy, reverse, upsidedown,
+ dx, dy, reverse, upsidedown,
bitplane, closure);
-glamor_upload_boxes(dst_pixmap, box, nbox, 0, 0, 0, 0,
+glamor_upload_boxes(dst_pixmap, box, nbox, src_xoff, src_yoff,
dst_xoff, dst_yoff,
(uint8_t *) src_bits, src_stride * sizeof(FbBits));
fbDestroyPixmap(src_pix);
} else {
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver v2] xwayland: Apply "last pointer window" check only to the pointer device
Hi Carlos,
> The checks in xwayland's XYToWindow handler pretty much assumes that the
> sprite is managed by the wl_pointer, which is not entirely right, given
> 1) The Virtual Core Pointer may be controlled from other interfaces, and
> 2) there may be other SpriteRecs than the VCP's.
>
> This makes XYToWindow calls return a sprite trace with just the root
> window if any of those two assumptions are broken, eg. on touch events.
>
> So turn the check upside down, first assume that the default XYToWindow
> proc behavior is right, and later cut down the spriteTrace if the
> current device happens to be the pointer and is out of focus. We work
> our way to the device's lastSlave here so as not to break assumption #1
> above.
>
> Signed-off-by: Carlos Garnacho
> ---
> hw/xwayland/xwayland-input.c | 59
> ++--
> 1 file changed, 40 insertions(+), 19 deletions(-)
>
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index 32cfb35..029ae04 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -945,44 +945,65 @@ DDXRingBell(int volume, int pitch, int duration)
> {
> }
>
> -static WindowPtr
> -xwl_xy_to_window(ScreenPtr screen, SpritePtr sprite, int x, int y)
> +static Bool
> +sprite_check_lost_focus(SpritePtr sprite, WindowPtr window)
> {
> -struct xwl_seat *xwl_seat = NULL;
> -DeviceIntPtr device;
> -WindowPtr ret;
> +DeviceIntPtr device, master;
> +struct xwl_seat *xwl_seat;
>
> for (device = inputInfo.devices; device; device = device->next) {
> +/* Ignore non-wayland devices */
> if (device->deviceProc == xwl_pointer_proc &&
> -device->spriteInfo->sprite == sprite) {
> -xwl_seat = device->public.devicePrivate;
> +device->spriteInfo->sprite == sprite)
> break;
> -}
> }
>
> -if (xwl_seat == NULL) {
> -sprite->spriteTraceGood = 1;
> -return sprite->spriteTrace[0];
> -}
> +if (!device)
> +return FALSE;
> +
> +xwl_seat = device->public.devicePrivate;
> +
> +master = GetMaster(device, POINTER_OR_FLOAT);
> +if (!master || !master->lastSlave)
> +return FALSE;
>
> -screen->XYToWindow = xwl_seat->xwl_screen->XYToWindow;
> +/* We do want the last active slave, we only check on slave xwayland
> devices
> + * so we can find out the xwl_seat, but those don't actually own their
> + * sprite, so the match doesn't mean a lot.
> + */
> +if (master->lastSlave == xwl_seat->pointer &&
> +xwl_seat->focus_window == NULL &&
> +xwl_seat->last_xwindow == window)
> +return TRUE;
> +
> +xwl_seat->last_xwindow = window;
> +return FALSE;
> +}
> +
> +static WindowPtr
> +xwl_xy_to_window(ScreenPtr screen, SpritePtr sprite, int x, int y)
> +{
> +struct xwl_screen *xwl_screen;
> +WindowPtr ret;
> +
> +xwl_screen = xwl_screen_get(screen);
> +
> +screen->XYToWindow = xwl_screen->XYToWindow;
> ret = screen->XYToWindow(screen, sprite, x, y);
> -xwl_seat->xwl_screen->XYToWindow = screen->XYToWindow;
> +xwl_screen->XYToWindow = screen->XYToWindow;
> screen->XYToWindow = xwl_xy_to_window;
>
> -/* If the pointer has left the Wayland surface but the DIX still
> - * finds the pointer within the previous X11 window, it means that
> +/* If the device controlling the sprite has left the Wayland surface but
> + * the DIX still finds the pointer within the X11 window, it means that
> * the pointer has crossed to another native Wayland window, in this
> * case, pretend we entered the root window so that a LeaveNotify
> * event is emitted.
> */
> -if (xwl_seat->focus_window == NULL && xwl_seat->last_xwindow == ret) {
> +if (sprite_check_lost_focus(sprite, ret)) {
> sprite->spriteTraceGood = 1;
> return sprite->spriteTrace[0];
> }
>
> -xwl_seat->last_xwindow = ret;
> -
> return ret;
> }
It just feels odd that the function is called sprite_check_lost_focus() and
updates the xwl_seat's last_xwindow value.
I am no input expert so maybe you'll want to wait for someone else's better
review, but as far as I can see it looks good to me, so:
Acked-by: Olivier Fourdan
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver 1/2] xwayland: Apply "last pointer window" check only to the pointer device
Hey Carlos,
- Original Message -
> The checks in xwayland's XYToWindow handler pretty much assumes that the
> sprite is managed by the wl_pointer, which is not entirely right, given
> 1) The Virtual Core Pointer may be controlled from other interfaces, and
> 2) there may be other SpriteRecs than the VCP's.
>
> This makes XYToWindow calls return a sprite trace with just the root
> window if any of those two assumptions are broken, eg. on touch events.
>
> So turn the check upside down, first assume that the default XYToWindow
> proc behavior is right, and later cut down the spriteTrace if the
> current device happens to be the pointer. We work our way to the
> device's lastSlave here so as not to break assumption #1 above.
>
> Also, simplify the actual nested call to XYToWindow, the method pointer
> in ScreenPtr was reinstaurated before calling, and moved back to
> xwl_screen domain afterwards. This seems kind of pointless, as we have
> the function pointer anyway.
>
> Signed-off-by: Carlos Garnacho
> ---
> hw/xwayland/xwayland-input.c | 64
> ++--
> 1 file changed, 38 insertions(+), 26 deletions(-)
>
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index 32cfb35..9b385dc 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -945,43 +945,55 @@ DDXRingBell(int volume, int pitch, int duration)
> {
> }
>
> -static WindowPtr
> -xwl_xy_to_window(ScreenPtr screen, SpritePtr sprite, int x, int y)
> +static DeviceIntPtr
> +sprite_get_last_active_device(SpritePtr sprite, struct xwl_seat **xwl_seat)
> {
> -struct xwl_seat *xwl_seat = NULL;
> DeviceIntPtr device;
> -WindowPtr ret;
>
> for (device = inputInfo.devices; device; device = device->next) {
> -if (device->deviceProc == xwl_pointer_proc &&
> -device->spriteInfo->sprite == sprite) {
> -xwl_seat = device->public.devicePrivate;
> +/* Ignore non-wayland devices */
> +if (device->deviceProc != xwl_pointer_proc &&
> +device->deviceProc != xwl_touch_proc &&
> +device->deviceProc != xwl_keyboard_proc)
> +continue;
> +
> +if (device->spriteInfo->sprite == sprite)
> break;
> -}
> }
>
> -if (xwl_seat == NULL) {
> -sprite->spriteTraceGood = 1;
> -return sprite->spriteTrace[0];
> -}
> +if (!device || IsFloating(device))
> +return NULL;
>
> -screen->XYToWindow = xwl_seat->xwl_screen->XYToWindow;
> -ret = screen->XYToWindow(screen, sprite, x, y);
> -xwl_seat->xwl_screen->XYToWindow = screen->XYToWindow;
> -screen->XYToWindow = xwl_xy_to_window;
That wrapping is on purpose, see Daniel's review on my initial patch here:
https://lists.x.org/archives/xorg-devel/2016-June/050238.html
> +*xwl_seat = device->public.devicePrivate;
>
> -/* If the pointer has left the Wayland surface but the DIX still
> - * finds the pointer within the previous X11 window, it means that
> - * the pointer has crossed to another native Wayland window, in this
> - * case, pretend we entered the root window so that a LeaveNotify
> - * event is emitted.
> +/* We do want the last active slave, we only check on slave xwayland
> devices
> + * so we can find out the xwl_seat, but those don't actually own their
> + * sprite, so the match doesn't mean a lot.
> */
> -if (xwl_seat->focus_window == NULL && xwl_seat->last_xwindow == ret) {
> -sprite->spriteTraceGood = 1;
> -return sprite->spriteTrace[0];
> -}
> +return device->master->lastSlave;
I wonder, would it make sense to use the GetMaster() API here or else check if
master is actually non-null?
(note, I tried to detach the xwayland-pointer (with xinput float) from its
master, it doesn't crash because we actually get no more events from Xwayland
for this device after that)
> +}
>
> -xwl_seat->last_xwindow = ret;
> +static WindowPtr
> +xwl_xy_to_window(ScreenPtr screen, SpritePtr sprite, int x, int y)
> +{
> +struct xwl_seat *xwl_seat = NULL;
> +struct xwl_screen *xwl_screen;
> +DeviceIntPtr device;
> +WindowPtr ret;
> +
> +xwl_screen = xwl_screen_get(screen);
> +ret = xwl_screen->XYToWindow(screen, sprite, x, y);
> +
> +device = sprite_get_last_active_device(sprite, &xwl_seat);
Do you plan to reuse that sprite_get_last_active_device() elsewhere?
If not, the whole logic below (device && xwl_seat && xwl_seat->pointer ==
device) could be moved to the function that would return either the right
xwl_seat or NULL, as we don't actually use the value of device here, apart from
this test.
> +if (device && xwl_seat && xwl_seat->pointer == device) {
> +if (xwl_seat->focus_window == NULL && xwl_seat->last_xwindow == ret)
> {
> +sprite->spriteTraceGood = 1;
> +return sprite->spriteTrace[0];
> +}
> +
>
[Pull request]: couple of fixes for xwayland
The following changes since commit d0c5d205a919fc1d2eb599356090b58b1bf0176d: dix: Make InitCoreDevices() failures more verbose. (2016-09-21 21:11:40 +1000) are available in the git repository at: git://people.freedesktop.org/~ofourdan/xserver xwayland for you to fetch changes up to 0a20d5f8cb3f3f9a81b2795ae1865e72541022d4: xwayland: Clear up x_cursor on UnrealizeCursor() (2016-09-23 09:00:58 +0200) Olivier Fourdan (2): xwayland: handle EAGAIN on Wayland fd xwayland: Clear up x_cursor on UnrealizeCursor() Rui Matos (1): xwayland: Close the shm fd as early as possible hw/xwayland/xwayland-cursor.c | 7 --- hw/xwayland/xwayland-shm.c| 46 +++--- hw/xwayland/xwayland.c| 34 +++--- hw/xwayland/xwayland.h| 1 + 4 files changed, 55 insertions(+), 33 deletions(-) ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: Clear up x_cursor on UnrealizeCursor()
In Xwayland's xwl_unrealize_cursor(), the x_cursor is cleared up only
when a device value is provided to the UnrealizeCursor() routine, but
if the device is NULL as called from FreeCursor(), the corresponding
x_cursor for the xwl_seat is left untouched.
This might cause a segfault when trying to access the unrealized
cursor's devPrivates in xwl_seat_set_cursor().
A possible occurrence of this is the client changing the cursor, the
Xserver calling FreeCursor() which does UnrealizeCursor() and then
the Wayland server sending a pointer enter event, which invokes
xwl_seat_set_cursor() while the seat's x_cursor has just been
unrealized.
To avoid this, walk through all the xwl_seats and clear up all x_cursor
matching the cursor being unrealized.
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-cursor.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/xwayland/xwayland-cursor.c b/hw/xwayland/xwayland-cursor.c
index 7d14a3d..0f22b5d 100644
--- a/hw/xwayland/xwayland-cursor.c
+++ b/hw/xwayland/xwayland-cursor.c
@@ -76,7 +76,8 @@ static Bool
xwl_unrealize_cursor(DeviceIntPtr device, ScreenPtr screen, CursorPtr cursor)
{
PixmapPtr pixmap;
-struct xwl_seat *xwl_seat;
+struct xwl_screen *xwl_screen;
+struct xwl_seat *xwl_seat, *next_xwl_seat;
pixmap = dixGetPrivate(&cursor->devPrivates, &xwl_cursor_private_key);
if (!pixmap)
@@ -85,9 +86,10 @@ xwl_unrealize_cursor(DeviceIntPtr device, ScreenPtr screen,
CursorPtr cursor)
dixSetPrivate(&cursor->devPrivates, &xwl_cursor_private_key, NULL);
/* When called from FreeCursor(), device is always NULL */
-if (device) {
-xwl_seat = device->public.devicePrivate;
-if (xwl_seat && cursor == xwl_seat->x_cursor)
+xwl_screen = xwl_screen_get(screen);
+xorg_list_for_each_entry_safe(xwl_seat, next_xwl_seat,
+ &xwl_screen->seat_list, link) {
+if (cursor == xwl_seat->x_cursor)
xwl_seat->x_cursor = NULL;
}
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver v2] xwayland: Clear up x_cursor on UnrealizeCursor()
In Xwayland's xwl_unrealize_cursor(), the x_cursor is cleared up only
when a device value is provided to the UnrealizeCursor() routine, but
if the device is NULL as called from FreeCursor(), the corresponding
x_cursor for the xwl_seat is left untouched.
This might cause a segfault when trying to access the unrealized
cursor's devPrivates in xwl_seat_set_cursor().
A possible occurrence of this is the client changing the cursor, the
Xserver calling FreeCursor() which does UnrealizeCursor() and then
the Wayland server sending a pointer enter event, which invokes
xwl_seat_set_cursor() while the seat's x_cursor has just been
unrealized.
To avoid this, walk through all the xwl_seats and clear up all x_cursor
matching the cursor being unrealized.
Signed-off-by: Olivier Fourdan
Reviewed-by: Jonas Ådahl
---
v2: Use xorg_list_for_each_entry() instead of xorg_list_for_each_entry_safe()
as suggested by Jonas and add his r-b as per his review.
hw/xwayland/xwayland-cursor.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/hw/xwayland/xwayland-cursor.c b/hw/xwayland/xwayland-cursor.c
index 7d14a3d..b2ae93f 100644
--- a/hw/xwayland/xwayland-cursor.c
+++ b/hw/xwayland/xwayland-cursor.c
@@ -76,6 +76,7 @@ static Bool
xwl_unrealize_cursor(DeviceIntPtr device, ScreenPtr screen, CursorPtr cursor)
{
PixmapPtr pixmap;
+struct xwl_screen *xwl_screen;
struct xwl_seat *xwl_seat;
pixmap = dixGetPrivate(&cursor->devPrivates, &xwl_cursor_private_key);
@@ -85,9 +86,9 @@ xwl_unrealize_cursor(DeviceIntPtr device, ScreenPtr screen,
CursorPtr cursor)
dixSetPrivate(&cursor->devPrivates, &xwl_cursor_private_key, NULL);
/* When called from FreeCursor(), device is always NULL */
-if (device) {
-xwl_seat = device->public.devicePrivate;
-if (xwl_seat && cursor == xwl_seat->x_cursor)
+xwl_screen = xwl_screen_get(screen);
+xorg_list_for_each_entry(xwl_seat, &xwl_screen->seat_list, link) {
+if (cursor == xwl_seat->x_cursor)
xwl_seat->x_cursor = NULL;
}
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] modesetting: Fix build without glamor
> There already was a fix for this posted a while ago, > but that still needs to be merged. Ah yeah, a couple of times even :) Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] modesetting: Fix build without glamor
Build would abort if configure without glamor:
| present.c: error: implicit declaration of function
| ‘ms_flush_drm_events’ [-Werror=implicit-function-declaration]
| if (errno != EBUSY || ms_flush_drm_events(screen) < 0) {
ms_flush_drm_events() is avaialble only with glamor, so avoid the
compilation error by putting th code that use it within an
Signed-off-by: Olivier Fourdan
---
hw/xfree86/drivers/modesetting/present.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/xfree86/drivers/modesetting/present.c
b/hw/xfree86/drivers/modesetting/present.c
index 55b622c..ed2b813 100644
--- a/hw/xfree86/drivers/modesetting/present.c
+++ b/hw/xfree86/drivers/modesetting/present.c
@@ -71,6 +71,7 @@ ms_present_get_ust_msc(RRCrtcPtr crtc, CARD64 *ust, CARD64
*msc)
return ms_get_crtc_ust_msc(xf86_crtc, ust, msc);
}
+#ifdef GLAMOR
/*
* Called when the queued vblank event has occurred
*/
@@ -98,7 +99,7 @@ ms_present_vblank_abort(void *data)
free(event);
}
-
+#endif
/*
* Queue an event to report back to the Present extension when the specified
* MSC has past
@@ -108,6 +109,7 @@ ms_present_queue_vblank(RRCrtcPtr crtc,
uint64_t event_id,
uint64_t msc)
{
+#ifdef GLAMOR
xf86CrtcPtr xf86_crtc = crtc->devPrivate;
ScreenPtr screen = crtc->pScreen;
ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
@@ -149,6 +151,7 @@ ms_present_queue_vblank(RRCrtcPtr crtc,
DebugPresent(("\t\tmq %lld seq %u msc %llu (hw msc %u)\n",
(long long) event_id, seq, (long long) msc,
vbl.request.sequence));
+#endif
return Success;
}
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver v4] xwayland: handle EAGAIN on Wayland fd
wl_display_flush() can fail with EAGAIN and Xwayland would make this a
fatal error.
When this happens, it means that Xwayland has flooded the Wayland file
descriptor, either because the Wayland compositor cannot cope or more
likely because of a deadlock situation where the Wayland compositor is
blocking, waiting for an X reply while Xwayland tries to write data to
the Wayland file descriptor.
The general consensus to avoid the deadlock is for the Wayland
compositor to never issue blocking X11 roundtrips, but in practice
blocking rountrips can occur in various places, including Xlib calls
themselves so this is not always achievable without major surgery in the
Wayland compositor/Window manager.
What this patch does is to avoid dispatching to the Wayland file
descriptor until it becomes available for writing again, while at the
same time continue processing X11 requests to release the deadlock.
This is not perfect, as there is still the possibility of another X
client hammering the connection and we'll still fail writing to the
Wayland connection eventually, but this improves things enough to avoid
a 100% repeatable crash with vlc and gtkperf.
Also, it is worth considering that window managers and Wayland
compositors such as mutter already have a higher priority than other
regular X clients thanks to XSyncSetPriority(), mitigating the risk.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1278159
Bugzilla: https://bugzilla.gnome.org/show_bug.cgi?id=763400
Signed-off-by: Olivier Fourdan
Reviewed-by: Daniel Stone
---
v2: oops, need to poll() on EAGAIN between retries
v3: Mostly a rewrite, non-blocking on poll()
v4: Check for EINTR after poll() and add R-b from Daniel
hw/xwayland/xwayland.c | 34 +++---
hw/xwayland/xwayland.h | 1 +
2 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 847321e..46a2dfc 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -33,6 +33,7 @@
#include
#include
#include
+#include
#ifdef XF86VIDMODE
#include
@@ -470,6 +471,9 @@ xwl_read_events (struct xwl_screen *xwl_screen)
{
int ret;
+if (xwl_screen->wait_flush)
+return;
+
ret = wl_display_read_events(xwl_screen->display);
if (ret == -1)
FatalError("failed to read Wayland events: %s\n", strerror(errno));
@@ -481,10 +485,25 @@ xwl_read_events (struct xwl_screen *xwl_screen)
FatalError("failed to dispatch Wayland events: %s\n", strerror(errno));
}
+static int
+xwl_display_pollout (struct xwl_screen *xwl_screen, int timeout)
+{
+struct pollfd poll_fd;
+
+poll_fd.fd = wl_display_get_fd(xwl_screen->display);
+poll_fd.events = POLLOUT;
+
+return xserver_poll(&poll_fd, 1, timeout);
+}
+
static void
xwl_dispatch_events (struct xwl_screen *xwl_screen)
{
-int ret;
+int ret = 0;
+int ready;
+
+if (xwl_screen->wait_flush)
+goto pollout;
while (xwl_screen->prepare_read == 0 &&
wl_display_prepare_read(xwl_screen->display) == -1) {
@@ -496,9 +515,18 @@ xwl_dispatch_events (struct xwl_screen *xwl_screen)
xwl_screen->prepare_read = 1;
-ret = wl_display_flush(xwl_screen->display);
-if (ret == -1)
+pollout:
+ready = xwl_display_pollout(xwl_screen, 5);
+if (ready == -1 && errno != EINTR)
+FatalError("error polling on XWayland fd: %s\n", strerror(errno));
+
+if (ready > 0)
+ret = wl_display_flush(xwl_screen->display);
+
+if (ret == -1 && errno != EAGAIN)
FatalError("failed to write to XWayland fd: %s\n", strerror(errno));
+
+xwl_screen->wait_flush = (ready == 0 || ready == -1 || ret == -1);
}
static void
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index db3dd0b..3ce7a63 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -83,6 +83,7 @@ struct xwl_screen {
#define XWL_FORMAT_RGB565 (1 << 2)
int prepare_read;
+int wait_flush;
char *device_name;
int drm_fd;
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver v3] xwayland: handle EAGAIN on Wayland fd
Hi Pekka,
> all the rationale in this commit message looks good to me, just some
> things I'd like to ask more about.
>
> > What this patch does is to avoid dispatching to the Wayland file
> > descriptor until it becomes available for writing again, while at the
> > same time continue processing X11 requests to release the deadlock.
>
> Could you expand on why the Wayland fd should not be dispatched while
> you wait for it to flush?
>
> Is it just because dispatching it is likely to cause more Wayland
> requests to be sent? I wonder how that holds up. Maybe it doesn't
> matter as the premise is that the Wayland compositor is stuck.
Actually, the idea is to minimize the risk of breaking further down in
libwayland itself, either in copy_fds_to_connection()
("request could not be marshaled: can't send file descriptor") or in
wl_proxy_marshal_array_constructor_versioned() ("Error sending request:
Resource temporarily unavailable")
> > This is not perfect, as there is still the possibility of another X
> > client hammering the connection and we'll still fail writing to the
> > Wayland connection eventually, but this improves things enough to avoid
> > a 100% repeatable crash with vlc and gtkperf.
> >
> > Also, it is worth considering that window managers and Wayland
> > compositors such as mutter already have a higher priority than other
> > regular X clients thanks to XSyncSetPriority(), mitigating the risk.
>
> The blocking X11 calls from the Wayland compositor - are they all of
> the nature that the X server will reply to them immediately when it
> reads them, or could the reply be delayed allowing other X11 clients to
> be serviced in the mean time?
Either requests or replies can be queued up while new ones come in, the
previous version of that patch (one that I didn't send) was setting the
Xserver's "dispatchException" (see ajax' reply) which has the effect of going
straight to FlushAllOutput() to dequeue the replies without processing new
incoming requests, but that would still leave the door open for a deadlock if
the X11 requests wasn't read yet by the Xserver when the Wayland fd flooding
occurs, as it would not process new incoming requests - I was able to reproduce
this with gtkperf (another case of that issue).
> If all replies are immediate, it sounds like this would be a fairly
> reliable fix after all.
To be honest, I wouldn't consider this a fix, merely a workaround, not to say a
ugly hack, but afaics from my testing here, it avoids the lock (even though,
with vlc fullscreen, the deadlock reoccur continuously when the pointer hovers
the timeline as the tooltip, a shaped window, gets mapped/unmapped continuously
which causes mutter to issue a lot of those blocking rountrips) - Yet it offers
a chance to get out of the deadlock (eventually...) and not lose Xwayland,
which for gnome-shell/mutter means losing the entire user session.
Note: Reviewing my past logs, I think it's worth checking EINTR after the
poll() returns...
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver v3] xwayland: handle EAGAIN on Wayland fd
wl_display_flush() can fail with EAGAIN and Xwayland would make this a
fatal error.
When this happens, it means that Xwayland has flooded the Wayland file
descriptor, either because the Wayland compositor cannot cope or more
likely because of a deadlock situation where the Wayland compositor is
blocking, waiting for an X reply while Xwayland tries to write data to
the Wayland file descriptor.
The general consensus to avoid the deadlock is for the Wayland
compositor to never issue blocking X11 roundtrips, but in practice
blocking rountrips can occur in various places, including Xlib calls
themselves so this is not always achievable without major surgery in the
Wayland compositor/Window manager.
What this patch does is to avoid dispatching to the Wayland file
descriptor until it becomes available for writing again, while at the
same time continue processing X11 requests to release the deadlock.
This is not perfect, as there is still the possibility of another X
client hammering the connection and we'll still fail writing to the
Wayland connection eventually, but this improves things enough to avoid
a 100% repeatable crash with vlc and gtkperf.
Also, it is worth considering that window managers and Wayland
compositors such as mutter already have a higher priority than other
regular X clients thanks to XSyncSetPriority(), mitigating the risk.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1278159
Bugzilla: https://bugzilla.gnome.org/show_bug.cgi?id=763400
Signed-off-by: Olivier Fourdan
---
v2: oops, need to poll() on EAGAIN between retries
v3: Mostly a rewrite, non-blocking on poll()
hw/xwayland/xwayland.c | 34 +++---
hw/xwayland/xwayland.h | 1 +
2 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 847321e..2efbff8 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -33,6 +33,7 @@
#include
#include
#include
+#include
#ifdef XF86VIDMODE
#include
@@ -470,6 +471,9 @@ xwl_read_events (struct xwl_screen *xwl_screen)
{
int ret;
+if (xwl_screen->wait_flush)
+return;
+
ret = wl_display_read_events(xwl_screen->display);
if (ret == -1)
FatalError("failed to read Wayland events: %s\n", strerror(errno));
@@ -481,10 +485,25 @@ xwl_read_events (struct xwl_screen *xwl_screen)
FatalError("failed to dispatch Wayland events: %s\n", strerror(errno));
}
+static int
+xwl_display_pollout (struct xwl_screen *xwl_screen, int timeout)
+{
+struct pollfd poll_fd;
+
+poll_fd.fd = wl_display_get_fd(xwl_screen->display);
+poll_fd.events = POLLOUT;
+
+return xserver_poll(&poll_fd, 1, timeout);
+}
+
static void
xwl_dispatch_events (struct xwl_screen *xwl_screen)
{
-int ret;
+int ret = 0;
+int ready;
+
+if (xwl_screen->wait_flush)
+goto pollout;
while (xwl_screen->prepare_read == 0 &&
wl_display_prepare_read(xwl_screen->display) == -1) {
@@ -496,9 +515,18 @@ xwl_dispatch_events (struct xwl_screen *xwl_screen)
xwl_screen->prepare_read = 1;
-ret = wl_display_flush(xwl_screen->display);
-if (ret == -1)
+pollout:
+ready = xwl_display_pollout(xwl_screen, 5);
+if (ready == -1)
+FatalError("error polling on XWayland fd: %s\n", strerror(errno));
+
+if (ready > 0)
+ret = wl_display_flush(xwl_screen->display);
+
+if (ret == -1 && errno != EAGAIN)
FatalError("failed to write to XWayland fd: %s\n", strerror(errno));
+
+xwl_screen->wait_flush = (ready == 0 || ret == -1);
}
static void
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index db3dd0b..3ce7a63 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -83,6 +83,7 @@ struct xwl_screen {
#define XWL_FORMAT_RGB565 (1 << 2)
int prepare_read;
+int wait_flush;
char *device_name;
int drm_fd;
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: weird Xwayland and compositor deadlock issue [WAS: [PATCH xserver v2] xwayland: handle EAGAIN and EINTR gracefully]
Hi Pekka, - Original Message - > Hi Olivier, > > I don't have any solution for you. The interactions between the Wayland > compositor and Xwayland are known to be very easily deadlockable IIRC. I > believe the only thing you can do is ensure no such case can ever > occur, which is very painful. That is, never do a blocking roundtrip at > least from one side. > > Have the recent modifications caused a significant increase of Wayland > requests from Xwayland? If Xwayland needs to send an amount of data > bigger than bufferable, *any* blocking roundtrip via X11 from the > Wayland compositor is prone to deadlock. It will be waiting for a reply > via X11, while Xwayland is blocked on flushing, since the Wayland > compositor is not consuming requests. > > It can also trivially happen if both sides do a blocking roundtrip at > the same time. Or just a wait for an event. > > Either server needs to be able to return to its main loop to process the > protocol stream it is the server for. Preferably both, I think. Unfortunately, any XSync (like, for example, called in gdk_error_trap_pop() in gdk) will issue a blocking roundtrip, and window managers tend to do that quite a lot (some more than others) so I don't think we can easily chaneg that in window managers to avoid blocking rountrips on X11 side. > You could check how Weston's XWM works. I highly suspect that after > Xwayland launch it avoids doing any blocking roundtrips via X11. Yet sometimes some X calls are blocking, e.g. XShapeGetRectangles() or even XGetWindowAttributes() which is invoked by mutter each time the a new window is mapped. mutter still uses Xlib and not xcb. > I'd assume Xwayland also tries to avoid blocking on Wayland events, but > if nothing else, I believe Mesa via GLAMOR may block on > wl_buffer.release events... or maybe not if GLAMOR is smart with its > throttling. Anyway, since your flush is hitting EAGAIN, that doesn't > seem to be the cause. > > I wonder if making wl_display_flush() block immediately like in your > patch could be replaced by adding the wl_display fd to the main poll > loop, so that it would get flushed ASAP but still service X11 requests > in the mean time? It does run the risk of overflowing the Wayland send > buffer in Xwayland. Any way to prioritize the Wayland compositor's X11 > connection in Xwayland? If I don't make EAGAIN a FatalError() and wait for the Wayland display file descriptor to become writable again, Xwayland eventually dies with another error "(EE) request could not be marshaled: can't send file descriptor" from libwayland directly (in copy_fds_to_connection()). So I am at lost... Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: weird Xwayland and compositor deadlock issue [WAS: [PATCH xserver v2] xwayland: handle EAGAIN and EINTR gracefully]
Hi all - Original Message - > wl_display_flush() can fail with EAGAIN and Xwayland would make this a > fatal error. > > Handle the usual EAGAIN and EINTR gracefully so that Xwayland doesn't > die for so little. Right, I am running out of ideas... So the approach of using poll() to wait for the Wayland file descriptor to become writeable again leads straight to a deadlock apparently... Reason for this is the compositor (gnome-shell/mutter) is itself waiting for data on the X file descriptor: Backtrace of gnome-shell while we hit the EAGAIN case on the Wayland fd on the Xwayland side: #0 0x7f86d1cd400d in poll () at /lib64/libc.so.6 #1 0x7f86d1537d10 in _xcb_conn_wait () at /lib64/libxcb.so.1 #2 0x7f86d1539aa9 in xcb_wait_for_event () at /lib64/libxcb.so.1 #3 0x7f86d21fe03b in _XReadEvents (dpy=dpy@entry=0x55f956633000) at xcb_io.c:401 #4 0x7f86d21e562e in XIfEvent (dpy=0x55f956633000, event=0x7ffe30c28eb0, predicate=, arg=0x55f956761100) at IfEvent.c:68 #5 0x7f86d8031ddb in meta_display_get_current_time_roundtrip () at /lib64/libmutter.so.0 #6 0x7f86d805ac49 in handle_other_xevent () at /lib64/libmutter.so.0 #7 0x7f86d805b95b in xevent_filter () at /lib64/libmutter.so.0 #8 0x7f86d73b98f1 in gdk_event_apply_filters () at /lib64/libgdk-3.so.0 #9 0x7f86d73b9cf2 in _gdk_x11_display_queue_events () at /lib64/libgdk-3.so.0 #10 0x7f86d7380f19 in gdk_display_get_event () at /lib64/libgdk-3.so.0 #11 0x7f86d73b9962 in gdk_event_source_dispatch () at /lib64/libgdk-3.so.0 #12 0x7f86d37d0f22 in g_main_context_dispatch () at /lib64/libglib-2.0.so.0 #13 0x7f86d37d12a0 in g_main_context_iterate.isra () at /lib64/libglib-2.0.so.0 #14 0x7f86d37d15c2 in g_main_loop_run () at /lib64/libglib-2.0.so.0 #15 0x7f86d803c00c in meta_run () at /lib64/libmutter.so.0 #16 0x55f953220657 in main () i.e gnome-shell is stuck in meta_display_get_current_time_roundtrip(): https://git.gnome.org/browse/mutter/tree/src/core/display.c#n1300 While at the same time, Xwayland is trying to write to the Wayland file descriptor with wl_display_flush() and gets an EAGAIN in the block_handler(): https://cgit.freedesktop.org/xorg/xserver/tree/hw/xwayland/xwayland.c?h=server-1.18-branch#n483 I tried to poll() the Wayland fd with a timeout prior to wl_display_flush() to make sure to wl_display_flush() only when writable, to see if that would help unblocking mutter waiting for its PropertyNotify event but that did not work, the Wayland fd still remains in EAGAIN forever and gnome-shell/mutter remains stuck waiting for the PropertyNotify event... I am a bit puzzled, why is gnome-shell/mutter/xcb waiting for the PropertyNotify, where is that event gone? Any ideas? Thanks Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver v2] xwayland: handle EAGAIN and EINTR gracefully
wl_display_flush() can fail with EAGAIN and Xwayland would make this a
fatal error.
Handle the usual EAGAIN and EINTR gracefully so that Xwayland doesn't
die for so little.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1278159
Signed-off-by: Olivier Fourdan
---
v2: oops, need to poll() on EAGAIN between retries
hw/xwayland/xwayland.c | 25 -
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 847321e..2c7f45b 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -33,6 +33,7 @@
#include
#include
#include
+#include
#ifdef XF86VIDMODE
#include
@@ -481,6 +482,23 @@ xwl_read_events (struct xwl_screen *xwl_screen)
FatalError("failed to dispatch Wayland events: %s\n", strerror(errno));
}
+static Bool
+xwl_poll_display_fd (struct xwl_screen *xwl_screen)
+{
+struct pollfd poll_fd;
+int ret;
+
+poll_fd.fd = wl_display_get_fd(xwl_screen->display);
+poll_fd.events = POLLOUT;
+do {
+ret = xserver_poll(&poll_fd, 1, -1);
+if (ret > 0)
+return TRUE;
+} while (ret == -1 && (errno == EINTR || errno == EAGAIN));
+
+return FALSE;
+ }
+
static void
xwl_dispatch_events (struct xwl_screen *xwl_screen)
{
@@ -496,7 +514,12 @@ xwl_dispatch_events (struct xwl_screen *xwl_screen)
xwl_screen->prepare_read = 1;
-ret = wl_display_flush(xwl_screen->display);
+do {
+ret = wl_display_flush(xwl_screen->display);
+if (ret == -1 && errno == EAGAIN && xwl_poll_display_fd(xwl_screen))
+continue;
+} while (ret == -1 && errno == EINTR);
+
if (ret == -1)
FatalError("failed to write to XWayland fd: %s\n", strerror(errno));
}
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: handle EAGAIN and EINTR gracefully
wl_display_flush() can fail with EAGAIN and Xwayland would make this a
fatal error.
Handle the usual EAGAIN and EINTR gracefully so that Xwayland doesn't
die for so little.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1278159
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 847321e..0c67cc8 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -496,7 +496,10 @@ xwl_dispatch_events (struct xwl_screen *xwl_screen)
xwl_screen->prepare_read = 1;
-ret = wl_display_flush(xwl_screen->display);
+do {
+ret = wl_display_flush(xwl_screen->display);
+} while (ret == -1 && (errno == EINTR || errno == EAGAIN));
+
if (ret == -1)
FatalError("failed to write to XWayland fd: %s\n", strerror(errno));
}
--
2.9.3
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] xwayland: Close the shm fd as early as possible
> > > [...] > > > > > > the idea here looks fine to me. > > > > > > However, I've been wondering, surely there was a reason why it wasn't > > > coded like this in the first place? > > > > > > Could you check if this patch causes Xwayland to create lots of > > > wl_buffers it will never attach and commit to a wl_surface? If it does, > > > that was probably the reason. > > > > Was that evaluated? I guess the risk here is that we might end up creating useless wl_buffers. Still, the benefit of closing the fd as soon as possible is greater than the risk of creating too many unused wl_buffer, so I think it's worth it. (Not convinced? try running 10 instances of "gtk3-demo --run=cursors" in Wayland) > > AFAICS, when using glamor, the only code path that still uses the > > xwl_shm_*_pixmap() is the cursor code, so even with glamor, running several > > X11 apps which create several cursors each will quickly cause Xwayland to > > reach the limit of file descriptors. > > > > > If that is true, then one has to weigh between creating wl_buffers > > > immediately vs. creating them only when actually needed for a > > > wl_surface. > > > > > > Maybe Daniel knows? CC'ing also Olivier for his information. > > > > I cannot tell why this was done the way it is, but I think this patch does > > improve things for e.g. downstream bug > > https://bugzilla.redhat.com/show_bug.cgi?id=1373451 That particular bug is possibly a leak of cursors in Qt, but still, there is a weakness in Xwayland. so, weighing the pros and cons of this patch, FWIW: Reviewed-by: Olivier Fourdan Unless someone else is opposed to this patch, I'm in... Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] xwayland: Close the shm fd as early as possible
map->data, xwl_pixmap->size);
> > > -close(xwl_pixmap->fd);
> > > free(xwl_pixmap);
> > > }
> > >
> > > @@ -243,26 +254,7 @@ xwl_shm_destroy_pixmap(PixmapPtr pixmap)
> > > struct wl_buffer *
> > > xwl_shm_pixmap_get_wl_buffer(PixmapPtr pixmap)
> > > {
> > > -struct xwl_screen *xwl_screen =
> > > xwl_screen_get(pixmap->drawable.pScreen);
> > > -struct xwl_pixmap *xwl_pixmap = xwl_pixmap_get(pixmap);
> > > -struct wl_shm_pool *pool;
> > > -uint32_t format;
> > > -
> > > -if (xwl_pixmap->buffer)
> > > -return xwl_pixmap->buffer;
> > > -
> > > -pool = wl_shm_create_pool(xwl_screen->shm,
> > > - xwl_pixmap->fd, xwl_pixmap->size);
> > > -
> > > -format = shm_format_for_depth(pixmap->drawable.depth);
> > > -xwl_pixmap->buffer = wl_shm_pool_create_buffer(pool, 0,
> > > -
> > > pixmap->drawable.width,
> > > -
> > > pixmap->drawable.height,
> > > - pixmap->devKind,
> > > format);
> > > -
> > > - wl_shm_pool_destroy(pool);
> > > -
> > > -return xwl_pixmap->buffer;
> > > +return xwl_pixmap_get(pixmap)->buffer;
> > > }
> > >
> > > Bool
> >
> > Hi Rui,
> >
> > the idea here looks fine to me.
> >
> > However, I've been wondering, surely there was a reason why it wasn't
> > coded like this in the first place?
> >
> > Could you check if this patch causes Xwayland to create lots of
> > wl_buffers it will never attach and commit to a wl_surface? If it does,
> > that was probably the reason.
>
> Was that evaluated?
>
> AFAICS, when using glamor, the only code path that still uses the
> xwl_shm_*_pixmap() is the cursor code, so even with glamor, running several
> X11 apps which create several cursors each will quickly cause Xwayland to
> reach the limit of file descriptors.
>
> > If that is true, then one has to weigh between creating wl_buffers
> > immediately vs. creating them only when actually needed for a
> > wl_surface.
> >
> > Maybe Daniel knows? CC'ing also Olivier for his information.
>
> I cannot tell why this was done the way it is, but I think this patch does
> improve things for e.g. downstream bug
> https://bugzilla.redhat.com/show_bug.cgi?id=1338979
Blimey! copy/paste failed, that should have read
https://bugzilla.redhat.com/show_bug.cgi?id=1373451
> so:
>
> Tested-by: Olivier Fourdan
>
> Cheers,
> Olivier
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] xwayland: Close the shm fd as early as possible
> - > > -if (xwl_pixmap->buffer) > > -return xwl_pixmap->buffer; > > - > > -pool = wl_shm_create_pool(xwl_screen->shm, > > - xwl_pixmap->fd, xwl_pixmap->size); > > - > > -format = shm_format_for_depth(pixmap->drawable.depth); > > -xwl_pixmap->buffer = wl_shm_pool_create_buffer(pool, 0, > > - pixmap->drawable.width, > > - > > pixmap->drawable.height, > > - pixmap->devKind, > > format); > > - > > -wl_shm_pool_destroy(pool); > > - > > -return xwl_pixmap->buffer; > > +return xwl_pixmap_get(pixmap)->buffer; > > } > > > > Bool > > Hi Rui, > > the idea here looks fine to me. > > However, I've been wondering, surely there was a reason why it wasn't > coded like this in the first place? > > Could you check if this patch causes Xwayland to create lots of > wl_buffers it will never attach and commit to a wl_surface? If it does, > that was probably the reason. Was that evaluated? AFAICS, when using glamor, the only code path that still uses the xwl_shm_*_pixmap() is the cursor code, so even with glamor, running several X11 apps which create several cursors each will quickly cause Xwayland to reach the limit of file descriptors. > If that is true, then one has to weigh between creating wl_buffers > immediately vs. creating them only when actually needed for a > wl_surface. > > Maybe Daniel knows? CC'ing also Olivier for his information. I cannot tell why this was done the way it is, but I think this patch does improve things for e.g. downstream bug https://bugzilla.redhat.com/show_bug.cgi?id=1338979 so: Tested-by: Olivier Fourdan Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Missing Xwayland patch for X server 1.19
Hi Peter, Adam, Do you think we could land Rui's patch for Xwayland for 1.19?: https://patchwork.freedesktop.org/patch/95244/ It's been reviewed by Daniel and tested by myself (and others) for some time. If needed, I can prepare a pull request if that's more convenient for you. Thanks Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Backporting Xwayland fixes to 1.18.x
Hi, I have a few fixes in server-1.18-branch cherry-picked from master for Xwayland here: git://people.freedesktop.org/~ofourdan :server-1.18-backports (https://cgit.freedesktop.org/~ofourdan/xserver/log/?h=server-1.18-backports) These contains several fixes from master that can be useful for Xwayland (monotonic clock -including the fix for the original patch-, spurious key press events on focus changes, couple of fixes for memory leaks and double free) that can be useful for 1.18 (see bug 97467 for example). This also contains the fixes for the key repeat issue that landed in master some time ago - Maybe that fix (actually 4 patches) could be deemed as too risky for 1.18?... I don't know. So, do we plan release for the stable branch 1.18? If so, what's your take, should I send a pull request from this branch or are there patches that I should remove before as too risky for the stable branch? Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] wayland: Emulate crossing for native window
Hi all,
Gentle reminder about this patch...
Cheers,
Olivier
- Original Message -
> Emitting a LeaveNotify event every time the pointer leaves an X11 window
> may confuse focus follow mouse mode in window managers such as
> mutter/gnome-shell.
>
> Keep the previously found X window and compare against the new one, and
> if they match then it means the pointer has left an Xwayland window for
> a native Wayland surface, only in this case fake the crossing to the
> root window.
>
> Signed-off-by: Olivier Fourdan
> ---
> hw/xwayland/xwayland-input.c | 15 ++-
> hw/xwayland/xwayland.c | 3 ++-
> hw/xwayland/xwayland.h | 1 +
> 3 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index e295c71..043379e 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -959,7 +959,7 @@ xwl_xy_to_window(ScreenPtr screen, SpritePtr sprite, int
> x, int y)
> }
> }
>
> -if (xwl_seat == NULL || !xwl_seat->focus_window) {
> +if (xwl_seat == NULL) {
> sprite->spriteTraceGood = 1;
> return sprite->spriteTrace[0];
> }
> @@ -969,6 +969,19 @@ xwl_xy_to_window(ScreenPtr screen, SpritePtr sprite, int
> x, int y)
> xwl_seat->xwl_screen->XYToWindow = screen->XYToWindow;
> screen->XYToWindow = xwl_xy_to_window;
>
> +/* If the pointer has left the Wayland surface but the DIX still
> + * finds the pointer within the previous X11 window, it means that
> + * the pointer has crossed to another native Wayland window, in this
> + * case, pretend we entered the root window so that a LeaveNotify
> + * event is emitted.
> + */
> +if (xwl_seat->focus_window == NULL && xwl_seat->last_xwindow == ret) {
> +sprite->spriteTraceGood = 1;
> +return sprite->spriteTrace[0];
> +}
> +
> +xwl_seat->last_xwindow = ret;
> +
> return ret;
> }
>
> diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
> index 8c49b0b..1b29491 100644
> --- a/hw/xwayland/xwayland.c
> +++ b/hw/xwayland/xwayland.c
> @@ -323,7 +323,8 @@ xwl_unrealize_window(WindowPtr window)
> xorg_list_for_each_entry(xwl_seat, &xwl_screen->seat_list, link) {
> if (xwl_seat->focus_window && xwl_seat->focus_window->window ==
> window)
> xwl_seat->focus_window = NULL;
> -
> +if (xwl_seat->last_xwindow == window)
> +xwl_seat->last_xwindow = NullWindow;
> xwl_seat_clear_touch(xwl_seat, window);
> }
>
> diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
> index b8a58e7..aa78b44 100644
> --- a/hw/xwayland/xwayland.h
> +++ b/hw/xwayland/xwayland.h
> @@ -132,6 +132,7 @@ struct xwl_seat {
> struct wl_surface *cursor;
> struct wl_callback *cursor_frame_cb;
> Bool cursor_needs_update;
> +WindowPtr last_xwindow;
>
> struct xorg_list touches;
>
> --
> 2.7.4
>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: Avoid double free of RRCrtc and RROutput
At shutdown, the Xserver will free all its resources which includes the
RRCrtc and RROutput created.
Xwayland would do the same in its xwl_output_destroy() called from
xwl_close_screen(), leading to a double free of existing RRCrtc
RROutput:
Invalid read of size 4
at 0x4CDA10: RRCrtcDestroy (rrcrtc.c:689)
by 0x426E75: xwl_output_destroy (xwayland-output.c:301)
by 0x424144: xwl_close_screen (xwayland.c:117)
by 0x460E17: CursorCloseScreen (cursor.c:187)
by 0x4EB5A3: AnimCurCloseScreen (animcur.c:106)
by 0x4EF431: present_close_screen (present_screen.c:64)
by 0x556D40: dix_main (main.c:354)
by 0x6F0D290: (below main) (in /usr/lib/libc-2.24.so)
Address 0xbb1fc30 is 0 bytes inside a block of size 728 free'd
at 0x4C2BDB0: free (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x4CCE5F: RRCrtcDestroyResource (rrcrtc.c:719)
by 0x577541: doFreeResource (resource.c:895)
by 0x5787B5: FreeClientResources (resource.c:1161)
by 0x578862: FreeAllResources (resource.c:1176)
by 0x556C54: dix_main (main.c:323)
by 0x6F0D290: (below main) (in /usr/lib/libc-2.24.so)
Block was alloc'd at
at 0x4C2CA6A: calloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x4CC6DB: RRCrtcCreate (rrcrtc.c:76)
by 0x426D1C: xwl_output_create (xwayland-output.c:264)
by 0x4232EC: registry_global (xwayland.c:431)
by 0x76CB1C7: ffi_call_unix64 (in /usr/lib/libffi.so.6.0.4)
by 0x76CAC29: ffi_call (in /usr/lib/libffi.so.6.0.4)
by 0x556CEFD: wl_closure_invoke (connection.c:935)
by 0x5569CBF: dispatch_event.isra.4 (wayland-client.c:1310)
by 0x556AF13: dispatch_queue (wayland-client.c:1456)
by 0x556AF13: wl_display_dispatch_queue_pending
(wayland-client.c:1698)
by 0x556B33A: wl_display_roundtrip_queue (wayland-client.c:1121)
by 0x42371C: xwl_screen_init (xwayland.c:631)
by 0x552F60: AddScreen (dispatch.c:3864)
And:
Invalid read of size 4
at 0x522890: RROutputDestroy (rroutput.c:348)
by 0x42684E: xwl_output_destroy (xwayland-output.c:302)
by 0x423CF4: xwl_close_screen (xwayland.c:118)
by 0x4B6377: CursorCloseScreen (cursor.c:187)
by 0x539503: AnimCurCloseScreen (animcur.c:106)
by 0x53D081: present_close_screen (present_screen.c:64)
by 0x43DBF0: dix_main (main.c:354)
by 0x7068730: (below main) (libc-start.c:289)
Address 0xc403190 is 0 bytes inside a block of size 154 free'd
at 0x4C2CD5A: free (vg_replace_malloc.c:530)
by 0x521DF3: RROutputDestroyResource (rroutput.c:389)
by 0x45DA61: doFreeResource (resource.c:895)
by 0x45ECFD: FreeClientResources (resource.c:1161)
by 0x45EDC2: FreeAllResources (resource.c:1176)
by 0x43DB04: dix_main (main.c:323)
by 0x7068730: (below main) (libc-start.c:289)
Block was alloc'd at
at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
by 0x52206B: RROutputCreate (rroutput.c:84)
by 0x426763: xwl_output_create (xwayland-output.c:270)
by 0x422EDC: registry_global (xwayland.c:432)
by 0x740FC57: ffi_call_unix64 (unix64.S:76)
by 0x740F6B9: ffi_call (ffi64.c:525)
by 0x5495A9D: wl_closure_invoke (connection.c:949)
by 0x549283F: dispatch_event.isra.4 (wayland-client.c:1274)
by 0x5493A13: dispatch_queue (wayland-client.c:1420)
by 0x5493A13: wl_display_dispatch_queue_pending
(wayland-client.c:1662)
by 0x5493D2E: wl_display_roundtrip_queue (wayland-client.c:1085)
by 0x4232EC: xwl_screen_init (xwayland.c:632)
by 0x439F50: AddScreen (dispatch.c:3864)
Split xwl_output_destroy() into xwl_output_destroy() which frees the
wl_output and the xwl_output structure, and xwl_output_remove() which
does the RRCrtcDestroy() and RROutputDestroy() and call the latter only
when an output is effectively removed.
An additional benefit, on top of avoiding a double free, is to avoid
updating the screen size at shutdown.
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-output.c | 12 +---
hw/xwayland/xwayland.c| 2 +-
hw/xwayland/xwayland.h| 2 ++
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
index b66da13..38c92a6 100644
--- a/hw/xwayland/xwayland-output.c
+++ b/hw/xwayland/xwayland-output.c
@@ -292,20 +292,26 @@ err:
void
xwl_output_destroy(struct xwl_output *xwl_output)
{
+wl_output_destroy(xwl_output->output);
+free(xwl_output);
+}
+
+void
+xwl_output_remove(struct xwl_output *xwl_output)
+{
struct xwl_output *it;
struct xwl_screen *xwl_screen = xwl_output->xwl_screen;
int width = 0, height = 0;
-wl_output_destroy(xwl_output->output);
-xorg_list_del(&xwl_output->link);
RRCrtcDestroy(xwl_output->randr_crtc);
RROutputDestroy(xwl_output->randr_output);
+xorg_list_del(&xwl_output->link);
xorg_list_for_each_entry(it, &xwl_screen->output_list, link)
output_get_new_size(it, &am
[PATCH xserver] present: Free the fake_present OsTimerPtr
Plug a leak in present_fake_queue_vblank() where the OsTimer would not
be freed.
492,608 (482,816 direct, 9,792 indirect) bytes in 15,088 blocks
are definitely lost in loss record 3,954 of 3,954
at 0x4C2ABDE: malloc (in vgpreload_memcheck-amd64-linux.so)
by 0x586B19: TimerSet (WaitFor.c:433)
by 0x4F1AA9: present_fake_queue_vblank (present_fake.c:108)
by 0x4F15E0: present_pixmap (present.c:954)
by 0x4F23B4: proc_present_pixmap (present_request.c:138)
by 0x552BCE: Dispatch (dispatch.c:430)
by 0x556C22: dix_main (main.c:300)
by 0x6F0D290: (below main) (in /usr/lib/libc-2.24.so)
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97065
Signed-off-by: Olivier Fourdan
---
present/present_fake.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/present/present_fake.c b/present/present_fake.c
index 4985c81..2350638 100644
--- a/present/present_fake.c
+++ b/present/present_fake.c
@@ -64,6 +64,7 @@ present_fake_do_timer(OsTimerPtr timer,
present_fake_notify(fake_vblank->screen, fake_vblank->event_id);
xorg_list_del(&fake_vblank->list);
+TimerFree(fake_vblank->timer);
free(fake_vblank);
return 0;
}
@@ -75,7 +76,7 @@ present_fake_abort_vblank(ScreenPtr screen, uint64_t
event_id, uint64_t msc)
xorg_list_for_each_entry_safe(fake_vblank, tmp, &fake_vblank_queue, list) {
if (fake_vblank->event_id == event_id) {
-TimerCancel(fake_vblank->timer);
+TimerFree(fake_vblank->timer); /* TimerFree will call
TimerCancel() */
xorg_list_del(&fake_vblank->list);
free (fake_vblank);
break;
--
2.7.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: Plug memleak in frame callbacks
The frame callback set up via wl_surface_frame() needs to be freed with
wl_callback_destroy() or we'll leak memory.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97065
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-cursor.c | 2 ++
hw/xwayland/xwayland.c| 2 ++
2 files changed, 4 insertions(+)
diff --git a/hw/xwayland/xwayland-cursor.c b/hw/xwayland/xwayland-cursor.c
index 74dfe4e..7d14a3d 100644
--- a/hw/xwayland/xwayland-cursor.c
+++ b/hw/xwayland/xwayland-cursor.c
@@ -100,6 +100,8 @@ frame_callback(void *data,
uint32_t time)
{
struct xwl_seat *xwl_seat = data;
+
+wl_callback_destroy (xwl_seat->cursor_frame_cb);
xwl_seat->cursor_frame_cb = NULL;
if (xwl_seat->cursor_needs_update) {
xwl_seat->cursor_needs_update = FALSE;
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 2d44d07..e9892f7 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -359,6 +359,8 @@ frame_callback(void *data,
uint32_t time)
{
struct xwl_window *xwl_window = data;
+
+wl_callback_destroy (xwl_window->frame_callback);
xwl_window->frame_callback = NULL;
}
--
2.7.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: Override redirect windows with keyboard grabs on Xwayland
Hi Adam, > > That mechanism would probably not work as is with O-R windows for a > > couple of reasons: > > Your descriptions here seem to assume the inability to fix the > compositor, which to me seems... insane? No, I did not assume this, but I find focus management in X11 to be quite complicated in X11 window management alone, even more when it's coupled with a Wayland compositor managing both Wayland native surfaces and X11 windows, including O-R, so I try to remain as little intrusive as I can :) > _Nothing_ xserver can do in this situation is going to make this be > handled reasonably all on its own, we have to assume a cooperative > compositor. So... Yes, I guess there are cases where I can't avoid being a tad more intrusive... > [...] > Again: that it is painted and responds to input is not mandatory. The > conditions described here (o-r, the size of an output, etc) are things > the compositor is aware of before it decides to map the wayland > surface. X can give it more information (GrabNotify or whatever), but X > can only request politely that wl behave nicely. > > > We do (well, in gtk-shell no not strictly standard) have a "present" > > protocol that allows a Wayland client to ask the compositor to raise > > and focus a surface, we could use this with Xwayland to achieve that, > > but I suspect mutter would most likely deny such request on O-R (and > > being gtk-shell, that wouldn't work with any other compositor). > > Sure, I wouldn't want to depend on gtk_shell either. Perhaps a better > idea is to write our own x11_compatibility protocol and use it if it's > there. The compositor would of course be free to honor that protocol > only for Xwayland servers it happens to be managing, or to not > implement it at all if it doesn't care about mixed x11 and native > wayland surfaces. Yes, definitely a good idea imho. > > Thing is, weston seems to do this right so there should be a way to > > achieve that in mutter as well. > > > > The approach I took in my patch for GNOME bug https://bugzilla.gnome. > > org/show_bug.cgi?id=752956 is to not deny focus for O-R that are > > fullscreen (either on a single monitor or the whole screen): > > > > https://bugzilla.gnome.org/attachment.cgi?id=330053&action=diff > > > > It does fix the issue, but I am not sure this can be acceptable in > > GNOME. > > If gnome considers purity of essence to be more important than correct > functionality, well, that's an opinion they can have I guess. Sorry if my previous email made it sound like that, it's certainly not what I meant. > to see X extended to give the compositor more information and better > control. But I don't see a reasonable way of fixing this entirely > within xserver. The compositor has to want to get this right. Yes, agreed, I think adding an X11 compat protocol is the way to go. I shall start gathering ideas and inputs when I'll get back from PTO. Thanks Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: Override redirect windows with keyboard grabs on Xwayland
Hey Adam, > This feels a lot like any other "app wants attention" case where you > should just get a pulsing button or bouncing icon in the taskbar. The > o-r window might be mapped and focused from Xwayland's perspective but > there's nothing compelling wayland to actually show or focus it > promptly, you know it's o-r, you know it's full-screen sized, and you > have control of the display so when you _do_ put it up you can vignette > it and add a Get Me Out Of Here checkbox in the upper-right or > whatever. That mechanism would probably not work as is with O-R windows for a couple of reasons: - the WM is not supposed to manage O-R windows (well, a compositor has of course, but the WM doesn't get a MapRequest and most WM will do as little as possible with O-R). - O-R are not listed in the window list so even if the compositor would have set the demand attention flag, the shell would probably ignore it. Besides, what happens here is that O-R is already covering the entire screen (thus most likely cover any shell notification as well), and it feels natural for the user to start typing as the screen is covered and a nice text input is displayed :) > I guess the thing you don't really get is a GrabNotify to let the wm > know what's going on, though maybe you could infer it from FocusOut. > There also appears to be no "wants attention" request in any wayland > protocol, which is perhaps suboptimal. But if we had that, xserver > could just emit that when an active grab fires, and then once the > window gets focus the grab works just as well as it ever would. We do (well, in gtk-shell no not strictly standard) have a "present" protocol that allows a Wayland client to ask the compositor to raise and focus a surface, we could use this with Xwayland to achieve that, but I suspect mutter would most likely deny such request on O-R (and being gtk-shell, that wouldn't work with any other compositor). Thing is, weston seems to do this right so there should be a way to achieve that in mutter as well. The approach I took in my patch for GNOME bug https://bugzilla.gnome.org/show_bug.cgi?id=752956 is to not deny focus for O-R that are fullscreen (either on a single monitor or the whole screen): https://bugzilla.gnome.org/attachment.cgi?id=330053&action=diff It does fix the issue, but I am not sure this can be acceptable in GNOME. Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] xwayland: Process queued events before making wayland mods effective
Hi
- Original Message -
> Since xwayland's initial commit we have had a check to not process
> wayland modifier events while one of our surfaces has keyboard focus
> since the normal xkb event processing keeps our internal modifier
> state up to date and if we use the modifiers we get from the
> compositor we mess up that state.
>
> This was slightly changed in commit
> 10e9116b3f709bec6d6a50446c1341441a0564e4 to allow the xkb group to be
> set from the wayland event while we have focus in case the compositor
> triggers a group switch.
>
> There's a better solution to the original problem though. Processing
> queued events before overriding the xkb state with the compositor's
> allows those events to be sent properly modified to X clients while
> any further events will be modified with the wayland modifiers as
> intended.
>
> This allows us to fully take in the wayland modifiers, including
> depressed ones, which fixes an issue where we wouldn't be aware of
> already pressed modifiers on enter.
>
> Signed-off-by: Rui Matos
> ---
>
> Fixes https://bugzilla.gnome.org/show_bug.cgi?id=767475 . I'm not 100%
> sure there aren't unintended side-effects but it seems to be working
> fine in actual use for a couple of weeks.
>
> hw/xwayland/xwayland-input.c | 11 ++-
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index 4642efe..7aae7ba 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -503,6 +503,8 @@ keyboard_handle_modifiers(void *data, struct wl_keyboard
> *keyboard,
> xkbStateNotify sn;
> CARD16 changed;
>
> +mieqProcessInputEvents();
> +
> for (dev = inputInfo.devices; dev; dev = dev->next) {
> if (dev != xwl_seat->keyboard &&
> dev != GetMaster(xwl_seat->keyboard, MASTER_KEYBOARD))
> @@ -511,12 +513,11 @@ keyboard_handle_modifiers(void *data, struct
> wl_keyboard *keyboard,
> old_state = dev->key->xkbInfo->state;
> new_state = &dev->key->xkbInfo->state;
>
> -if (!xwl_seat->keyboard_focus) {
> -new_state->locked_mods = mods_locked & XkbAllModifiersMask;
> -XkbLatchModifiers(dev, XkbAllModifiersMask,
> - mods_latched & XkbAllModifiersMask);
> -}
> new_state->locked_group = group & XkbAllGroupsMask;
> +new_state->base_mods = mods_depressed & XkbAllModifiersMask;
> +new_state->locked_mods = mods_locked & XkbAllModifiersMask;
> +XkbLatchModifiers(dev, XkbAllModifiersMask,
> + mods_latched & XkbAllModifiersMask);
>
> XkbComputeDerivedState(dev->key->xkbInfo);
>
FWIW I have been using this patch for a few weeks and it works fine as far as I
can tell, and it doesn't seem to have any ill effect on Weston either, so:
Tested-by: Olivier Fourdan
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
