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

Multi-adapter support #227

Merged
merged 2 commits into from
Jan 9, 2023
Merged

Multi-adapter support #227

merged 2 commits into from
Jan 9, 2023

Conversation

rosefromthedead
Copy link
Contributor

Now Adapter, Device, Queue are bundled into DeviceHandle and created alongside Surface, or reused if we already created a compatible handle.

I'm open to design complaints, just wanted to prove that the logic would solve my problem from #224, and it does :). Now on x11 (through xwayland) the winit example works fine, and on wayland I get a different error message than before 🥳

All of this comes at the cost of a little extra user-side juggling, but in the winit example it's not much extra effort.

@lambda
Copy link

lambda commented Jan 2, 2023

Would just like to note that this branch fixes the issue for me as well; I am running on X11 (since I still can't screenshare using Zoom on Wayland), and have both integrated and discrete graphics. Some more detail on Zulip.

@rosefromthedead
Copy link
Contributor Author

Re: the discussion on Zulip - for what it's worth, I think this is complexity that has to happen somewhere. Picking an adapter with no criteria won't work, as we've seen for nvidia laptops. I tried to balance vello complexity and API consumer complexity, by exposing the fact that multiple adapters are in use but leaving the selection and reuse logic on our side. That logic could be left to the user, or the whole business could be hidden away, but whatever happens, there's gonna be more work somewhere.

@dfrg
Copy link
Collaborator

dfrg commented Jan 8, 2023

To add some context, I agree that we’ll need extra complexity somewhere. The open question for me is whether it’s actually necessary to maintain a collection of devices or just choose the appropriate one. In the former case, we’ll also need to duplicate the Renderer state and all associated resources per device. I also wonder what happens if you drag a window to another screen that may be attached to a different device. For the latter case, I’d rather add the logic to select a single device that supports the surface.

@dfrg
Copy link
Collaborator

dfrg commented Jan 8, 2023

I do appreciate the fix and this does address the immediate problem allowing people with dual GPUs to actually run the code so if you fix the conflicts I’ll be happy to approve this and get it merged. We can revisit to address the full implications later.

some `RenderContext` stuff is now `DeviceHandle` stuff, and device
handles are created alongside surfaces to ensure compatibility, but are
reused if possible.

Fixes linebender#224 wgpu Adapter and Surface might not be compatible
@rosefromthedead
Copy link
Contributor Author

That's weird, the merge conflicts weren't showing up locally, so I just rebased and hoped for the best.

I'd always imagined that dragging windows across screens should force apps to make a new surface for each new screen the window is on; maybe because I've been reading too many discussions about fractional scaling. In the absence of this, I see that things could get messy with my code.

Good point about duplication. Looking at wgpu again, I think it's possible to minimise the number of devices in use, but it would mean that a surface needs to switch device if a new surface is created. Room for improvement, I guess

Copy link
Collaborator

@dfrg dfrg 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, thanks!

@dfrg dfrg merged commit c57db1d into linebender:main Jan 9, 2023
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