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

[FIXED] transferQueue queueFamilyIndex #965

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

siystar
Copy link

@siystar siystar commented Sep 10, 2023

Pull Request Template

Description

Changed transfer queue setup to re-use the main graphics queue's queueFamilyIndex to avoid the need for queue family ownership transfer of resources.

The Vulkan spec 7.7.4 states: "Resources created with a VkSharingMode of VK_SHARING_MODE_EXCLUSIVE must have their ownership explicitly transferred from one queue family to another in order to access their content in a well-defined manner on a queue in a different queue family."

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Only one queue family has been observed until now.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@timoore
Copy link
Contributor

timoore commented Sep 19, 2023

While VSG doesn't currently emit the queue family ownership commands required to use a transfer queue in a different family, restricting the main queue and transfer queue to be in the same family makes the use of a transfer queue a lot less interesting in the future. As I understand it, queues in different families are more likely to be implemented in parallel in the hardware, whereas queues in single family could be just a driver construct. No guarantees, of course.

It's true that the use of queues in different families becomes more tractable if the requirement that the transfer queue support graphics operations be lifted. This is currently required in order to generate mipmaps on the GPU, but see https://community.khronos.org/t/how-to-generate-mipmaps-in-a-compute-shader/106630/2 . On my current machine (AMD integrated graphics), there is only one queue that supports graphics, but there are 4 in another queue family that support transfer and compute.

I guess I'm saying that it would be better to track the necessary queue family transfers than merge this PR.

@siystar
Copy link
Author

siystar commented Sep 20, 2023

The way to do queue family ownership transfers is with a BufferMemoryBarrier / ImageMemoryBarrier per resource. I see this as a possibility for the compile traversal's purposes, but not for the TransferTask's. The TransferTask is run every frame with potentially hundreds or thousands of entries depending on the application, and it's simply not economical to add that many barriers on a frame by frame basis.

At present, the changes made in this PR only affect the TransferTask's queue. The compile traversal's queue hasn't been changed, apart from adding a comment that it needs investigation.

Somewhat off-topic, an idea I've had to optimize the overlap of work for the TransferTask is to make the pipeline stages used for the transferCompletedSemaphore (and the potentially upcoming renderFinishedSemaphore) configurable. You'd set these to the pipeline stages that actually use the dynamic data. The idea is to allow applications that don't use dynamic data in their vertex or compute shaders to benefit from a greater overlap in command execution. It should be possible to collect the used pipeline stages during the CollectResourceRequirements traversal, and potentially adjust them at runtime with vsg::updateViewer, if needed.

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.

2 participants