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

Allow vkb::Device::get_queue to return a graphics queue as a fallback when no separate compute / transfer queue found. #175

Closed
wants to merge 3 commits into from

Conversation

APassbyDreg
Copy link

Plz forgive my stupid mistake in PR #174 🤣

===========================================

As mentioned in #109 , vkb currently throws an error when trying to get a compute / transfer queue in low-end devices that has no separate queues.

Following changes are made to allow vkb::Device::get_queue(_index) to return a graphics queue (index) as a fallback:

  1. add a undesired_flags parameter to vkb::detail::get_first_queue_index, it defaults to 0 to avoid changing existing calls
  2. use a modified implementation of get_seperate_queue_index which removes the graphics queue limitation to rewrite the function
  3. replace get_seperate_queue_index in vkb::Device::get_queue_index with get_first_queue_index and specify graphics & compute/transfer queue as undesired flags

Hope this can help.

@charles-lunarg
Copy link
Owner

Having get_queue return nothing when requesting a transfer queue or compute queue that isn't separate from graphics is by design.

What happens is that applications must know when the have a 'distinct' compute/transfer queue, since it affects the way in which semaphores are used. Not to mention, queues are externally synchronized, so its important they not think they have two separate queues when in reality its the same VkQueue handle.

I believe the intent of the mentioned issue is that a queue can support transfer operations even if it doesn't say it does. This only applies to transfer queues, not compute. And I hadn't resolved it because it doesn't mesh well with the existing queue logic. Not to mention, its a 'corner case' in the spec that to my knowledge doesn't exist in real drivers.

@APassbyDreg
Copy link
Author

I do agree that applications must know if there is a 'distinct' compute/transfer queue. However, this can be left for the users to perform a simple additional check on the returned queue handle, but not preventing them to get one.

Having only one queue supporting both graphics and compute functionality is not rare in low-end devices, especially integrated graphics card or older laptop graphics card like GTX960M.

@charles-lunarg
Copy link
Owner

So I am not going to budge on the current behavior. Trying to get a transfer queue or compute queue should not return the graphics queue if there is only 1 queue, this prevents users from misusing the queue by thinking its a separate queue.

However, what your PR indicates is that the stated design is not communicated well through the API or the documentation. This absolutely can be improved.

@APassbyDreg
Copy link
Author

This might be improved through an extra dedicate parameter in get_queue(_index) to maintain current behavior. But I'm not sure whether this is the best solution. After all, there exists a get_dedicated_queue(_index) function that should be used explicitly if a separate queue is needed.

There's another problem that current implementation may return duplicated compute / transfer queue (index) through get_queue(_index) function, which violates the distinct assumption.

@charles-lunarg
Copy link
Owner

I think a solution to this problem would be returning 'all' the queues in a single struct, rather than allowing individual querying of queues.
This works around the 'what if the user asks for a transfer/compute queue but its actually the same underlying queue?" issue by having each of the 3 queue types only be present if they are distinct from the other two.

struct Queue {
    VkQueue queue;
    VkQueueFlags capabilities; //specifies all the 'capabilities' of this queue.
    bool supports_present;
};
struct Queues {
    Queue graphics; 
    Queue compute; // non-null queue member if distinct from the graphics queue
    Queue transfer; // non-null queue member if distinct from the graphics queue & compute
    
};

Result<Queues> vkb::Device::get_queues() {...}

@charles-lunarg
Copy link
Owner

So after coming back to this repo and trying to clean things up, this is not the only instance of problems with the current design for getting VkQueues. It is probably the part of the API that was reworked the most, and did not have a satisfying design when I first released vk-bootstrap.

That said, I do not think I can take this PR mostly because it is doesn't do enough to clean things up and make things better. I am not closing the issue yet, rather I would like to keep it open until I create my own PR to implement what I think is a better solution. As a reminder, this will stay open.

I'd love to have your review & feedback when that happens, which hopefully should be soon (feel free to @ me if you think its been too long).

@charles-lunarg
Copy link
Owner

I have begun work on a solution to this here:
https://github.com/charles-lunarg/vk-bootstrap/tree/queue_rework

Needs tests & documentation, but the API is solidified (check VkBootstrap.h)

@charles-lunarg
Copy link
Owner

Closing in favor of #214

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