DRM: Explicit synchorisation support #3717

RAOF · 2025-01-10T07:16:02Z

This implements the linux_drm_syncobj_v1 explicit synchronisation protocol.

Currently only the simplest support - we accept acquire fences and wait for them to become ready before we process the associated wl_surface.commit, and we signal the release fence once we have finished with the buffer.

There are optimisation opportunities at both ends here - we could push the acquire fence deeper into the pipeline, and trigger the release fence sooner.

RAOF · 2025-01-10T07:18:58Z

At least two things should be resolved before landing this:

This currently lacks the miral-wlcs-integration needed to run it against the WLCS tests (this needs a bit of cleanup, particularly when we don't have DRM available), and
vkgears gets lower numbers with explicit sync than without; this needs investigation (but might just be because we do a bit more work in the Wayland event loop)

src/server/frontend_wayland/wl_surface.cpp

Saviq · 2025-03-20T18:22:29Z

@RAOF FYI there are non-trivial conflicts trying to rebase on main.

RAOF · 2025-03-24T07:34:48Z

I don't see any conflicts? (But do see things to fix)

Saviq · 2025-03-24T10:40:29Z

I don't see any conflicts?

Sorry, was looking at a wrong branch.

Saviq · 2025-04-08T12:17:31Z

NB: I've rebased #3595 on top of this and I can still see artifacts when running:

MIR_SERVER_PLATFORM_DISPLAY_LIBS=mir:atomic-kms \
MIR_SERVER_CURSOR=null \
./build-Debug/bin/mir_performance_tests

AlanGriffiths · 2025-04-23T08:45:18Z

The CI failures are all over the place:

The following tests FAILED:
	 24 - wlcs.GtkPrimarySelection.* (SEGFAULT)
	 55 - wlcs.FullSurface/RegionSurfaceInputCombinations.* (SEGFAULT)

The following tests FAILED:
	146 - miral-test.TestConfigFile.* (Failed)

The following tests FAILED:
	 52 - wlcs.TouchInputSubsurfaces/SubsurfaceMultilevelTest.* (SEGFAULT)
	 53 - wlcs.MultiRectEdges/RegionSurfaceInputCombinations.* (SEGFAULT)
	144 - miral-test.TestConfigFile.* (Failed)

The following tests FAILED:
	277 - mir_umock_unit_tests.InputPlatformProbe.* (Failed)

The following tests FAILED:
	 53 - wlcs.MultiRectEdges/RegionSurfaceInputCombinations.* (SEGFAULT)

The following tests FAILED:
	203 - mir_window_management_tests.MinimalWindowManagerTest.* (Failed)

The following tests FAILED:
	 57 - wlcs.ClippedLargerRegion/RegionSurfaceInputCombinations.* (SEGFAULT)

AlanGriffiths

Mostly some "housekeeping" notes before I get into reviewing the implementation.

One question that needs an answer: What properties do we get from PosixClock<CLOCK_MONOTONIC> that are not provided by std::chrono::steady_clock (which we use elsewhere)?

include/common/mir/time/posix_clock.h

AlanGriffiths · 2025-04-23T09:15:49Z

src/common/symbols.map

+    mir::time::clock_gettime_checked*;
+    mir::time::is_steady_specialisation::is_steady*;


Wrong stanza - Mir 2.19 has shipped

AlanGriffiths · 2025-04-23T09:24:45Z

src/common/time/posix_clock.cpp

Missing copyright nonsense

src/platform/graphics/drm_syncobj.cpp

AlanGriffiths · 2025-04-23T09:30:27Z

src/platform/graphics/drm_syncobj.h

Conflicts with include/platform/mir/graphics/drm_syncobj.h (but I don't think it is used)

AlanGriffiths · 2025-04-23T09:32:02Z

src/platform/graphics/drm_syncobj.cpp

@@ -0,0 +1,89 @@
+#include "mir/graphics/drm_syncobj.h"
+
+#include "mir/log.h"


tests/include/mir/test/doubles/stub_buffer.h

AlanGriffiths · 2025-04-23T10:02:45Z

Well, no trouble reproducing the test failures locally:

The following tests FAILED:
          6 - wlcs.ForeignToplevelHandleTest.* (Failed)
         26 - wlcs.Anchors/LayerSurfaceErrorsTest.* (SEGFAULT)
         27 - wlcs.Anchor/LayerSurfaceLayoutTest.* (SEGFAULT)
         28 - wlcs.Layer/LayerSurfaceLayerTest.* (SEGFAULT)
         30 - wlcs.Anchor/XdgPopupPositionerTest.* (Failed)
         35 - wlcs.ConstraintAdjustmentFlip/XdgPopupPositionerTest.* (Failed)
         48 - wlcs.MultiRectEdges/RegionSurfaceInputCombinations.* (SEGFAULT)
         49 - wlcs.DefaultEdges/RegionSurfaceInputCombinations.* (SEGFAULT)
         50 - wlcs.FullSurface/RegionSurfaceInputCombinations.* (SEGFAULT)
         53 - wlcs.MultiRectCorners/RegionSurfaceInputCombinations.* (SEGFAULT)
         54 - wlcs.SurfaceInputRegions/SurfaceInputCombinations.* (SEGFAULT)
         56 - wlcs.AllSurfaceTypes/TouchTest.* (Failed)
        142 - miral-test.WaylandExtensions.* (Failed)

Saviq · 2025-04-23T10:13:06Z

Well, no trouble reproducing the test failures locally:

NB: I did just rebase this on main, naively — there were no conflicts, but…

AlanGriffiths · 2025-04-23T10:17:02Z

Well, here's one of the segfaults:

(gdb) bt
#0  __strcmp_evex () at ../sysdeps/x86_64/multiarch/strcmp-evex.S:318
#1  0x00007fffe5fc6b49 in ??? () at /lib/x86_64-linux-gnu/libgallium-24.2.8-1ubuntu1~24.04.1.so
#2  0x00007fffe5fc804e in ??? () at /lib/x86_64-linux-gnu/libgallium-24.2.8-1ubuntu1~24.04.1.so
#3  0x00007fffe58e898b in ??? () at /lib/x86_64-linux-gnu/libgallium-24.2.8-1ubuntu1~24.04.1.so
#4  0x00007fffe58ec6a0 in ??? () at /lib/x86_64-linux-gnu/libgallium-24.2.8-1ubuntu1~24.04.1.so
#5  0x00007fffdd7c8176 in ??? () at /lib/x86_64-linux-gnu/libEGL_mesa.so.0
#6  0x00007fffdd7b9cc3 in ??? () at /lib/x86_64-linux-gnu/libEGL_mesa.so.0
#7  0x00007fffdaf5f8c7 in (anonymous namespace)::make_share_only_context (dpy=0x7fffedfb7040, share_with=std::optional [no contained value])
    at /home/alan/CLionProjects/mir/src/platforms/gbm-kms/server/surfaceless_egl_context.cpp:51
#8  0x00007fffdaf5faa5 in mir::graphics::gbm::SurfacelessEGLContext::SurfacelessEGLContext (this=0x7fffec0da8f0, dpy=0x7fffedfb7040)
    at /home/alan/CLionProjects/mir/src/platforms/gbm-kms/server/surfaceless_egl_context.cpp:62
#9  0x00007fffdaf14373 in std::make_unique<mir::graphics::gbm::SurfacelessEGLContext, void*> () at /usr/include/c++/13/bits/unique_ptr.h:1070
#10 0x00007fffdaf0d8cf in mir::graphics::gbm::RenderingPlatform::RenderingPlatform (this=0x7fffed6dd9c0, hw=warning: RTTI symbol not found for class 'std::_Sp_counted_deleter<gbm_device*, (anonymous namespace)::gbm_device_for_udev_device(mir::udev::Device const&, std::vector<std::shared_ptr<mir::graphics::DisplayPlatform>, std::allocator<std::shared_ptr<mir::graphics::DisplayPlatform> > > const&)::{lambda(gbm_device*)#1}, std::allocator<void>, (__gnu_cxx::_Lock_policy)2>'
warning: RTTI symbol not found for class 'std::_Sp_counted_deleter<gbm_device*, (anonymous namespace)::gbm_device_for_udev_device(mir::udev::Device const&, std::vector<std::shared_ptr<mir::graphics::DisplayPlatform>, std::allocator<std::shared_ptr<mir::graphics::DisplayPlatform> > > const&)::{lambda(gbm_device*)#1}, std::allocator<void>, (__gnu_cxx::_Lock_policy)2>'
std::variant [index 1] containing std::shared_ptr<gbm_device> (use count 2, weak count 0) = {...})
    at /home/alan/CLionProjects/mir/src/platforms/gbm-kms/server/kms/platform.cpp:397
#11 0x00007fffdaf0d7b6 in mir::graphics::gbm::RenderingPlatform::RenderingPlatform (this=0x7fffed6dd9c0, device=..., platforms=std::vector of length 1, capacity 1 = {...})
    at /home/alan/CLionProjects/mir/src/platforms/gbm-kms/server/kms/platform.cpp:389
#12 0x00007fffdaf4c578 in mir::(anonymous namespace)::make_module_ptr<mir::graphics::gbm::RenderingPlatform, mir::udev::Device&, std::vector<std::shared_ptr<mir::graphics::DisplayPlatform>, std::allocator<std::shared_ptr<mir::graphics::DisplayPlatform> > > const&> () at /home/alan/CLionProjects/mir/include/common/mir/module_deleter.h:98
#13 0x00007fffdaf4a328 in create_rendering_platform (device=..., platforms=std::vector of length 1, capacity 1 = {...}) at /home/alan/CLionProjects/mir/src/platforms/gbm-kms/server/kms/platform_symbols.cpp:86
#14 0x00007ffff6ecc948 in mir::DefaultServerConfiguration::the_rendering_platforms (this=0x7fffedee9cb0) at /home/alan/CLionProjects/mir/src/server/graphics/default_configuration.cpp:303

(Not sure why there's no symbols for libEGL_mesa.so.0 debuginford seems to be working)

AlanGriffiths · 2025-04-23T10:18:50Z

NB: I did just rebase this on main, naively — there were no conflicts, but…

To get even I just pushed the "housekeeping" fixes without fixing the tests

RAOF · 2025-05-02T06:40:15Z

One question that needs an answer: What properties do we get from PosixClock<CLOCK_MONOTONIC> that are not provided by std::chrono::steady_clock (which we use elsewhere)?

Fundamentally, we get the property that PosixClock<CLOCK_MONOTONIC> is backed by CLOCK_MONOTONIC, which is required by the Wayland protocol. It is not guaranteed that std::chrono::steady_clock is the same as CLOCK_MONOTONIC and there are other, reasonable choices a C++ standard library could make, like CLOCK_MONOTONIC_RAW.

RAOF · 2025-05-02T07:04:31Z

Well, no trouble reproducing the test failures locally:

Bah! This is because you've got different hardware to me (and CI) - none of those fail in CI. The ones that fail in CI look to be mostly races, I think - the miral config file one, the window on disconnected output looks like maybe a 1sec timeout? The ones that aren't seem to be the ubsan ones, which are still worth examining.

RAOF · 2025-05-06T08:01:25Z

In the interest of making reviews easier - and isolating the CI failures - this has been rebased on #3924.

…ructure

AlanGriffiths · 2025-05-30T13:41:53Z

In the interest of making reviews easier - and isolating the CI failures - this has been rebased on #3924.

As that landed, rebased on main and...

github-actions · 2025-05-30T14:06:51Z

TICS Quality Gate

❌ Failed

mir

1 Condition(s) failed

❌ Condition “Δ Average Code Coverage ≥ 0.00% for each file with respect to Previous analysis” failed for 3 files.

File	Blocking
src/server/frontend_wayland/wl_surface.h	-33.34%
src/server/frontend_wayland/wl_surface.cpp	-11.39%
src/server/frontend_wayland/wayland_connector.cpp	-0.46%

See the results in the TICS Viewer

The following files have been checked for this project

include/common/mir/time/posix_clock.h
include/platform/mir/graphics/drm_syncobj.h
include/platform/mir/graphics/platform.h
src/common/time/posix_clock.cpp
src/platform/graphics/drm_syncobj.cpp
src/platforms/gbm-kms/server/buffer_allocator.cpp
src/platforms/gbm-kms/server/buffer_allocator.h
src/platforms/gbm-kms/server/kms/platform.cpp
src/server/frontend_wayland/linux_drm_syncobj.cpp
src/server/frontend_wayland/linux_drm_syncobj.h
src/server/frontend_wayland/wayland_connector.cpp
src/server/frontend_wayland/wayland_connector.h
src/server/frontend_wayland/wayland_default_configuration.cpp
src/server/frontend_wayland/wl_surface.cpp
src/server/frontend_wayland/wl_surface.h

TICS / TICS / Run TICS analysis

RAOF requested a review from a team as a code owner January 10, 2025 07:16

RAOF commented Jan 17, 2025

View reviewed changes

src/server/frontend_wayland/wl_surface.cpp Outdated Show resolved Hide resolved

RAOF force-pushed the MIRENG-798 branch from 21ed509 to 27240a8 Compare February 7, 2025 00:56

RAOF changed the base branch from main to MIRENG-952/improve-firefox-frame-event-quirk February 7, 2025 00:57

Base automatically changed from MIRENG-952/improve-firefox-frame-event-quirk to main February 7, 2025 10:37

Saviq marked this pull request as draft March 20, 2025 10:11

Saviq marked this pull request as ready for review March 20, 2025 10:11

RAOF force-pushed the MIRENG-798 branch from 6a2f57f to fc67af6 Compare March 25, 2025 06:26

RAOF force-pushed the MIRENG-798 branch from fc67af6 to b5c9e79 Compare April 14, 2025 03:46

Saviq force-pushed the MIRENG-798 branch from b5c9e79 to 03be74f Compare April 23, 2025 04:53

Saviq requested a review from AlanGriffiths April 23, 2025 05:32

AlanGriffiths requested changes Apr 23, 2025

View reviewed changes

RAOF force-pushed the MIRENG-798 branch from 21ac360 to 9adf8ff Compare May 6, 2025 07:53

RAOF changed the base branch from main to use-real-rendering-platforms-in-miral-test May 6, 2025 07:53

RAOF force-pushed the MIRENG-798 branch from 9adf8ff to 00afcca Compare May 6, 2025 07:54

AlanGriffiths force-pushed the use-real-rendering-platforms-in-miral-test branch from 699502a to 12bf5d5 Compare May 9, 2025 09:00

AlanGriffiths added the blocked label May 27, 2025

Base automatically changed from use-real-rendering-platforms-in-miral-test to main May 30, 2025 13:21

AlanGriffiths removed the blocked label May 30, 2025

RAOF and others added 4 commits May 30, 2025 14:35

common: Add a Clock implementation backed by a known Posix clock.

15e2f03

Implement linux_drm_syncobj_v1 explicit-synchronisation support

86f50bd

common/symbols.map: Add new symbols for mir::time::PosixClock infrast…

dd074c8

…ructure

Tidy up

f443197

AlanGriffiths force-pushed the MIRENG-798 branch from 00afcca to f443197 Compare May 30, 2025 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

DRM: Explicit synchorisation support #3717

DRM: Explicit synchorisation support #3717

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		mir::time::clock_gettime_checked*;
		mir::time::is_steady_specialisation::is_steady*;

		@@ -0,0 +1,89 @@
		#include "mir/graphics/drm_syncobj.h"

		#include "mir/log.h"

DRM: Explicit synchorisation support #3717

Are you sure you want to change the base?

DRM: Explicit synchorisation support #3717

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TICS Quality Gate

❌ Failed

mir

1 Condition(s) failed

Uh oh!

Uh oh!