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

Fix: Error output is not require, VK_QUEUE_TRANSFER_BIT is optional. #99413

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

Alex2782
Copy link
Member

@Alex2782 Alex2782 commented Nov 19, 2024

Fixes #99412

A queue family with the requested bits could not be found.

https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkQueueFlagBits.html

...reporting the VK_QUEUE_TRANSFER_BIT capability separately for that queue family is optional.

@Alex2782 Alex2782 requested a review from a team as a code owner November 19, 2024 01:06
@clayjohn clayjohn requested a review from DarioSamo November 19, 2024 01:15
@clayjohn clayjohn added this to the 4.4 milestone Nov 19, 2024
@Alex2782
Copy link
Member Author

Alex2782 commented Nov 19, 2024

Note: it's not a critical error, everything works so far. The error message constantly appears in the Android editor as soon as a project is started. (also Logcat - Android Studio)

Tested also on MacMini M1 Molten / Vulkan, VK_QUEUE_TRANSFER_BIT is available there.

image

transfer_queue_family = driver->command_queue_family_get(RDD::COMMAND_QUEUE_FAMILY_TRANSFER_BIT);
if (transfer_queue_family) {
// Create the transfer queue.
transfer_queue = driver->command_queue_create(transfer_queue_family);
ERR_FAIL_COND_V(!transfer_queue, FAILED);
} else {
// Use main queue as the transfer queue.
transfer_queue = main_queue;
transfer_queue_family = main_queue_family;

@DarioSamo
Copy link
Contributor

DarioSamo commented Nov 19, 2024

I'm afraid this seems like the wrong way to fix the problem and it just adds more complexity to code that shouldn't have it.

If the issue is that the error message shows up, then I suggest making a PR to handle this...

ERR_FAIL_COND_V_MSG(picked_family_index >= queue_family_properties.size(), CommandQueueFamilyID(), "A queue family with the requested bits could not be found.");

...as a correct case instead and just make it return the empty Id without printing an error.

To further support that, the code that calls this already handles the empty Id properly:

transfer_queue_family = driver->command_queue_family_get(RDD::COMMAND_QUEUE_FAMILY_TRANSFER_BIT);
if (transfer_queue_family) {
// Create the transfer queue.
transfer_queue = driver->command_queue_create(transfer_queue_family);
ERR_FAIL_COND_V(!transfer_queue, FAILED);
} else {
// Use main queue as the transfer queue.
transfer_queue = main_queue;
transfer_queue_family = main_queue_family;
}

So to me it just looks like a case of an error message that shouldn't be printed.

@Alex2782 Alex2782 changed the title Fix: Avoid using optional VK_QUEUE_TRANSFER_BIT unsupported on most Android devices Fix: Error output is not require, VK_QUEUE_TRANSFER_BIT is optional. Nov 19, 2024
Copy link
Contributor

@DarioSamo DarioSamo left a comment

Choose a reason for hiding this comment

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

Change looks good to me now.

For your consideration as well: now that we're properly requesting it like this and we're fully aware Android does not cover this case, it might be worth considering to add code to RenderingDevice when it requests the transfer family to attempt to get a compute queue instead if it fails to find it.

As it is now, the code will just fall back to the graphics queue. However, the existence of the compute bit implies the capability of transfer, so it should be possible to use asynchronous compute queues if they're supported.

I don't think a check like that needs to be part of this PR, and I think it's subject to some testing if it actually gives much of a benefit on Android devices.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally on Samsung Galaxy Z Fold4 (Android 14), it works as expected. I used to get an error on startup before this change.

@Alex2782
Copy link
Member Author

... However, the existence of the compute bit implies the capability of transfer, so it should be possible to use asynchronous compute queues if they're supported.

... and I think it's subject to some testing if it actually gives much of a benefit on Android devices.

Android devices with multiple queue families are very rare (and very expensive).
Such smartphones have only been available for a few weeks. It will probably only be worth looking into it in more detail in 2 to 3 years.

Adreno (TM) 830 (like Xiaomi 15 (Pro) for 900+ €)
https://vulkan.gpuinfo.org/displayreport.php?id=34702#queuefamilies

screenshots

image

image

@akien-mga akien-mga merged commit 70963cf into godotengine:master Nov 20, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vulkan: Error output on Android devices A queue family with the requested bits could not be found.
5 participants