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

[Vulkan] Device API Multi-streams, multi-queue, and initial multi-thread support #2802

Merged
merged 27 commits into from
Aug 27, 2021

Conversation

bobcao3
Copy link
Collaborator

@bobcao3 bobcao3 commented Aug 25, 2021

@AmesingFlank This changes the command list acquire and submit API so GGUI needs adjustments.

In Vulkan each command buffer and command pool must be limited to a single thread, in this case we will have a unique compute / graphics queue per thread, and each queue contains a command pool.

#2736

@bobcao3 bobcao3 requested review from k-ye and AmesingFlank August 25, 2021 23:56
@bobcao3 bobcao3 force-pushed the device-api-streams branch from a0045b5 to 384cdcf Compare August 25, 2021 23:57
@bobcao3
Copy link
Collaborator Author

bobcao3 commented Aug 25, 2021

/format

@bobcao3 bobcao3 changed the title [Device API] Multi-streams, multi-queue, and initial multi-thread support [Vulkan] Device API Multi-streams, multi-queue, and initial multi-thread support Aug 26, 2021
@bobcao3 bobcao3 requested review from g1n0st and KuribohG August 26, 2021 00:02
taichi/backends/vulkan/vulkan_device.h Outdated Show resolved Hide resolved
taichi/backends/vulkan/vulkan_device.h Outdated Show resolved Hide resolved
@k-ye
Copy link
Member

k-ye commented Aug 26, 2021

Shall we fix the empty root buffer first to unblock CI?

@bobcao3
Copy link
Collaborator Author

bobcao3 commented Aug 26, 2021

Shall we fix the empty root buffer first to unblock CI?

There is this seg fault bug with this PR tho and this also needs to be solved. XD

@bobcao3
Copy link
Collaborator Author

bobcao3 commented Aug 26, 2021

/format

virtual void command_sync() = 0;
// Each thraed will acquire its own stream
virtual Stream *get_compute_stream() = 0;
virtual Stream *get_graphics_stream() = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: only declare this for GraphicsDevice..?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

right

TI_ERROR("cannot find a queue");
}
std::unique_ptr<CommandList> cmd = new_command_list(config);
Stream *stream = get_compute_stream();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will get_compute_stream() always succeed?

Comment on lines +1261 to +1262
compute_stream_[tid] = std::make_unique<VulkanStream>(
*this, compute_queue_, compute_queue_family_index_);
Copy link
Collaborator

Choose a reason for hiding this comment

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

compute_queue_ and compute_queue_family_index_ might not always be valid :((

Currently in EmbeddedVulkanDevice, if params.is_for_ui is true, the compute queue won't be created.

This might be a good chance to fix up EmbeddedVulkanDevice a bit.

Copy link
Collaborator

@AmesingFlank AmesingFlank Aug 26, 2021

Choose a reason for hiding this comment

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

Maybe just change EmbeddedVulkanDevice to always create a compute queue for now so it doesn't break? There might be a lot of cleanup work to be done in embedded device, and those should prolly be left for future PRs.

Copy link
Collaborator

@AmesingFlank AmesingFlank Aug 26, 2021

Choose a reason for hiding this comment

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

Or we could have a standalone queue for copying buffers and stuff. idk.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should in future create dedicated transfer queue, but for all the devices & APIs taichi support, the compute queue will always be there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think let's create both queues in EmbeddedVulkanDevice for now and rename and rework it to VulkanDeviceLoader or something in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good.

Copy link
Collaborator

@AmesingFlank AmesingFlank Aug 26, 2021

Choose a reason for hiding this comment

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

I'd also be happy if you leave it for now and add a TODO, since I don't think ggui uses memcpy anywhere yet (and if it does, we can create a cmd list). This PR is big enough as it is lol.

@g1n0st
Copy link
Contributor

g1n0st commented Aug 27, 2021

Seems E RuntimeError: Alloc descriptor set from pool failed in test_mpm88.py @bobcao3

@bobcao3
Copy link
Collaborator Author

bobcao3 commented Aug 27, 2021

/format

@bobcao3 bobcao3 merged commit 5896048 into taichi-dev:master Aug 27, 2021
ailzhang added a commit that referenced this pull request Aug 27, 2021
@g1n0st g1n0st added the vulkan Vulkan backend label Aug 28, 2021
@Leonz5288 Leonz5288 mentioned this pull request Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vulkan Vulkan backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants