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

remove external swapchain usage from drm_egl #15948

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Dudemanguy
Copy link
Member

@kasper93: It turned out to be way simpler than I thought.

@Dudemanguy Dudemanguy force-pushed the nuke-external-swapchain branch from 420ac6e to 3c77246 Compare February 23, 2025 20:17
@sfan5 sfan5 self-requested a review February 23, 2025 20:21
Copy link

github-actions bot commented Feb 23, 2025

Download the artifacts for this pull request:

Windows
macOS

5370069 introduced this while
implementing n-buffering. The changes are all good, but the suspect part
is the implementation of wait_fence/new_fence to specifically bypass
what is done generically in context.c for all other opengl backends. The
commit message says that the reason for this is because "the places we
create and wait for the fences needs to be somewhat different for best
performance". It's certainly plausible that it may have been true then,
but I see no evidence it's true now. Driver code is way different these
days under the hood, and the general vo drm code has been rewritten a
lot since then. The generic fencing in context.c works fine and shows no
difference compared to this custom implementation. Delete it and
simplify.
With the previous commits, all usage of this is gone. We can remove it
for good.
@Dudemanguy Dudemanguy force-pushed the nuke-external-swapchain branch from 3c77246 to 6756c56 Compare February 23, 2025 22:02
@Dudemanguy
Copy link
Member Author

Sorry about swiping two commits before you had a chance to review. Wanted to fix breakage I caused.

Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

looks good.
will test later but I don't expect any problems

@kasper93
Copy link
Contributor

Works for me.

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.

3 participants