This will allow eliminating indirections and making the Xwayland Present
code more efficient and easier to follow.
While this technically changes the Xorg video driver ABI, I don't know
of any drivers using the dropped present_wnmd_* symbols, and I doubt a
Xorg driver could make use of them as is anyway.
(As a bonus, Xorg no longer links any Xwayland specific Present code)
v2:
* Wrap DestroyWindow before initializing Present, so that
present_destroy_window runs before xwl_present_cleanup. Avoids crash
due to present_destroy_window calling xwl_present_* functions when
xwl_present_window was already freed. (Olivier Fourdan)
Acked-by: Olivier Fourdan <ofourdan@redhat.com>
Allow X11 clients to create shared pixmaps via the MIT-SHM
extension under Xwayland. Tested with a wlroots patch [1].
Also add a few assertions to make sure we have wl_buffers where we
need them.
[1]: https://github.com/swaywm/wlroots/pull/2875
Signed-off-by: Simon Ser <contact@emersion.fr>
Acked-by: Michel Dänzer <mdaenzer@redhat.com>
If the buffer is NULL, do not even try to attach it, and risk a Wayland
protocol error which would be fatal to us.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Martin Peres <martin.peres@mupuf.org>
Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>
https://gitlab.freedesktop.org/xorg/xserver/-/issues/1156
Provides an implementation for the pixmap_from_buffers DRI3 function for
xwayland's eglstream backend. This will be used by the NVIDIA GLX driver
to pass buffers from client applications to the server. These can then
be presented using the PRESENT extension.
To hopefully make this less error-prone, we also introduce a "type"
field for this struct to distinguish between xwl_pixmaps for the new
DRI3-created pixmaps and those for the existing glamor-created pixmaps.
Additionally, the patch enables wnmd present mode with the eglstream backend.
This involves creating a wl_buffer for the provided dma-buf before importing it
into EGL and passing this to the compositor so it can be scanned out directly
if possible.
Since both backends now support this present mode, the HAS_PRESENT_FLIP flag is
no longer needed, so it can be removed.
Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>
Acked-by: Olivier Fourdan <ofourdan@redhat.com>
Signed-off-by: Erik Kurzinger <ekurzinger@nvidia.com>
There are currently no callers that make use of the "created" output parameter
of xwl_glamor_pixmap_get_wl_buffer. Remove it, along with the corresponding
argument of the associated EGL backend entrypoint.
The present flip does not work with the EGLStream backend. Similarly,
the EGLStream backend does not require the buffer to be flushed as
eglSwapBuffers() should take care of this.
Instead of actually checking the backend in use in the present code,
add a flag in the form of a bitfield to the EGL backend to indicate
its features and requirements.
This should not introduce any functional change.
v2: Fix logical test (Adam Jackson <ajax@redhat.com>)
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
We can only flip if the window pixmap matches that of the toplevel
window. Doing so regardless could cause the toplevel window pixmap to
get destroyed while it was still referenced by the window, resulting in
use-after-free and likely a crash.
Closes: https://gitlab.freedesktop.org/xorg/xserver/-/issues/1033
Reviewed-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Roman Gilg <subdiff@gmail.com>
Instead of optionally return early when an event is aborted and potentially
clean it up in there we can only optionally inform Present if not aborted and
afterwards clean it up if required.
Saves some lines of code and conditional branches.
Signed-off-by: Roman Gilg <subdiff@gmail.com>
With the newly introduced separate API method for idling a presented Pixmap in
window mode we can simplify the logic by allowing calls to it at any point in
time.
This is done by setting the flip_idler flag if the Pixmap was idled before
being presented.
Signed-off-by: Roman Gilg <subdiff@gmail.com>
Notifying Present about events' states was done prior with the single function
present_wnmd_event_notify just like in screen mode. But it is more intelligible
if at least in window mode we make use of three different functions with names
that directly indicate what their purpose is:
* present_wnmd_event_notify only for queued events feedback.
* present_wnmd_flip_notify for when a presentation occured (flip).
* present_wnmd_idle_notify for when the Pixmap of the event can be reused.
This is an API-breaking change in regards to window mode. DDX written against
the previous version won't work anymore. It is assumed that there only exists
the XWayland DDX at the moment using the window mode such that this is not an
issue for the overall ecosystem.
Signed-off-by: Roman Gilg <subdiff@gmail.com>
Rename the lists release_queue to release_list and event_list to
wait_list.
The prior names release_queue and event_list were ambiguous: in both are event-
like vblanks which can be removed from the lists in random order. In the
release_queue can be flips that are already released but still wait for the
sync or frame callback but normally the release comes later. In the event_list
are queued events waiting for a later msc.
Signed-off-by: Roman Gilg <subdiff@gmail.com>
In 9141196d positional coordinates were added to the damage call of pixmap
flips. The damage box coordinates are in screen space though and we need
to convert them first to surface-relative ones by substracting the origin
of the window.
Signed-off-by: Roman Gilg <subdiff@gmail.com>
These events aren't reachable after xwl_present_cleanup, so they're
leaked if we don't free them first.
This requires storing the pixmap pointer in struct xwl_present_window.
Luckily, the buffer pointer isn't used for anything, so just replace
that.
v2:
* Bump pixmap reference count in xwl_present_flip and drop it in
xwl_present_free_event, fixes use-after-free in the latter due to the
pixmap already being destroyed.
Reviewed-by: Dave Airlie <airlied@redhat.com>
We were only calling xwl_present_unrealize_window for the toplevel
window, but the list can contain entries from child windows as well,
in which case we were leaving dangling pointers to freed memory.
Closes: https://gitlab.freedesktop.org/xorg/xserver/issues/1000
Fixes: c5067feaee "xwayland: Use single frame callback for Present
flips and normal updates"
Reviewed-by: Olivier Fourdan <ofourdan@redhat.com>
Tested-by: Olivier Fourdan <ofourdan@redhat.com>
Now that each source and header should be in order, we can safely cleaup
the last remaining bits from the main `xwayland.h` which is not needed
anymore and can be removed.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>
Move Xwayland screen related code to a separate source file and header.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>
Move the Xwayland Present declarations to their own header file.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>
Move Xwayland generic pixmap code to a separate source file and header.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>
Over time, Xwayland main source file `xwayland.c` has grown in size
which makes it look cluttered and harder to read.
Move the code dealing with Xwayland window to its own source and header
files.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>
Instead of only the fallback timer.
Fixes https://gitlab.freedesktop.org/xorg/xserver/issues/854
v2:
* Drop unused frame_callback member of struct xwl_present_window
(Olivier Fourdan)
Reviewed-by: Olivier Fourdan <ofourdan@redhat.com>
Using a list of Present windows that need to be called back.
This prepares for the following change, there should be no change in
observed behaviour.
v2:
* Use xwl_window_create_frame_callback instead of making the
frame_listener struct non-static (Olivier Fourdan)
Reviewed-by: Olivier Fourdan <ofourdan@redhat.com>
The Present code sends the idle notification event to the client after
xwl_present_flush returns. If we don't flush our GPU work here, the
client may race to draw another frame to the same buffer, so we may end
up copying (parts of) that new frame instead of the one we meant to.
Fixes https://gitlab.freedesktop.org/xorg/xserver/issues/835
Reviewed-by: Olivier Fourdan <ofourdan@redhat.com>
The API `wl_buffer_add_listener` is misleading in the sense that there
can be only one `wl_buffer` release callback, and trying to add a new
listener when once is already in place will lead to a protocol error.
The Xwayland EGL backends may need to set up their own `wl_buffer`
release listener, meaning that there is no way to our own `wl_buffer`
release callback.
To avoid the problem, add our own callback API to be notified when the
`wl_buffer` associated with an `xwl_pixmap` is released, triggered from
the different `xwl_pixmap` implementations.
Also update the Present code to use the new buffer release callback API.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>
When a viewport is set, damage will only work properly when using
wl_surface_damage_buffer instead of wl_surface_damage.
When no viewport is set, there should be no difference between
surface and buffer damage.
This is a preparation patch for using viewport to add support for fake
mode-changes through xrandr for apps which want to change the resolution
when going fullscreen.
Changes by Hans de Goede <hdegoede@redhat.com>:
-Split the damage changes out into their own patch
-Add xwl_surface_damage helper
-Also use buffer_damage / the new helper for the present and cursor code
Reviewed-by: Olivier Fourdan <ofourdan@redhat.com>
Acked-by: Michel Dänzer <mdaenzer@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
xwl_present_cleanup frees the struct xwl_present_window memory,
so if there's a pending callback, we have to destroy it to prevent
use-after-free in xwl_present_sync_callback.
Should fix issue #645.
Reviewed-by: Olivier Fourdan <ofourdan@redhat.com>
Xwayland creates and destroys the CRTC along with the Wayland outputs,
so there is possibly a case where the number of CRTC drops to 0.
However, `xwl_present_get_crtc()` always return `crtcs[0]` which is
invalid when `numCrtcs` is 0.
That leads to crash if a client queries the Present capabilities when
there is no CRTC, the backtrace looks like:
#0 raise() from libc.so
#1 abort() from libc.so
#2 OsAbort() at utils.c:1350
#3 AbortServer() at log.c:879
#4 FatalError() at log.c:1017
#5 OsSigHandler() at osinit.c:156
#6 OsSigHandler() at osinit.c:110
#7 <signal handler called>
#8 main_arena() from libc.so
#9 proc_present_query_capabilities() at present_request.c:236
#10 Dispatch() at dispatch.c:478
#11 dix_main() at main.c:276
To avoid returning an invalid pointer (`crtcs[0]`) in that case, simply
check for `numCrtcs` being 0 and return `NULL` in that case.
Thanks to Michel Dänzer <michel.daenzer@amd.com> for pointing this as a
possible cause of the crash.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>
Bugzilla: https://bugzilla.redhat.com/1609181
The buffer release queue has two kinds of entries:
* Pending async flips.
* Completed flips waiting for their buffer to be released by the Wayland
compositor.
xwl_present_timer_callback neither completes async flips nor releases
buffers, so the timer isn't needed for the buffer release queue.
Fixes issue #12. Presumably the problem was that Present operations on
unmapped windows were executed immediately instead of only when reaching
the target MSC.
When a window is unrealized, a pending frame callback may never be
called, which could result in repeatedly freezing until the frame timer
fires after a second.
Fixes these symptoms when switching from fullscreen to windowed mode in
sauerbraten.
There's no need to keep track of the window which last performed a
Present flip. This fixes crashes due to the assertion in
xwl_present_flips_stop failing. Fixes issue #10.
The damage generated by a flip only needs to be ignored once, then
xwl_window::present_flipped can be cleared. This may fix freezing in
the (hypothetical) scenario where Present flips are performed on a
window, followed by other drawing requests using the window as the
destination, but nothing triggering xwl_present_flips_stop. The damage
from the latter drawing requests would continue being ignored.
Completing them from xwl_present_sync_callback had at least two issues:
* It was before the MSC was incremented in xwl_present_frame_callback,
so the MSC value in the completion event could be lower than the
target specified by the client. This could cause hangs with the Mesa
Vulkan drivers.
* It allowed clients to run at a frame-rate higher than the Wayland
compositor's frame-rate, wasting energy on generating frames which
were never displayed. This isn't expected to happen unless the client
specified PresentOptionAsync (in which case flips are still completed
from xwl_present_sync_callback, allowing higher frame-rates).
v2:
* Make xwl_present_has_events return true when there's a pending
"synchronous" flip, so those complete after at most ~1 second even if
the Wayland server doesn't send a frame event.
Bugzilla: https://bugs.freedesktop.org/106713
Apart from simplifying the code, this should also prevent a condition
(which might only be possible with the following fix) reported in
https://gitlab.freedesktop.org/wayland/weston/issues/115#note_52467:
1. xwl_present_timer_callback indirectly calls xwl_present_reset_timer
-> xwl_present_free_timer
2. xwl_present_timer_callback then returns a non-0 value, so DoTimer
calls TimerSet with the old xwl_present_window->frame_timer pointer
which was freed in step 1 => use after free
Calling xwl_present_reset_timer explicitly passes NULL to TimerSet if
step 1 freed xwl_present_window->frame_timer, and it will allocate a new
one.
`xwl_present_timer_callback()` is initially marked a private and later
implemented as public.
Let's keep that private, shall we.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Xwayland's `xwl_destroy_window()` invokes `xwl_present_cleanup()`
before the common `DestroyWindow()`.
But then `DestroyWindow()` calls `present_destroy_window()` which will
possibly end up in `xwl_present_abort_vblank()` which will try to access
data that was previously freed by `xwl_present_cleanup()`:
Invalid read of size 8
at 0x434184: xwl_present_abort_vblank (xwayland-present.c:378)
by 0x53785B: present_wnmd_abort_vblank (present_wnmd.c:651)
by 0x53695A: present_free_window_vblank (present_screen.c:87)
by 0x53695A: present_destroy_window (present_screen.c:152)
by 0x42A90D: xwl_destroy_window (xwayland.c:653)
by 0x584298: compDestroyWindow (compwindow.c:613)
by 0x53CEE3: damageDestroyWindow (damage.c:1570)
by 0x4F1BB8: DbeDestroyWindow (dbe.c:1326)
by 0x46F7F6: FreeWindowResources (window.c:1031)
by 0x472847: DeleteWindow (window.c:1099)
by 0x46B54C: doFreeResource (resource.c:880)
by 0x46C706: FreeClientResources (resource.c:1146)
by 0x446ADE: CloseDownClient (dispatch.c:3473)
Address 0x182abde0 is 80 bytes inside a block of size 112 free'd
at 0x4C2FDAC: free (vg_replace_malloc.c:530)
by 0x42A937: xwl_destroy_window (xwayland.c:647)
by 0x584298: compDestroyWindow (compwindow.c:613)
by 0x53CEE3: damageDestroyWindow (damage.c:1570)
by 0x4F1BB8: DbeDestroyWindow (dbe.c:1326)
by 0x46F7F6: FreeWindowResources (window.c:1031)
by 0x472847: DeleteWindow (window.c:1099)
by 0x46B54C: doFreeResource (resource.c:880)
by 0x46C706: FreeClientResources (resource.c:1146)
by 0x446ADE: CloseDownClient (dispatch.c:3473)
by 0x446DA5: ProcKillClient (dispatch.c:3279)
by 0x4476AF: Dispatch (dispatch.c:479)
Block was alloc'd at
at 0x4C30B06: calloc (vg_replace_malloc.c:711)
by 0x433F46: xwl_present_window_get_priv (xwayland-present.c:54)
by 0x434228: xwl_present_get_crtc (xwayland-present.c:302)
by 0x539728: proc_present_query_capabilities (present_request.c:227)
by 0x4476AF: Dispatch (dispatch.c:479)
by 0x44B5B5: dix_main (main.c:276)
by 0x75F611A: (below main) (libc-start.c:308)
This is because `xwl_present_cleanup()` frees the memory but does not
remove it from the window's privates, and `xwl_present_abort_vblank()`
will still find it and hence try to access that freed memory...
Remove `xwl_present_window` from window's privates on cleanup so that no
other function can find and reuse that data once it's freed.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1616269
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
When retrieving the Wayland buffer from a pixmap, if the buffer already
exists, the GBM backend will return that existing buffer.
However, as seen with the Present issues, if the call had previously
passed a wrong size, that buffer will remain at the wrong size for as
long as the buffer exists, which is error prone.
Considering that the width/height passed to get_wl_buffer() is always the
actual pixmap drawable size, and considering that the EGLStream backend
makes no use of the size either, there is really no point in passing the
width/height around.
Simplify the xwl_glamor_pixmap_get_wl_buffer() and EGL backends API by
removing the pixmap size, and use the drawable size instead.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Now that we have separate backends for EGLStream and GBM, we can
explicitly check for the EGLStream backend to disable present support
in that case.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
To be able to check for availability of the Wayland interfaces required
to run a given EGL backend (either GBM or EGLStream for now), we need
to have each backend structures and vfuncs in place before we enter the
Wayland registry dance.
That basically means that we should init all backends at first, connect
to the Wayland compositor and query the available interfaces and then
decide which backend is available and should be used (or none if either
the Wayland interfaces or the EGL extensions are not available).
For this purpose, hold an egl_backend struct for each backend we are to
consider prior to connect to the Wayland display so that, when we get to
query the Wayland interfaces, everything is in place for each backend to
handle the various Wayland interfaces.
Eventually, when we need to chose which EGL backend to use for glamor,
the available Wayland interfaces and EGL extensions available are all
known to Xwayland.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
If the pixmap size does not match the present box size, flickering
occurs.
This can happen when the client changes its size (e.g. switching to
fullscreen), and since the buffer is kept as long as the pixmap is
valid, once the buffer is created, it remains at the wrong (old) size
and causes continuous flickering.
Use the actual pixmap's drawable size instead of the present box to
create the buffer so that it's sized appropriately.
Bugzilla: https://bugs.freedesktop.org/106841
Fixes: 0fb2cca193 "xwayland: Preliminary support for Present's new
window flip mode"
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Roman Gilg <subdiff@gmail.com>
We probably don't want a fake crtc to be visible to clients, and we
definitely don't want to generate events every time we create such a
fake (which would happen as a side effect from RRCrtcCreate hitting
RRTellChanged). As it happens we're not actually using that crtc for
anything because xwayland doesn't store any state on the crtc object,
so it suffices to use the real crtc for the screen.
Signed-off-by: Adam Jackson <ajax@redhat.com>
Tested-by: Roman Gilg <subdiff@gmail.com>
Reviewed-by: Roman Gilg <subdiff@gmail.com>
Michel Dänzer
Simon Ser
Olivier Fourdan
Erik Kurzinger
Roman Gilg
Robert Mader
Michel Dänzer
Adam Jackson