Skip to content

core: refactor and improve surface commit #9805

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

Merged
merged 7 commits into from
Apr 7, 2025

Conversation

ikalco
Copy link
Contributor

@ikalco ikalco commented Mar 30, 2025

Describe your PR, what does it fix/add?

part 2 of #9774

relevant commits:
cleanup SSurfaceState and surface pending commit tracking
move surface code from DRMSyncobj, and move acquire to SSurfaceState
use queue for comitted pending surface states like proto says
drop, not release, prev buffer if 2nd buffer wl_surface.attach is sent

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

based on part 1

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

should be gtg now
nvm lol nvidia borkde again (╥_╥)
yes finally

@ikalco ikalco closed this Mar 30, 2025
@ikalco ikalco reopened this Mar 30, 2025
@ikalco ikalco changed the title Refactor and improve surface commit core: refactor and improve surface commit Mar 30, 2025
@gulafaran
Copy link
Contributor

currently discord typing is occasionally delayed, not updating. minecraft flickers, cs2 flickers.

@gulafaran
Copy link
Contributor

and obs -> right click preview -> open windowed projector should die with

[ 653985.111] {Display Queue} wl_display#1.error(wp_linux_drm_syncobj_surface_v1#77, 4, "Missing acquire timeline")
wp_linux_drm_syncobj_surface_v1#77: error 4: Missing acquire timeline

as it does on -git, currently doesnt.

@ikalco
Copy link
Contributor Author

ikalco commented Mar 31, 2025

and obs -> right click preview -> open windowed projector should die with

[ 653985.111] {Display Queue} wl_display#1.error(wp_linux_drm_syncobj_surface_v1#77, 4, "Missing acquire timeline")
wp_linux_drm_syncobj_surface_v1#77: error 4: Missing acquire timeline

as it does on -git, currently doesnt.

fixed

@ikalco ikalco force-pushed the refactor_and_improve_surface_commit branch 2 times, most recently from 860ed07 to cbecc98 Compare April 2, 2025 04:34
@ikalco
Copy link
Contributor Author

ikalco commented Apr 2, 2025

works on nvidia now, no flicker or delayed frames :)
I had to dig up my old 1660 nvidia card (thank god I could repro lol), and this pr works on that now

@PlasmaPower
Copy link
Contributor

Did you test this with the vkcube FIFO mode Tom mentioned was broken? That might've been from another PR but still worth testing here

@ikalco
Copy link
Contributor Author

ikalco commented Apr 3, 2025

tested with explicit_sync both on and off, all work on nvidia

  • minecraft, vsync and unlimited, windowed and ds
  • csgo, windowed and ds
  • obs recording
  • vkcube --wsi wayland --present_mode {number here} with 0, 1, and 2

I'm also daily running this pr on an old Intel laptop with no issues (explicit enabled)

@ikalco
Copy link
Contributor Author

ikalco commented Apr 4, 2025

@vaxerski do you mind if we just merge this as one pr lol, cause I don't really wanna go through all the combinations of testing all over again for #9804 if I know this works

maybe we can do something like
git squash the buffer ref stuff into one commit
git squash the surface stuff into another
and merge those 2 kernel style like you said before

or just merge the entire pr into one commit like normal

p.s. the other two prs aren't required for this, still drafts until I get the time to clean/fix them up, discuss, and test

@ikalco ikalco mentioned this pull request Apr 4, 2025
6 tasks
@PlasmaPower
Copy link
Contributor

I've been running this PR for a couple days and it seems to be working well for my usecases on NVIDIA.

ikalco added 7 commits April 5, 2025 21:00
TODO: use CHLBufferReference in direct scanout properly
      the only problem is the scanout buffer release timing,
      specifically the onBackendRelease mechanism
"The content update is placed in a queue until it becomes active." - wl_surface::commit
"A wl_buffer that has been attached and then replaced by another attach instead of committed will not receive a release event, and is not used by the compositor." - wl_surface::attach
@ikalco ikalco force-pushed the refactor_and_improve_surface_commit branch from cbecc98 to 3002845 Compare April 6, 2025 02:01
@ikalco
Copy link
Contributor Author

ikalco commented Apr 6, 2025

rebased to main

also should be good for final review/merge
PlasmaPower, gulafaran, and I, dont have any glitching bugs

@vaxerski
Copy link
Member

vaxerski commented Apr 6, 2025

my question is what is this supposed to fix?

@ikalco
Copy link
Contributor Author

ikalco commented Apr 6, 2025

my question is what is this supposed to fix?

there isn't a specific bug this pr is fixing, but it is building towards more well defined surface commit behavior
so that in something like my proper buffer release pr (#9807) I know what gets called where,
and can hopefully fix the last of those pesky nvidia flickering bugs (#9640)

this pr also follows wayland surface protocol closer, like in the last 2 commits
and while I was in there changing all of that, I also decided to clean up SSurfaceState by

  • making updated easier to use
  • setting the right initial values based on the protocol text
  • grouping functions and adding some clarifying comments

p.s.

here are some exmaples of previously not well defined behavior
  • previously, buffer refs were tracked using a SP<CHLBufferReference> which has 2 ref counting methods, the SP and buffer->nLocks... that's confusing and unneeded, especially for having proper buffer releases

  • release and acquire points were held in the CHLBufferReference, which is wrong
    this pr moves the acquire point to SurfaceState,
    and the release will be done properly in the later release stuff pr

  • some commitPendingState calls were obscured through a events.precommit.emit() and done in DRMSyncobj protocol
    which again adds some confusion as to how and where state is committed
    and sometimes the DRMSyncobj protocol committed state incorrectly (some state in pending isn't committed...)

    if (!surface->pending.buffer && !PENDING_HAS_NEW_BUFFER && surface->current.buffer) {
            surface->current.bufferDamage.clear();
            surface->current.damage.clear();
            surface->commitPendingState(surface->current);
            return; // no new buffer, but we still have current around and a commit happend, commit current again.
      }
  • previously, the pending surface state after a commit was being reset in multiple places in different ways, now we just have SSurfaceState::reset() to clearly define that new pending state

  • simlarly to right above, the SSurfaceState::updateFrom just copied everything when a buffer commit happened, which is wrong. Now we only copy committed things

    if (ref.updated & SURFACE_UPDATED_BUFFER) {
            ref.updated &= ~SURFACE_UPDATED_BUFFER;
            *this = ref;
            ref.damage.clear();
            ref.bufferDamage.clear();
            ref.buffer = {};
    }

@vaxerski
Copy link
Member

vaxerski commented Apr 7, 2025

cs works fine on my end so yea

@vaxerski vaxerski merged commit da86db4 into hyprwm:main Apr 7, 2025
12 checks passed
aphelei pushed a commit to aphelei/Hyprland that referenced this pull request Apr 29, 2025
* make CHLBufferReference not a SP anymore

* copy over release and acquire points in CHLBufferReference

* use CHLBufferReference in screencopy and toplevel export

TODO: use CHLBufferReference in direct scanout properly
      the only problem is the scanout buffer release timing,
      specifically the onBackendRelease mechanism

* cleanup SSurfaceState and surface pending commit tracking

* move surface code from DRMSyncobj, and move acquire to SSurfaceState

* use queue for comitted pending surface states like proto says

"The content update is placed in a queue until it becomes active." - wl_surface::commit

* drop, not release, prev buffer if 2nd buffer wl_surface.attach is sent

"A wl_buffer that has been attached and then replaced by another attach instead of committed will not receive a release event, and is not used by the compositor." - wl_surface::attach
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.

4 participants