Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update for wlroots 0.18 #1641

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from
Draft

Conversation

Consolatis
Copy link
Member

@Consolatis Consolatis commented Mar 19, 2024

Same story as

This branch is not intended to be merged anytime soon but should allow to collaborate on chasing wlroots master.
As in the previous PR, feel free to push changes into this branch.

I suggest not using force pushes for now to reduce chances of accidentally overwriting something. If some existing commit should be amended just use !fixup or [fixup] or something along that line and refer to the subject of the commit to be amended (the hash may change due to rebases). We can remove that no-force-push restriction once we have a somewhat stable base to work with.

Like in #626 our goal should also be to make this PR as small as possible to reduce rebase conflicts with master. So if we can merge a commit in master that works for 0.17 and 0.18 without introducing any unnecessary complexity we should likely do that.

@ahesford @jlindgren90 @johanmalm

Further information:

Current issues:

  • parentless xdg popups are not be handled correctly
  • the commit messages and wlroots.wrap files do not yet contain the correct wlroots hash and might also be out of order, I only did the first step of getting labwc to work with current wlroots master.
  • https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4600 seems to implement a immediate scale event for surfaces within scene. We may be able to remove our downstream fix (4238d7f) for that issue, @ahesford could say more about that
  • listen to wlr_renderer.events.lost to catch GPU resets and do a Reconfigure to reload all compositor side buffers
  • xdg decorations must not be confirmed directly but only if xdg_surface->initialized == true, also needs storage of the requested deco mode or link from view to the deco object itself which can be used from the xdg commit handler
  • explicit sync (wlroots MR, sway PR)
  • icon spec (wlroots MR)

Fixed:

  • the initial xdg_surface configure flow has been changed, compositors now need to do that on their own
  • at least when running nested the rounded corners sometimes flicker on mouse movements and are then completely transparent (caused by a wlroots bug, MR pending)
  • as wlroots removed wlr_output->pending we have to use the wlr_output_state_() variants everywhere. But to reduce unnecessary screen blanking during mode changes we likely want to keep using a shared pending state. Rather than changing everything around to that I added a new src/output-state.c file which provides wrappers for the old wlr_output_set_()functions. This massively reduces rebase conflicts.
  • we currently crash on ctrl-c in gdb when running nested, needs investigation this is a libwayland bug, fixed via this MR but not yet released
  • lab_wlr_scene_commit() still contains a comment about handling damage, that was an issue in wlroots 0.17.x but might now have been fixed upstream, needs to be verified
  • The CI is currently not set up to deal with wlroots compilations, should be easy to fix
  • WLR_SCENE_DEBUG_DAMAGE=highlight damage tracking is pretty broken, wlroots MR

There might be more issues, only slightly tested.

@jlindgren90
Copy link
Contributor

Thanks for getting this going!

@kode54

This comment was marked as resolved.

@kode54

This comment was marked as resolved.

@Consolatis
Copy link
Member Author

Consolatis commented Apr 23, 2024

The damage issues and wayland native windows not be appearing should hopefully be fixed by the last commits. Also rebased on top of master.

@kode54
Copy link
Contributor

kode54 commented Apr 24, 2024

Further outstanding issues:

  • The display refreshes at less than its native refresh rate. More like 20-30fps instead of 60. This doesn't seem to be related to use of adaptive sync on one of the displays.
  • Atomic modesetting tearing support, which is supposed to be enabled upstream without any changes to the compositor to allow it, other than removing your check for the WLR_DRM_NO_ATOMIC variable, doesn't seem to work. I can try to pull DRM debug logs, but last I checked, it was wlroots committing things it shouldn't be committing during the atomic flips. Hell, AMD doesn't even want it to be committing cursor plane updates to allow tearing.
  • (added by Consolatis: copied over from a previous post) Steam's Xwayland override redirect popup menus seem to display a single frame of solid white before resolving to Steam's intended surface contents. Not sure if this is a bug in Steam, though.

@Consolatis Consolatis mentioned this pull request Apr 24, 2024
3 tasks
@kode54
Copy link
Contributor

kode54 commented Apr 26, 2024

Logs for the above issues, possibly also relevant to wlroots 0.18 instead of just labwc:

dmesg.log.gz
labwc.log.gz

@kode54
Copy link
Contributor

kode54 commented May 1, 2024

Confirmed these frame rate issues do not occur with Sway master, so it's down to labwc and this PR being updated to handle whatever the refresh issue is.

@Consolatis Consolatis force-pushed the chase/0.18 branch 2 times, most recently from b63309f to faf33a7 Compare May 9, 2024 11:34
@Consolatis
Copy link
Member Author

@kode54 added a FPS counter per output via debug log, maybe this helps to shed some light on your half monitor refresh rate observation. I have a slight feeling that this has something to do with the presentation time protocol but I am not sure. At least nested I get the full fps via es2gears_x11 and es2gears_wayland. The FPS counter via debug log shows a reduction of the fps to about screen refresh rate in case of es2gears_x11, for the _wayland case the fps roughly matches between the es2gears output and labwc debug log.

@johanmalm
Copy link
Collaborator

As discussed the other day, I've been running this PR for a couple of days now and it seems fine.

I've also carried out an independent review by studying the PR and the sway commit-history between 2023-11-21 and 2024-05-12 (i.e. since wlroots 0.17.0 release). See findings below.

wlroots chasing - can be done on master now (I think)

wlroots chasing required on 0.18 PR

There are three that I couldn't see covered in the PR.

wlroots chasing not required by labwc

Things in 0.18 PR that may need more work

xdg-shell (toplevels and popups)

Ref: cb8d89e

It looks like there is a lot of xdg/layer-shell toplevel/popup stuff that has changed. Found the following commits that I don't think we've not fully covered in PR 0.18. Guess some of them can be done on master now.

XWayland re-stacking

I'm sure you're right on this, but couldn't find anything in sways commit log that gave me confidence that we're approaching it right. How do we gain confidence that this XWayland restacking stuff is right? What changed in wlroots to trigger this amendment?

Aside

As an aside, I spotted the following on sway master and think we could consider these at some point (but not pre-req to 0.18 merge). We could probably start a separate issue with these later, but am including them here for the minute for completeness.

@Consolatis
Copy link
Member Author

I'm sure you're right on this, but couldn't find anything in sways commit log that gave me confidence that we're approaching it right. How do we gain confidence that this XWayland restacking stuff is right? What changed in wlroots to trigger this amendment?

wlroots now handles that internally (and in fact asserts that the given surface is not an unamanged one), see https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/4052. That was implemented in wlroots to fix a sway bug because it did not restack unmanged surfaces on top like we do / did.

@kode54
Copy link
Contributor

kode54 commented May 24, 2024

Frame rate issues seem to be resolved now.

@Consolatis
Copy link
Member Author

Rebased on master and disabled the fps counter, can be re-enabled by uncommenting #define FPS_TIMEOUT (2 * 1000) in src/output-state.c.

@kode54
Copy link
Contributor

kode54 commented May 24, 2024

Closing virt-manager window for a VM now crashes with memory corruption:

#0  __pthread_kill_implementation
    (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0)
    at pthread_kill.c:44
#1  0x00007604079e4eb3 in __pthread_kill_internal
    (threadid=<optimized out>, signo=6) at pthread_kill.c:78
#2  0x000076040798ca30 in __GI_raise (sig=sig@entry=6)
    at ../sysdeps/posix/raise.c:26
#3  0x00007604079744c3 in __GI_abort () at abort.c:79
#4  0x0000760407975354 in __libc_message_impl
    (fmt=fmt@entry=0x760407afe2ea "%s\n") at ../sysdeps/posix/libc_fatal.c:132
#5  0x00007604079ef085 in malloc_printerr
    (str=str@entry=0x760407afbffe "corrupted double-linked list")
    at malloc.c:5772
#6  0x00007604079efbfc in unlink_chunk
    (p=p@entry=0x5af189f7bcf0, av=0x760407b32ac0 <main_arena>) at malloc.c:1617
#7  0x00007604079f2a9a in _int_malloc
    (av=av@entry=0x760407b32ac0 <main_arena>, bytes=bytes@entry=208)
    at malloc.c:4381
#8  0x00007604079f477e in __libc_calloc
    (n=n@entry=1, elem_size=elem_size@entry=208) at malloc.c:3754
#9  0x0000760408397130 in zalloc (s=208)
    at ../wayland-1.22.0/src/wayland-private.h:234
#10 wl_closure_init
    (message=message@entry=0x760408385fb0 <zwlr_foreign_toplevel_handle_v1_events.lto_priv+144>, size=size@entry=0, num_arrays=num_arrays@entry=0x0, args=args@entry=0x7ffe3d108350) at ../wayland-1.22.0/src/connection.c:572
#11 0x0000760408399abf in wl_closure_marshal
    (message=0x760408385fb0 <zwlr_foreign_toplevel_handle_v1_events.lto_priv+144>, args=0x7ffe3d108350, opcode=6, sender=0x5af189f9c7a0)
    at ../wayland-1.22.0/src/connection.c:608
#12 handle_array
    (resource=resource@entry=0x5af189f9c7a0, opcode=opcode@entry=6, args=args@entry=0x7ffe3d108350, send_func=send_func@entry=0x7604083976f0 <wl_closure_send>)
    at ../wayland-1.22.0/src/wayland-server.c:224
#13 0x0000760408399cf0 in wl_resource_post_event_array
    (resource=resource@entry=0x5af189f9c7a0, opcode=opcode@entry=6, args=args@entry=0x7ffe3d108350) at ../wayland-1.22.0/src/wayland-server.c:244
#14 0x0000760408399ddf in wl_resource_post_event
    (resource=resource@entry=0x5af189f9c7a0, opcode=opcode@entry=6)
    at ../wayland-1.22.0/src/wayland-server.c:259
#15 0x00007604082f3eb8 in zwlr_foreign_toplevel_handle_v1_send_closed
    (resource_=0x5af189f9c7a0)
    at protocol/wlr-foreign-toplevel-management-unstable-v1-protocol.h:490
#16 wlr_foreign_toplevel_handle_v1_destroy (toplevel=0x5af189f9c6c0)
    at ../wlroots-hidpi-xprop-git/types/wlr_foreign_toplevel_management_v1.c:499
#17 wlr_foreign_toplevel_handle_v1_destroy (toplevel=0x5af189f9c6c0)
    at ../wlroots-hidpi-xprop-git/types/wlr_foreign_toplevel_management_v1.c:489
#18 0x00005af17d759615 in view_destroy (view=0x5af189f7be60)
    at ../labwc/src/view.c:2325
#19 0x000076040839900e in wl_signal_emit_mutable
    (signal=signal@entry=0x5af189ef4da8, data=data@entry=0x0)
    at ../wayland-1.22.0/src/wayland-server.c:2241
#20 0x00007604082ec0c8 in destroy_xdg_surface (surface=0x5af189ef4d00)
    at ../wlroots-hidpi-xprop-git/types/xdg_shell/wlr_xdg_surface.c:492
#21 0x00007604082f1e0d in surface_destroy_role_object (surface=0x5af189ef48a0)
    at ../wlroots-hidpi-xprop-git/types/wlr_compositor.c:910
#22 surface_destroy_role_object (surface=0x5af189ef48a0)
    at ../wlroots-hidpi-xprop-git/types/wlr_compositor.c:904
#23 surface_handle_role_resource_destroy
    (listener=0x5af189ef4bf0, data=<optimized out>)
    at ../wlroots-hidpi-xprop-git/types/wlr_compositor.c:891
#24 0x000076040839a99f in wl_priv_signal_final_emit
    (data=<optimized out>, signal=<optimized out>)
    at ../wayland-1.22.0/src/wayland-server.c:2377
#25 destroy_resource (element=0x5af189ef4e50, data=data@entry=0x0, flags=0)
    at ../wayland-1.22.0/src/wayland-server.c:729
#26 0x000076040839c7e9 in wl_resource_destroy (resource=<optimized out>)
    at ../wayland-1.22.0/src/wayland-server.c:749
#27 0x00007604073fc596 in ffi_call_unix64 () at ../src/x86/unix64.S:104
#28 0x00007604073f900e in ffi_call_int
    (cif=cif@entry=0x7ffe3d1087e0, fn=<optimized out>, rvalue=<optimized out>, avalue=<optimized out>, closure=closure@entry=0x0) at ../src/x86/ffi64.c:673
#29 0x00007604073fbbd3 in ffi_call
    (cif=cif@entry=0x7ffe3d1087e0, fn=<optimized out>, rvalue=rvalue@entry=0x0, avalue=avalue@entry=0x7ffe3d1088b0) at ../src/x86/ffi64.c:710
#30 0x0000760408397ab0 in wl_closure_invoke
    (closure=closure@entry=0x5af189ed0500, target=<optimized out>,
    target@entry=0x5af189ef4e50, opcode=opcode@entry=0, data=<optimized out>,
    data@entry=0x5af189e79b40, flags=2)
    at ../wayland-1.22.0/src/connection.c:1025
#31 0x000076040839c198 in wl_client_connection_data
    (fd=<optimized out>, mask=<optimized out>, data=<optimized out>)
    at ../wayland-1.22.0/src/wayland-server.c:438
#32 0x000076040839ab02 in wl_event_loop_dispatch
    (loop=0x5af188923c20, timeout=timeout@entry=-1)
    at ../wayland-1.22.0/src/event-loop.c:1027
#33 0x000076040839b317 in wl_display_run (display=0x5af188923b30)
    at ../wayland-1.22.0/src/wayland-server.c:1493
#34 0x00005af17d7436e3 in main (argc=<optimized out>, argv=<optimized out>)
    at ../labwc/src/main.c:217

@droc12345
Copy link
Contributor

Started running this PR as my daily driver.
No problems so far, seems a tad snappier than the wlroots 17.* version, at least running the vulkan renderer.

Good job.

@kode54
Copy link
Contributor

kode54 commented May 25, 2024

Updated wlroots, one new commit dealing with direct scanout and color processing, and now the above crash isn't happening any longer?

@kode54
Copy link
Contributor

kode54 commented May 26, 2024

This apparently fixes the quarter screen issue I was having with Waydroid? Seems to work without requiring me to suspend my displays first.

@Consolatis Consolatis force-pushed the chase/0.18 branch 3 times, most recently from 0008d36 to 4f3fa22 Compare May 31, 2024 03:35
@droc12345
Copy link
Contributor

droc12345 commented Jun 1, 2024

I've been running this version for a while, and turned on logging this morning and noticed error messages

00:00:00.182 [ERROR] [types/wlr_layer_shell_v1.c:296] A configure is sent to an uninitialized wlr_layer_surface_v1 0x17d9a80

It doesn't seem to matter which window, every time I open a new app/window, the message appears.
conky, foot, etc trigger it, whatever it is I start, doesn't seem to affect the running.
I've updated everything this morning to check, wlroots, foot, and several apps, still the messages.

Edit to add: seems to come from src/xdg.c

if (xdg_surface->initial_commit) { wlr_log(WLR_DEBUG, "scheduling configure"); //FIXME: confirm potential xdg-deco mode wlr_xdg_surface_schedule_configure(xdg_surface); return; }

Note: I do run with decorations turned completely off

ETA2:
The error messages come from wlr_xdg_surface_schedule_configure, and it's only called in the
one place, but I only see the "scheduling configure" message occasionally, So I'm not sure what
exactly is triggering off the messages.

@Consolatis
Copy link
Member Author

Consolatis commented Jun 1, 2024

The error messages come from wlr_xdg_surface_schedule_configure, and it's only called in the
one place, but I only see the "scheduling configure" message occasionally, So I'm not sure what
exactly is triggering off the messages.

Its caused by the decoration manager:

xdg decorations must not be confirmed directly but only if xdg_surface->initialized == true, also needs storage of the requested deco mode or link from view to the deco object itself which can be used from the xdg commit handler

Edit:
Sorry, your log is actually pointing to a layer surface rather than a xdg surface. The same error should happen for xdg surface for the above reason. For layer surfaces this must be something different. Thanks for pointing it out.

@droc12345
Copy link
Contributor

droc12345 commented Jun 1, 2024

It does report both depending on whether a layer app or regular app.

Edit to add:
[types/wlr_layer_shell_v1.c:296] A configure is sent to an uninitialized wlr_layer_surface_v1
and
[types/xdg_shell/wlr_xdg_surface.c:169] A configure is scheduled for an uninitialized xdg_surface

@kode54
Copy link
Contributor

kode54 commented Jun 10, 2024

Is this worth rebasing without addressing the issues mentioned above? Or is there some pending work already?

@Consolatis
Copy link
Member Author

Is this worth rebasing without addressing the issues mentioned above?

Yes, rebased on current master.

Or is there some pending work already?

No, didn't yet look into them.

@kode54
Copy link
Contributor

kode54 commented Jun 10, 2024

I'll rebuild again tonight, but there was a bug I noticed with Fullscreen toggle, if toggling from a Maximized window. If toggling from Maximized, it wants to resize the window, but fit it within the same position offset as Maximized, and leaves the dock overlay onscreen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants