layer_chassis_dispatch.cpp attempts to unwrap output handles

[pdaniell-nv]

As discussed in https://gitlab.khronos.org/Tracker/vk-gl-cts/-/issues/3685#note_360445, it appears some of the generated functions in layer_chassis_dispatch.cpp are treating parameters that are meant to output a handle as inputs, and attempts to Unwrap them, when it should be doing WrapNew().

It appears the bug is in

Vulkan-ValidationLayers/scripts/layer_chassis_dispatch_generator.py

Line 2077 in c935ec7

iscreate = True if True in [create_txt in cmdname for create_txt in ['Create', 'Allocate', 'GetRandROutputDisplayEXT', 'RegisterDeviceEvent', 'RegisterDisplayEvent', 'AcquirePerformanceConfigurationINTEL']] else False

where there needs to be an exception for "GetDrmDisplayEXT" and possibly "GetWinrtDisplayNV". I didn't check all functions exhaustively, so there may be others.

edit: CTS

dEQP-VK.wsi.direct_drm.*
dEQP-VK.api.image_compression_control.swapchain.direct_drm.*
dEQP-VK.image.swapchain_mutable.direct_drm.*
dEQP-VK.protected_memory.interaction.wsi.direct_drm.*

[ncesario-lunarg]

Thanks @pdaniell-nv, it seems like some additional annotation in the xml would be beneficial, but I'm not sure how realistic it would be to get something like an in/out/inout attribute added to each command parameter.

FYI @mikes-lunarg as he probably understands the code generation best.

[mikes-lunarg]

Yeah, treating Get*Display* as "create" calls is probably the quickest fix. I'm a little surprised we don't just infer in/out from constness. It would probably be a worthwhile exercise to compare the current iscreate list with a list of all entry points that take a non-const handle pointer.

[ncesario-lunarg]

@mikes-lunarg yes, that makes sense. I tried this out in #4138 and it seems to have the desired effect, but not sure if it's 100% correct (that code is still more mysterious to me than it should be).

[math4origami]

Hi, I'm the one who opened the original issue in CTS.

To resolve this issue, I think we'll need we need to add this around here:
https://github.com/KhronosGroup/Vulkan-ValidationLayers/blob/master/scripts/thread_safety_generator.py#L1344

void ThreadSafety::PostCallRecordGetDrmDisplayEXT(
    VkPhysicalDevice                            physicalDevice,
    int32_t                                     drmFd,
    uint32_t                                    connectorId,
    VkDisplayKHR*                               display,
    VkResult                                    result) {
    if ((result != VK_SUCCESS) || (display == nullptr)) return;
    CreateObjectParentInstance(*display);
}
// (and the corresponding header declaration)

In addition to removing Unwrap() mentioned previously, we also need to add this on the line:
https://github.com/KhronosGroup/Vulkan-ValidationLayers/blob/master/layers/generated/layer_chassis_dispatch.cpp#L9342

VkResult DispatchGetDrmDisplayEXT() {
    if (VK_SUCCESS == result) {
        if (*display) *display = layer_data->MaybeWrapDisplay(*display, layer_data);
    }

I notice that all other uses of MaybeWrapDisplay() are not actually generated, but are manual.
https://github.com/KhronosGroup/Vulkan-ValidationLayers/blob/master/scripts/layer_chassis_dispatch_generator.py#L866
So, I'm not sure if that means we'd need to add new generation, or do it manually and remove the generation.

Something like this was mentioned in #4138, and from my prototyping this outcome would satisfy the tests.

[spencer-lunarg]

So looking at this, the biggest issue is still trying to understand VkDisplayKHR usage pattern

Uses the Display

• vkCreateDisplayModeKHR
• vkRegisterDisplayEventEXT
• vkGetDisplayModePropertiesKHR
• vkGetDisplayModeProperties2KHR
• vkDisplayPowerControlEXT

Controls Display, but doesn't create/destroy it

• vkAcquireXlibDisplayEXT
• vkAcquireWinrtDisplayNV
• vkAcquireDrmDisplayEXT
• vkReleaseDisplayEXT

Creates Display? (not sure)

• vkGetDrmDisplayEXT
• vkGetDisplayPlaneSupportedDisplaysKHR
• vkGetRandROutputDisplayEXT
• vkGetWinrtDisplayNV

@math4origami / @pdaniell-nv can someone help explain for VkDisplayKHR when it is "created"/"destroyed" and what general life cycle there is, from there I can more easily adjust the code to reflect this in the Handle Wrapping logic

[russellcnv]

VkDisplayKHR are static handles, they are created when the driver loads and persist throughout the lifetime of the VkPhysicalDevice.
The main way to enumerate them is:

vkGetPhysicalDeviceDisplayPropertiesKHR(
    VkPhysicalDevice                            physicalDevice,
    uint32_t*                                   pPropertyCount,
    VkDisplayPropertiesKHR*                     pProperties);

The properties struct is:

typedef struct VkDisplayPropertiesKHR {
    VkDisplayKHR                  display;
    const char*                   displayName;
    VkExtent2D                    physicalDimensions;
    VkExtent2D                    physicalResolution;
    VkSurfaceTransformFlagsKHR    supportedTransforms;
    VkBool32                      planeReorderPossible;
    VkBool32                      persistentContent;
} VkDisplayPropertiesKHR;

The VkDisplayKHR here is the handle used in any subsequent API that wants to use the display with these properties.

What functions like vkGetDrmDisplayEXT and vkGetRandROutputDisplayEXT do is, answer the question:
"Among the statically allocated displays that you (the driver) know about, please tell me which one corresponds to this <3rd party identifier>?"

Then, the function returns the handle in the output VkDisplay *pDisplay.

From the user's perspective, they do not actually interact with the lifetime of these objects at all. They just ask for the handle of an existing one, then use it.

[spencer-lunarg]

@russellcnv thanks for this! So what I am concluding is VkDisplayKHR are basically just the same as VkQueue (and vkGetDrmDisplayEXT is like vkGetDeviceQueue) ...of course the difference it not being a dispatch handle

In this case I think the answer is that there should be no Handle Wrapping on the VkDisplayKHR as their life time is not ever control from an app/layer level

Edit: I might be wrong, the handle wrapping is not the issue, it is just making sure it is "created" when we first see it in a call like vkGetDrmDisplayEXT