[PATCH xserver 0/2] RFC: implement and use `present_event_abandon()`

[Olivier Fourdan]

Hi,

I reckon https://bugs.freedesktop.org/show_bug.cgi?id=108249 ("[xwayland]
Crash in Xpresent code on resume from suspend") is caused by the present
code using a RRCrtcPtr previously freed by Xwayland.

Reason for this is because Xwayland's `xwl_output_remove()` will destroy
the RRCrtcPtr for the Wayland outputs when removed, but if there is a
flip pending, the `xwl_present_sync_callback()` will trigger after the
CRTC is destroyed and not much good will come out of this.

So I was tempted to use `present_event_abandon(RRCrtcPtr crtc)` which looked
like a good candidate, until I realized that there was no actual implementation
for that function declaration in present.h.

So, the two following patches are my attempt at implementing such a
`present_event_abandon()` function and make use of it in Xwayland.

There are marked as "RFC" because the Present code is still pretty much
alien territory for me...

Cheers,
Olivier

Olivier Fourdan (2):
present: implement `present_event_abandon()`
xwayland: abandon present events when removing CRTC

hw/xwayland/xwayland-output.c | 3 ++-
present/present_screen.c | 32 ++++++++++++++++++++++++++++++++
2 files changed, 34 insertions(+), 1 deletion(-)

[Olivier Fourdan]

The function `present_event_abandon()` is marked as exported in the
present.h header but it's nowhere to be found.

Such a function might prove handy for the DDX such as Xwayland who
creates and destroys CRTCs as new outputs are added or removed.

`present_event_abandon()` checks all vblank for all windows for the
given CRTC and abort those who match, so that we don't leave events
behind referring to a `RRCtrcPtr` pointing to freed memory.

Signed-off-by: Olivier Fourdan <***@redhat.com>
Bugzilla: https://bugs.freedesktop.org/108249
---
present/present_screen.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)

diff --git a/present/present_screen.c b/present/present_screen.c
index c7e37c5fd..35a106ada 100644
--- a/present/present_screen.c
+++ b/present/present_screen.c
@@ -167,6 +167,38 @@ present_destroy_window(WindowPtr window)
return ret;
}

+static int
+check_vblank_crtc(WindowPtr window, void *value)
+{
+ ScreenPtr screen = window->drawable.pScreen;
+ present_window_priv_ptr window_priv = present_window_priv(window);
+ present_screen_priv_ptr screen_priv = present_screen_priv(screen);
+ present_vblank_ptr vblank, tmp;
+ RRCrtcPtr target_crtc = value;
+
+ if (!window_priv)
+ return WT_WALKCHILDREN;
+
+ xorg_list_for_each_entry_safe(vblank, tmp, &window_priv->vblank, window_list) {
+ if (target_crtc == vblank->crtc) {
+ screen_priv->abort_vblank(window->drawable.pScreen,
+ window, vblank->crtc,
+ vblank->event_id, vblank->target_msc);
+ present_vblank_destroy(vblank);
+ }
+ }
+
+ return WT_WALKCHILDREN;
+}
+
+void
+present_event_abandon(RRCrtcPtr crtc)
+{
+ ScreenPtr pScreen = crtc->pScreen;
+
+ WalkTree(pScreen, check_vblank_crtc, (void *) crtc);
+}
+
/*
* Hook the config notify screen function to deliver present config notify events
*/

[Olivier Fourdan]

Xwayland will add and remove CRTCs as Wayland outputs are added or
removed.

If there is a pending flip when this occurs, the
`xwl_present_sync_callback()` will be triggered after the Xwayland
output's RRCtrcPtr has been destroyed, hence causing a crash in Xwayland
while trying to use freed memory:

#1 abort ()
#2 OsAbort () at utils.c:1350
#3 AbortServer () at log.c:877
#4 FatalError () at log.c:1015
#5 OsSigHandler () at osinit.c:156
#6 <signal handler called>
#7 dixGetPrivate () at ../include/privates.h:122
#8 dixLookupPrivate () at ../include/privates.h:166
#9 present_screen_priv () at present_priv.h:198
#10 present_wnmd_flip () at present_wnmd.c:358
#11 present_wnmd_execute () at present_wnmd.c:466
#12 present_wnmd_re_execute () at present_wnmd.c:80
#13 xwl_present_sync_callback () at xwayland-present.c:287
#14 ffi_call_unix64 () from /lib64/libffi.so.6
#15 ffi_call () from /lib64/libffi.so.6
#16 wl_closure_invoke () at src/connection.c:1006
#17 dispatch_event () at src/wayland-client.c:1427
#18 dispatch_queue () at src/wayland-client.c:1573
#19 wl_display_dispatch_queue_pending () at src/wayland-client.c:1815
#20 wl_display_dispatch_pending () at src/wayland-client.c:1878
#21 xwl_read_events () at xwayland.c:814
#22 ospoll_wait () at ospoll.c:651
#23 WaitForSomething () at WaitFor.c:208
#24 Dispatch () at ../include/list.h:220
#25 dix_main () at main.c:276

To avoid the issue, make sure we call `present_event_abandon()` on the
CRTC we're about to destroy in `xwl_output_remove()`.

Signed-off-by: Olivier Fourdan <***@redhat.com>
Bugzilla: https://bugs.freedesktop.org/108249
---
hw/xwayland/xwayland-output.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
index cc68f0340..50f27c624 100644
--- a/hw/xwayland/xwayland-output.c
+++ b/hw/xwayland/xwayland-output.c
@@ -29,6 +29,7 @@

#include "xwayland.h"
#include <randrstr.h>
+#include <present.h>

#define DEFAULT_DPI 96
#define ALL_ROTATIONS (RR_Rotate_0 | \
@@ -409,7 +410,7 @@ xwl_output_remove(struct xwl_output *xwl_output)
xorg_list_for_each_entry(it, &xwl_screen->output_list, link)
output_get_new_size(it, need_rotate, &height, &width);
update_screen_size(xwl_output, width, height);
-
+ present_event_abandon (xwl_output->randr_crtc);
RRCrtcDestroy(xwl_output->randr_crtc);
RROutputDestroy(xwl_output->randr_output);

[Olivier Fourdan]

Hi,

Post by Olivier Fourdan
I reckon https://bugs.freedesktop.org/show_bug.cgi?id=108249 ("[xwayland]
Crash in Xpresent code on resume from suspend") is caused by the present
code using a RRCrtcPtr previously freed by Xwayland.
Reason for this is because Xwayland's `xwl_output_remove()` will destroy
the RRCrtcPtr for the Wayland outputs when removed, but if there is a
flip pending, the `xwl_present_sync_callback()` will trigger after the
CRTC is destroyed and not much good will come out of this.
So I was tempted to use `present_event_abandon(RRCrtcPtr crtc)` which looked
like a good candidate, until I realized that there was no actual implementation
for that function declaration in present.h.
So, the two following patches are my attempt at implementing such a
`present_event_abandon()` function and make use of it in Xwayland.
There are marked as "RFC" because the Present code is still pretty much
alien territory for me...

If that makes it easier for reviews/comments, I posted the same as a WIP/MR:

https://gitlab.freedesktop.org/xorg/xserver/merge_requests/45

Cheers,
Olivier
_______________________________________________
xorg-***@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg