Add VK_KHR_display extension support

[ehegnes]

Quick attempt at adding direct-to-display rendering support—heavily based on the already provided extensions.

• Other extensions allowed dead code. I wasn't sure if that's necessary here.
• Display presumably does not clash with any other structs in the codebase?

[ehegnes]

Because there are too many source annotations above...

I'm happy with the proposal to use std::mem::MaybeUninit. I was using std::mem::zeroed to be consistent with usages throughout this repository. Perhaps refactoring other instances of std::mem::zeroed is best suited for a separate issue/PR?

Regarding return value ignorance: this occurs only when setting a count. Presumably this is only an issue if the initial Vulkan call fails to set the count, and the subsequent Vulkan call succeeds. Vec::with_capacity(0) would have run prior, which, according to std::vec::Vec#Guarantees, means that Vec will not have allocated memory.

Let's take a close look at vkGetPhysicalDeviceDisplayPropertiesKHR(3).

Description
If pProperties is NULL, then the number of display devices available for physicalDevice is returned in pPropertyCount. Otherwise, pPropertyCount must point to a variable set by the user to the number of elements in the pProperties array, and on return the variable is overwritten with the number of structures actually written to pProperties. If the value of pPropertyCount is less than the number of display devices for physicalDevice, at most pPropertyCount structures will be written. If pPropertyCount is smaller than the number of display devices available for physicalDevice, VK_INCOMPLETE will be returned instead of VK_SUCCESS to indicate that not all the available values were returned.

Given a pPropertyCount of 0 and non-null pProperties, the second Vulkan call returns with one of the following. Either

1. we actually have no display devices (or their resources are unavailable), and end up with an empty Vec, or
2. the initial Vulkan call fails to set the count, the subsequent Vulkan call finds one or more display devices, and we get VK_INCOMPLETE.

In both cases, we succeed as expected, or fail (as expected) if either call fails. This invalidates our original concern.

It is worth noting that KhronosGroup/Vulkan-Tools checks such functions twice. Additionally, the initial Vulkan call that sets the count could error with something other than VK_INCOMPLETE.

[Ralith]

I'm on board with using MaybeUninit in new code, and in eventually updating old code with it. The two-call idiom use in ash is not as tidy as it could be (we should probably eventually have a helper function for this, like I do in openxrs) but the pattern here, which is equivalent to that used elsewhere in ash already, does seem sound.

[aloucks]

Note that MaybeUninit::zeroed().assume_init() is the same as mem::zeroed(). There's nothing safer with the two-step verbose form in this context. The all zero bit pattern is valid for these FFI structs.

[Ralith]

I don't see any calls of assume_init prior to the data being written by the Vulkan impl. uninit could be used here instead, to take more advantage of the safety, but this pattern is more idiomatic in any case.

…

On Tue, Oct 29, 2019, 16:32 aloucks ***@***.***> wrote: Note that MaybeUninit::zeroed().assume_init() is the same as mem::zeroed(). There's nothing safer with the two-step verbose form in this context. The all zero bit pattern is valid for these FFI structs. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#247>, or unsubscribe </~https://github.com/notifications/unsubscribe-auth/AAAZQ3WSMBRXDEX3OMXATELQRDBXRANCNFSM4JGAJPSQ> .

[ehegnes]

@aloucks agreed regarding safety parity.

@Ralith uninit or zeroed—I'm not familiar enough with the idioms to make a statement one way or the other.

Is there anything else we'd like to change here?

[Ralith]

Seems reasonable to me.

[MaikKlein]

Looks good, thanks for the PR