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

DRM: Explicit synchorisation support #3717

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

DRM: Explicit synchorisation support #3717

wants to merge 6 commits into from

Conversation

RAOF
Copy link
Contributor

@RAOF RAOF commented Jan 10, 2025

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 RAOF requested a review from a team as a code owner January 10, 2025 07:16
@RAOF
Copy link
Contributor Author

RAOF commented Jan 10, 2025

At least two things should be resolved before landing this:

  1. 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
  2. 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)

RAOF added 4 commits January 13, 2025 15:36
We want to be able to load real hardware rendering platforms on top
in tests, so have the stub `DisplayPlatform` implement the simplest
of the concrete interfaces
When we're making a `StubBuffer` with non-empty `size`, make sure we also
set the stride to a reasonable value.
None of our code *really* cares that we don't want to get events
before calling `monitor.enable()`, and UDev in Ubuntu 25.04 has
changed behaviour so that events *are* received before enabling
the monitor, so just drop the test.
sync_timeline.value().resource,
SyncTimeline::Error::no_buffer,
"Timeline release sync point set, but no buffer committed"};
}
frame_callback_executor->spawn(std::move(executor_send_frame_callbacks));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha! Here is the race causing BadBufferTest.test_truncated_shm_file to fail, sometimes.

The process goes:

  • The WLCS test in surface creation makes a buffer-less commit while setting up the surface roles
  • That commit goes down this codepath, which primes a 16ms timer here. When that expires, it will call send_frame_callbacks(), which sends all the pending frame callbacks at the time of the 16ms-later call (not at the time of this call).
  • The WLCS test attaches a buffer, requests a frame event, commits, and waits for that frame event.
  • The buffer goes through here, gets a composition pass is triggered.
  • In the compositor thread the buffer is consumed triggering the the send_frame_callbacks, Mir sends the frame event.
  • The WLCS test attaches the bad buffer, requests a frame event, and waits, expecting dispatch on that frame event to receive the protocol error.
  • Sometimes the 16ms timer fires after the new buffer has been committed, but before the compositor thread consumes it. This results in the WLCS test seeing a frame event, and the test fails (because it didn't protocol error).

This whole 16ms-timer infrastructure is because Firefox has a frame/commit loop if we immediately dispatch the frame event on an empty commit.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • it will call send_frame_callbacks(), which sends all the pending frame callbacks at the time of the 16ms-later call (not at the time of this call).

That seems wrong but I'm not sure chosing the right "right" is straightforward

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.

2 participants