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

ThunkLibs: Add support for GLX/GL without thunking X11, thunked as FGL #1817

Closed
wants to merge 4 commits into from

Conversation

skmp
Copy link
Contributor

@skmp skmp commented Jun 30, 2022

Overview

This includes only the required bits from #1735 to support GLX thunking without X11 thunks.

The GL linking fixes in #1734 are not included.

A new entry is added to ThunksDB.json, "FGL" that does not depend on X11. The same library is used.

GL only thunk in nvidia AGX ORIN

Screenshot from 2022-06-30 16-15-44

Rationale

Our X11 thunks are (very) minimal. This allows us to thunk GL for most applications,

Depends

#1805 For custom hostcall support

Details

See #1735

Follow Up

  • Support for EGL, GLESv2, GLESv1

@skmp
Copy link
Contributor Author

skmp commented Jun 30, 2022

For the thunked xcb + unthunked libX11 case, https://xcb.freedesktop.org/MixingCalls/, XGetXCBConnection & XOpenDisplay might offer a solution there. It was an issue with the hostcall support, after all.

@skmp skmp force-pushed the skmp/optional-x11-thunks branch from 795239b to f1f4d66 Compare June 30, 2022 11:04
@Sonicadvance1
Copy link
Member

Change this so we have libGL thunk and libFGL so the two implementations can live side by side and I don't see any reason why it can't be tested at the same time.
We don't need to disrupt the previous implementation while this new path is being tested.

@skmp
Copy link
Contributor Author

skmp commented Aug 1, 2022

They are the same code, the mode of operation is detected by whenever X11 is thunked or not. FGL only exists as a json alias.

@Sonicadvance1
Copy link
Member

Except that it is changing a wackload of code from passthrough to custom implementations. Which I'm not sure if we agree is the right path forward yet.
Which is why having the two implementations side-by-side allows easier testing without breaking one or the other.

@skmp
Copy link
Contributor Author

skmp commented Aug 1, 2022

Only GLX is affected, and sadly, due to how libGL works on linux, GLX is also in libGL, not only libGLX.

@skmp
Copy link
Contributor Author

skmp commented Aug 1, 2022

So to keep separate impls, we'd have to also keep two libGLs with is easily 100x GLX in entrypoints

@Sonicadvance1
Copy link
Member

Yes. That's exactly what I want here.

@skmp
Copy link
Contributor Author

skmp commented Aug 1, 2022

(The original PR /did/ have two libs, this is the rewrite that merged them for less code)

@Sonicadvance1
Copy link
Member

I'm fine with more code to keep the implementations separate, or if you want to save some code duplication add some GL_Common folder or something then have each implementation inline a .inl file from that folder. Just for code that is identical.

@neobrain
Copy link
Member

neobrain commented Aug 2, 2022

As a general remark, I'll quote my comment from #1734:

As mentioned in other channels, what I need to review your ThunkLibs PRs is better separation of changes and documentation. The bare minimum needed is a rudimentary explanation of what problem each of the patches solves. Otherwise, we have no solid basis for discussion and I won't be able to gauge how the changes will impact my ongoing work.

I won't repeat another review like #1809 where I had to spend a full week until I understood what you're doing and actually got it into a working state. Since I'm continuing to drive forward the bigger-scheme thunking work, I don't have capacity to carry early-state PRs out of prototype state.

As for this PR, I don't know if we critically need to merge it right now since you haven't outlined your specific future plans about it. Currently, I see no drawback from leaving this unmerged until some specific later date so we can continue evaluating limitations of "regular" GL thunking. This will save us unnecessary refactoring and feature creep in the generator which would come back and haunt us at a later point. If the chosen date passes or if we hit a hard blocker in the regular GL thunks, we can still integrate FGL at that point.

As for how to integrate FGL, @Sonicadvance1's proposal sounds reasonable. Since this is more of a stopgap solution it makes sense to minimize code sharing between FGL and the other thunk libraries. (If we find regular-GL hits a blocker it would be preferable to have stronger integration, but again that's something we would be in a better position to decide later.) The separation must be strong enough not to overlap with any of my generator changes, and it must be possible to safely make changes to one code path without having to retest the other all the time. The last thing we need is two separate configurations where at least one is constantly broken.

Finally, I need to stress that until #1611 is merged, I'm not in a position where I can review generator or build system changes unless they are clearly communicated and well-motivated. This includes massively-copy-pasted code that would need to be updated manually if changes to the generator are made, as such generator changes are still happening frequently.

@skmp
Copy link
Contributor Author

skmp commented Aug 3, 2022

I'm fine with more code to keep the implementations separate, or if you want to save some code duplication add some GL_Common folder or something then have each implementation inline a .inl file from that folder. Just for code that is identical.

Sure, we can go this way. I just don't want to duplicate the code all over.

@skmp
Copy link
Contributor Author

skmp commented Aug 3, 2022

As a general remark, I'll quote my comment from #1734:

As mentioned in other channels, what I need to review your ThunkLibs PRs is better separation of changes and documentation. The bare minimum needed is a rudimentary explanation of what problem each of the patches solves. Otherwise, we have no solid basis for discussion and I won't be able to gauge how the changes will impact my ongoing work.

One can read code too, apart from documentation, I presume. I've offered endless time to discuss/call/text w/e about it.

I won't repeat another review like #1809 where I had to spend a full week until I understood what you're doing and actually got it into a working state. Since I'm continuing to drive forward the bigger-scheme thunking work, I don't have capacity to carry early-state PRs out of prototype state.

Yeah, so instead these have been pending for months now. Seems an improvement.

As for this PR, I don't know if we critically need to merge it right now since you haven't outlined your specific future plans about it.

That's your opinion, and I strongly disagree there. I have explained during several past meetups, and over discord, exactly why this is needed and is blocking progress. Last week I even offer to discuss how you want it structured, and I do the work.

Currently, I see no drawback from leaving this unmerged until some specific later date so we can continue evaluating limitations of "regular" GL thunking.

This has nothing to do with GL, and only with GLX.

This will save us unnecessary refactoring and feature creep in the generator which would come back and haunt us at a later point. If the chosen date passes or if we hit a hard blocker in the regular GL thunks, we can still integrate FGL at that point.

What feature creep? Nothing is special about this.

If you think the gen can get too complex, someone else can also work on it or refactor it.

As for how to integrate FGL, @Sonicadvance1's proposal sounds reasonable. Since this is more of a stopgap solution it makes sense to minimize code sharing between FGL and the other thunk libraries. (If we find regular-GL hits a blocker it would be preferable to have stronger integration, but again that's something we would be in a better position to decide later.)

Again, this is not about GL, but GLX.

The separation must be strong enough not to overlap with any of my generator changes, and it must be possible to safely make changes to one code path without having to retest the other all the time. The last thing we need is two separate configurations where at least one is constantly broken.

My view here is, the generator needs to serve the needs of the project, not the other way around.

Finally, I need to stress that until #1611 is merged, I'm not in a position where I can review generator or build system changes unless they are clearly communicated and well-motivated. This includes massively-copy-pasted code that would need to be updated manually if changes to the generator are made, as such generator changes are still happening frequently.

That's fine, we discussed yesterday with @Sonicadvance1 and he'll be taking over the review on this one, from what I understand.

@skmp
Copy link
Contributor Author

skmp commented Aug 4, 2022

After today's discussion,

  • This will not affect the non-FGL path in general, and users should take note that things may break with FGL.
  • Apart from getting thunks up-and-running fast, this will enable differential debugging (X11 thunks on vs X11 thunks off)
  • It will be much easier to do passthrough GL + FGLX rather than X11 thunking for 32-bits

@Sonicadvance1
Copy link
Member

I see nothing has moved on this still.

@skmp skmp marked this pull request as draft August 10, 2022 16:00
@skmp skmp modified the milestones: 2210, 2209 Aug 10, 2022
@skmp skmp self-assigned this Aug 10, 2022
@skmp
Copy link
Contributor Author

skmp commented Aug 29, 2022

I see nothing has moved on this still.

Yes, as I've been blocked doing noop work on other tickets, and of course this now has to be needlessly rebased, and there's been no progress in resolving disagreements in #1805.

I looked a bit more into separating GLX away from GL today, and it involves handling a lot of things that we don't currently handle.

I don't think it makes sense to separate them over such a small (~ 100 lines in total, excluding #1805) change.

Again, I'm re-iterating the value of this to make it to (some) release, as it

  • Removes X11 as a thunk dependency - we keep running into X11 bugs and issues
  • Enables a path towards 32-bit GL thunking with minimal effort (see the RFC/PoC)
  • The GL/GLX (and GLdispatch) splits are far bigger than this change is, and can be worked on as a follow up as we formalize our thunking and GL/GLX/GLdispatch infra.

@skmp
Copy link
Contributor Author

skmp commented Sep 1, 2022

(Closing this as there is an ongoing powergrab by @Sonicadvance1, I will migrate my work to https://github.com/skmp/fex-emu-ng.git)

@skmp skmp closed this Sep 1, 2022
@Sonicadvance1 Sonicadvance1 deleted the skmp/optional-x11-thunks branch August 23, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔖 Ready
Development

Successfully merging this pull request may close these issues.

3 participants