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

Fix a bunch of issues with dropCurrentBuffer #8966

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

UjinT34
Copy link
Contributor

@UjinT34 UjinT34 commented Jan 5, 2025

Describe your PR, what does it fix/add?

Moved dropPendingBuffer closer to a point where we stop using it.
Commented out dropCurrentBuffer for sync buffers since it causes more trouble than solves.
Looks like it it either wrong to drop the buffer so early or there is a bug somewhere else. Gamescope complains about incorrect buffer releases from time to time (definitely not on every release) https://github.com/ValveSoftware/gamescope/blob/f873ec7868fe84d2850e91148bcbd6d6b19a7443/src/Backends/WaylandBackend.cpp#L786 For some reason it fails to detect that the buffer was acquired by the compositor. I guess that some other clients have similar issues but they are not smart enough to handle the release correctly and destroy the buffer upon receiving a release while they still need it for something internally.
This fixes
#6844
#6912
#8399 (comment) and bunch of other corrupted/invisible cursors with cursor:use_cpu_buffer

hyprwm/hyprpicker#85 seems to work fine. Couldn't reproduce #7091 but if it'll come back with this PR I'd rather have some minor green artifacts while having a stroke trying to resize kitty than issues mentioned above.

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

Tested it for a long time since #6844 (comment) without an issue. Still might need more testing, haven't checked everything related to #7110. Might behave differently with explicit sync disabled.
Maybe isSynchronous isn't always correct? Or some clients have a different idea about buffer sync? Or some events should be sent before the release?

Is it ready for merging, or does it need work?

Ready.

@vaxerski
Copy link
Member

vaxerski commented Jan 5, 2025

oh, no, the kitty thing is a very bad problem (it's when we drop a dma buffer before we are done sampling from it)

@vaxerski
Copy link
Member

vaxerski commented Jan 5, 2025

IIRC the early drop was for foot...?

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.

tested, lgtm

@vaxerski vaxerski merged commit 6a90b50 into hyprwm:main Jan 6, 2025
11 checks passed
@goggi
Copy link

goggi commented Jan 7, 2025

Hi! This is kind of strange, but after this commit I got stuttering in animations when a firefox based browser is present (both floorp and zen). It works fine if I revert back to 602d6b7

@UjinT34
Copy link
Contributor Author

UjinT34 commented Jan 7, 2025

Doesn't happen to me with FF 128.5.0esr. Might be specific to some video driver and explicit sync combo.

@goggi
Copy link

goggi commented Jan 7, 2025

Doesn't happen to me with FF 128.5.0esr. Might be specific to some video driver and explicit sync combo.

If I remove the comments for:

        // if (current.buffer->buffer->isSynchronous())
             // dropCurrentBuffer();

it works like expected again.

Its a very minor shuttering in the animation. If I have the browser (zen) on workspace 1 and then move to workspace 2 that contains vscode, the animation shutters. If I move back and forward fast between the workspace then i get around 10 fps drop. This does not happen if I move between workspace 2 (vscode) and workspace 3 (chrome)

@UjinT34
Copy link
Contributor Author

UjinT34 commented Jan 7, 2025

I guess dropping it there is too early for some scenarios and dropping it on the next commitPendingState is too late for other scenarios.
Couldn't reproduce it with intel and floorp 11.21.0.

@goggi
Copy link

goggi commented Jan 7, 2025

I guess dropping it there is too early for some scenarios and dropping it on the next commitPendingState is too late for other scenarios. Couldn't reproduce it with intel and floorp 11.21.0.

image

Im getting the same issue with both floorp and zen. Can try regular firefox later today. So you have no suggestion on something I could try out to fix it except for uncommenting the section I mentioned above?

@UjinT34
Copy link
Contributor Author

UjinT34 commented Jan 7, 2025

Try toggling explicit_sync and explicit_sync_kms. Looks like an amd bug or some specific config issue.
Basically client has two buffers A and B.
With this line

  1. client submits A
  2. hl does it's stuff with A
  3. hl releases A
  4. client is ready to handle the release of A
  5. client submits B
  6. client needs to work with A
  7. hl does it's stuff with B
  8. and so on...

Without this line

  1. client submits A
  2. hl does it's stuff with A
  3. client is ready to handle the release of A
  4. client submits B
  5. client needs to work with A
  6. hl releases A
  7. hl does it's stuff with B
  8. and so on...

Some clients are fine when A is released early. Some client are fine when A is released late. For everything to work fine we need to release A between 3 and 5. No idea where this point is... Maybe after presentFeedback? At least gamescope is ready to receive a release after some stuff related to Present.

@goggi
Copy link

goggi commented Jan 7, 2025

Hi! Toggling explicit_sync and explicit_sync_kms didnt not work. I will just create a new issue for now. Thanks for the help!

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

Successfully merging this pull request may close these issues.

3 participants