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

syncobj: use eventfd instead of stalling fd checks #9437

Merged
merged 20 commits into from
Mar 14, 2025

Conversation

gulafaran
Copy link
Contributor

@gulafaran gulafaran commented Feb 18, 2025

use eventfd and add it to the event loop and when it recieves a signal release the the queued surfacestate, this means we dont stall entire compositor when waiting for materilisation of the fd. and change its related usage.

should solve a lot of "compositor 5 fps while ingame overlay shows 300fps" and other similiar weird stalls.

on nvidia this shuffles compositor stalls to sometimes a noticeable rendering stutter, technically the acquire point isnt signaled fast enough so the buffer is stuck and nothing gets rendered until it is. pretty much the same stall as seen before only now its per surface rendering. but that i contribute to nvidia driver being simply shit, and is visible on other compositors.

NVIDIA/open-gpu-kernel-modules#777
NVIDIA/open-gpu-kernel-modules#743

fixes #7857
fixes #9340
fixes #9340
fixes #8588
fixes #7643
fixes #7317
fixes #9376
fixes #6844
fixes #6912

potentially helps:
#6617
#9011

remaining quirks to be fixed

  • [ x ] resizing rendering window like vkcube is delayed hard
  • its because we are getting a gazzillion xdg shell configure requests that fill the wayland event loop, and eventfd signal gets behind in the queue.
  • [ x ] walker crashes https://paste.pika-os.com/upload/bat-tiger-jaguar
  • Doesn't seem to work on GTK4:4.16 wmww/gtk4-layer-shell#50 requires gtk4-layer-shell 1.0.4
  • test and see if implicit/explicit sync didnt break hard or some other card/vendors AMD/Intel/etc
  • test and see if gpu/cpu usage didnt regress into other areas. not resetting damage or similiar
  • [ x ] syncobj points for async buffers

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

huge if works

@ikalco
Copy link
Contributor

ikalco commented Feb 26, 2025

wait... for ds, shouldn't we release the wl_buffer whenever DRM signals the out fence?
so the client can reuse that buffer, and the output won't artifact cause drm said scanout is done

edit:
uuuh it seems HL just isn't using the out fence lol, am I missing something?
for reference (https://docs.kernel.org/gpu/drm-kms.html)

“OUT_FENCE_PTR”:
Use this property to pass a file descriptor pointer to DRM. Once the Atomic Commit request call returns OUT_FENCE_PTR will be filled with the file descriptor number of a Sync File. This Sync File contains the CRTC fence that will be signaled when all framebuffers present on the Atomic Commit * request for that given CRTC are scanned out on the screen.

@gulafaran
Copy link
Contributor Author

wait... for ds, shouldn't we release the wl_buffer whenever DRM signals the out fence? so the client can reuse that buffer, and the output won't artifact cause drm said scanout is done

edit: uuuh it seems HL just isn't using the out fence lol, am I missing something? for reference (https://docs.kernel.org/gpu/drm-kms.html)

“OUT_FENCE_PTR”:
Use this property to pass a file descriptor pointer to DRM. Once the Atomic Commit request call returns OUT_FENCE_PTR will be filled with the file descriptor number of a Sync File. This Sync File contains the CRTC fence that will be signaled when all framebuffers present on the Atomic Commit * request for that given CRTC are scanned out on the screen.

There is a lot wrong at the moment, explicit sync currently relies on new buffer attached, the client can commit multiple times on the same buffer if it wants to, and does sometimes, and this PR probably exposed that quirk even more, and yeah direct scanout i havent begun reading into but removed that buffer manual buffer shenanigans because it was conflicting with the changes i made, there is also a few other things im checking that has popped up while testing

@gulafaran
Copy link
Contributor Author

wait... for ds, shouldn't we release the wl_buffer whenever DRM signals the out fence? so the client can reuse that buffer, and the output won't artifact cause drm said scanout is done

edit: uuuh it seems HL just isn't using the out fence lol, am I missing something? for reference (https://docs.kernel.org/gpu/drm-kms.html)

“OUT_FENCE_PTR”:
Use this property to pass a file descriptor pointer to DRM. Once the Atomic Commit request call returns OUT_FENCE_PTR will be filled with the file descriptor number of a Sync File. This Sync File contains the CRTC fence that will be signaled when all framebuffers present on the Atomic Commit * request for that given CRTC are scanned out on the screen.

And "releasing" when it comes to explicit sync is sending the release point not the buffer .release

@UjinT34
Copy link
Contributor

UjinT34 commented Feb 26, 2025

output->state->enableExplicitOutFenceForNextCommit() should be called before the commit if out fence is needed. It fills output->state->state().explicitOutFence.
Who should handle that fd?

Note that OUT_FENCE_PTR shouldn't be used when tearing is allowed.

@JunaidQrysh
Copy link

I tested this pr & it fixed glmark2 lag but minecraft lag is still the same and also it introduced slight lag in other areas like workspace switching and cursor lag when wlogout is opened.

@ikalco
Copy link
Contributor

ikalco commented Feb 26, 2025

output->state->enableExplicitOutFenceForNextCommit() should be called before the commit if out fence is needed. It fills output->state->state().explicitOutFence. Who should handle that fd?

Note that OUT_FENCE_PTR shouldn't be used when tearing is allowed.

we use that fd, i think we import it as a sync_file, then wait for it to signal, meaning aq plane is done with the primary buffer

@gulafaran
Copy link
Contributor Author

I tested this pr & it fixed glmark2 lag but minecraft lag is still the same and also it introduced slight lag in other areas like workspace switching and cursor lag when wlogout is opened.

yeah got a few very broken things to figure out, im trying tho.

@JunaidQrysh
Copy link

@gulafaran you did amazing work, now everything runs smoothly. I ran various gpu stress tests, they ran without any lag.
This will be a major change in hyprland for nvidia users. I can finally ditch kde plasma for gaming.

@ikalco
Copy link
Contributor

ikalco commented Mar 2, 2025

for this part

// #TODO does this apply to explicit sync?
    if (!syncobj && previousBuffer && previousBuffer->buffer && !previousBuffer->buffer->isSynchronous()) {

the lockedByBackend is unlocked when page flip buffer swap happens, so technically that's when we should signal release point since DRM scanout is no longer using the buffer
but also, im pretty sure this is what the DRM out fence does, so you could use that instead of lockedByBackend

@czM1K3
Copy link

czM1K3 commented Mar 2, 2025

I can also say that this fixed my issue with my RX 580 so it might fix something like #8119 and others.

@gulafaran
Copy link
Contributor Author

I can also say that this fixed my issue with my RX 580 so it might fix something like #8119 and others.

Probably will, and also some minecraft flickering and other things, im pushing soon a fix for a gtk issue and hyprlock issue so its not quite flawless yet 👍

@gulafaran
Copy link
Contributor Author

I tested this pr & it fixed glmark2 lag but minecraft lag is still the same and also it introduced slight lag in other areas like workspace switching and cursor lag when wlogout is opened.

should be in a better state now, but not done yet.

vaxerski
vaxerski previously approved these changes Mar 6, 2025
Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

fine by me. Runs alright on my end, but I don't know about others.

@fufexan
Copy link
Member

fufexan commented Mar 6, 2025

WFM as well, fixed desync problems.

gulafaran and others added 19 commits March 14, 2025 09:41
cleanup a bit missing removals if resource not good, erasing from
containers etc. make use of unique ptrs instead. and add default
destructors.
remove early buffer release that was breaking explicit sync, the buffer
needs to exist until the surface commit event has been emitted and draw
calls added egl sync points, move to eventfd signaling instead of
stalling sync point checks, and recommit pending commits if waiting on a
signal. add a CDRMSyncPointState helper class. move a few weak pointers
to shared pointers so they dont destruct before we need to use them.
eventfd requires us to queue pending stats until ready and then apply to
current, and also when no ready state exist commit the client commit on
the current existing buffer, if there is one.
clear current buffer damage on current buffer commits.
remove unused code, and ensure we dont commit a empty texture causing
locksession protocol and gtk4-layer-shell misbehaving.
ensure the containers having the various buffers actually gets cleaned
up from their containers, incase the CSignal isnt signaled because of
expired smart pointers or just wrong order destruction because mishaps.
also move the acquire/point setting to buffer attaching. instead of on
precommit.
remove unused code and merge sync fds if fence is valid, remove manual
directscanout buffer dropping that signals release point on pageflip, it
can cause us to signal the release point while still keeping the current
buffer and rendering it yet again causing wrong things.
delay buffer releases on non syncobj surfaces until next commit, and
check on async buffers if syncobj and drop and signal the release point
on backend buffer release.
ensure we follow protocol by replacing acquire/release points if they
arrive late and replace already existing ones. also remove unneded
brackets, and dont try to manual lock/release buffers when it comes to
explicit protocol. it doesnt care about buffer releases only about
acquire and release points and signaling them.
set points in precommit, before checking protocol errors and we catch
any pending acquire/release points arriving late.
remove destructor resource destroying, let resources destroys them on
their events, and move SSurfaceStates to types/SurfaceState.hpp
have to actually store the mergedfd to use it.
ensure the current asynchronous buffer is actually released on pageflip
not the previous. cleanup a bit FD handling in
commitPendingAndDoExplicitSync, and reuse the in fence when syncing
surfaces.
calling resetexplicitfence without properly ensuring the FD is closed
before will leak it, store it per monitor and let it close itself with
the CFileDescriptor class.
buffers were never being sent released properly.
ensure the infence fd survives the scope of attemptdirectscanout so it
doesnt close before it should have.
we might hit a race to finish on exit where the timeline just has
destructed but the buffer waiter is still pending. and such we
removeAllWaiters null dereferences.
remove quack comment, change to m_foo and use a std::vector and
weakpointer in the waiter for removal instead of a std::list.
vaxerski
vaxerski previously approved these changes Mar 14, 2025
Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

lgtm, are there any issues left? Works fine for me

remove unused async buffer drop, only related to directscanout and is
handled elsewhere.
Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

lets merge this shit

@vaxerski vaxerski merged commit 6ffde36 into hyprwm:main Mar 14, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment