[PATCH v2 1/2] winevulkan: Manually fixup struct alignment for VkPipelineCreationFeedback.
We can't reasonably auto generate this because it's output in an otherwise input pNext chain. Signed-off-by: Georg Lehmann <dadschoorse(a)gmail.com> --- dlls/winevulkan/make_vulkan | 8 +++ dlls/winevulkan/vulkan.c | 110 ++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+) diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan index c609bbfd151..2f3d647ef9b 100755 --- a/dlls/winevulkan/make_vulkan +++ b/dlls/winevulkan/make_vulkan @@ -205,6 +205,8 @@ FUNCTION_OVERRIDES = { # Device functions "vkAllocateCommandBuffers" : {"dispatch" : True, "driver" : False, "thunk" : ThunkType.NONE}, "vkCreateCommandPool" : {"dispatch": True, "driver" : False, "thunk" : ThunkType.NONE}, + "vkCreateComputePipelines" : {"dispatch": True, "driver" : False, "thunk" : ThunkType.PRIVATE}, + "vkCreateGraphicsPipelines" : {"dispatch": True, "driver" : False, "thunk" : ThunkType.PRIVATE}, "vkDestroyCommandPool" : {"dispatch": True, "driver" : False, "thunk" : ThunkType.NONE}, "vkDestroyDevice" : {"dispatch" : True, "driver" : False, "thunk" : ThunkType.NONE}, "vkFreeCommandBuffers" : {"dispatch" : True, "driver" : False, "thunk" : ThunkType.NONE}, @@ -250,6 +252,9 @@ FUNCTION_OVERRIDES = { "vkGetDeviceGroupSurfacePresentModesKHR" : {"dispatch" : True, "driver" : True, "thunk" : ThunkType.PUBLIC}, "vkGetPhysicalDevicePresentRectanglesKHR" : {"dispatch" : True, "driver" : True, "thunk" : ThunkType.PUBLIC}, + # VK_KHR_ray_tracing_pipeline + "vkCreateRayTracingPipelinesKHR" : {"dispatch": True, "driver" : False, "thunk" : ThunkType.PRIVATE}, + # VK_EXT_calibrated_timestamps "vkGetPhysicalDeviceCalibrateableTimeDomainsEXT" : {"dispatch" : True, "driver" : False, "thunk" : ThunkType.NONE}, "vkGetCalibratedTimestampsEXT" : {"dispatch" : True, "driver" : False, "thunk" : ThunkType.NONE}, @@ -261,6 +266,9 @@ FUNCTION_OVERRIDES = { # VK_EXT_debug_report "vkCreateDebugReportCallbackEXT" : {"dispatch": True, "driver" : False, "thunk" : ThunkType.NONE}, "vkDestroyDebugReportCallbackEXT" : {"dispatch": True, "driver" : False, "thunk" : ThunkType.NONE}, + + # VK_NV_ray_tracing + "vkCreateRayTracingPipelinesNV" : {"dispatch": True, "driver" : False, "thunk" : ThunkType.PRIVATE}, } STRUCT_CHAIN_CONVERSIONS = { diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index 19cc999433b..f5d1a1f4fc2 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -1758,6 +1758,116 @@ NTSTATUS wine_vkDestroyDebugReportCallbackEXT(void *args) return STATUS_SUCCESS; } +static void fixup_pipeline_feedback(VkPipelineCreationFeedback *feedback, uint32_t count) +{ +#if defined(USE_STRUCT_CONVERSION) + struct host_pipeline_feedback + { + VkPipelineCreationFeedbackFlags flags; + uint64_t duration; + } *host_feedback; + int64_t i; + + host_feedback = (void *) feedback; + + for (i = count - 1; i >= 0; i--) + { + memmove(&feedback[i].duration, &host_feedback[i].duration, sizeof(uint64_t)); + feedback[i].flags = host_feedback[i].flags; + } +#else + (void)feedback; + (void)count; +#endif +} + +static void fixup_pipeline_feedback_info(const void *pipeline_info) +{ + VkPipelineCreationFeedbackCreateInfo *feedback; + + feedback = wine_vk_find_struct(pipeline_info, PIPELINE_CREATION_FEEDBACK_CREATE_INFO); + + if (!feedback) + return; + + fixup_pipeline_feedback(feedback->pPipelineCreationFeedback, 1); + fixup_pipeline_feedback(feedback->pPipelineStageCreationFeedbacks, + feedback->pipelineStageCreationFeedbackCount); +} + +NTSTATUS wine_vkCreateComputePipelines(void *args) +{ + struct vkCreateComputePipelines_params *params = args; + VkResult res; + uint32_t i; + + TRACE("%p, 0x%s, %u, %p, %p, %p\n", params->device, wine_dbgstr_longlong(params->pipelineCache), + params->createInfoCount, params->pCreateInfos, params->pAllocator, params->pPipelines); + + res = thunk_vkCreateComputePipelines(params->device, params->pipelineCache, + params->createInfoCount, params->pCreateInfos, params->pAllocator, params->pPipelines); + + for (i = 0; i < params->createInfoCount; i++) + fixup_pipeline_feedback_info(¶ms->pCreateInfos[i]); + + return res; +} + +NTSTATUS wine_vkCreateGraphicsPipelines(void *args) +{ + struct vkCreateGraphicsPipelines_params *params = args; + VkResult res; + uint32_t i; + + TRACE("%p, 0x%s, %u, %p, %p, %p\n", params->device, wine_dbgstr_longlong(params->pipelineCache), + params->createInfoCount, params->pCreateInfos, params->pAllocator, params->pPipelines); + + res = thunk_vkCreateGraphicsPipelines(params->device, params->pipelineCache, + params->createInfoCount, params->pCreateInfos, params->pAllocator, params->pPipelines); + + for (i = 0; i < params->createInfoCount; i++) + fixup_pipeline_feedback_info(¶ms->pCreateInfos[i]); + + return res; +} + +NTSTATUS wine_vkCreateRayTracingPipelinesKHR(void *args) +{ + struct vkCreateRayTracingPipelinesKHR_params *params = args; + VkResult res; + uint32_t i; + + TRACE("%p, 0x%s, 0x%s, %u, %p, %p, %p\n", params->device, + wine_dbgstr_longlong(params->deferredOperation), wine_dbgstr_longlong(params->pipelineCache), + params->createInfoCount, params->pCreateInfos, params->pAllocator, params->pPipelines); + + res = thunk_vkCreateRayTracingPipelinesKHR(params->device, params->deferredOperation, params->pipelineCache, + params->createInfoCount, params->pCreateInfos, params->pAllocator, params->pPipelines); + + for (i = 0; i < params->createInfoCount; i++) + fixup_pipeline_feedback_info(¶ms->pCreateInfos[i]); + + return res; +} + +NTSTATUS wine_vkCreateRayTracingPipelinesNV(void *args) +{ + struct vkCreateRayTracingPipelinesNV_params *params = args; + VkResult res; + uint32_t i; + + TRACE("%p, 0x%s, %u, %p, %p, %p\n", params->device, wine_dbgstr_longlong(params->pipelineCache), + params->createInfoCount, params->pCreateInfos, params->pAllocator, params->pPipelines); + + res = thunk_vkCreateRayTracingPipelinesNV(params->device, params->pipelineCache, + params->createInfoCount, params->pCreateInfos, params->pAllocator, params->pPipelines); + + for (i = 0; i < params->createInfoCount; i++) + fixup_pipeline_feedback_info(¶ms->pCreateInfos[i]); + + return res; +} + BOOL WINAPI wine_vk_is_available_instance_function(VkInstance instance, const char *name) { return !!vk_funcs->p_vkGetInstanceProcAddr(instance->instance, name); -- 2.35.1
We did previously not support this extension because it runs into 32bit alignment issues but those are now fixed. Signed-off-by: Georg Lehmann <dadschoorse(a)gmail.com> --- dlls/winevulkan/make_vulkan | 1 - 1 file changed, 1 deletion(-) diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan index 2f3d647ef9b..ca5cf94af55 100755 --- a/dlls/winevulkan/make_vulkan +++ b/dlls/winevulkan/make_vulkan @@ -96,7 +96,6 @@ UNSUPPORTED_EXTENSIONS = [ "VK_AMD_display_native_hdr", "VK_EXT_full_screen_exclusive", "VK_EXT_hdr_metadata", # Needs WSI work. - "VK_EXT_pipeline_creation_feedback", "VK_GOOGLE_display_timing", "VK_KHR_external_fence_win32", "VK_KHR_external_semaphore_win32", -- 2.35.1
On 2/9/22 10:39, Georg Lehmann wrote:
We can't reasonably auto generate this because it's output in an otherwise input pNext chain.
Aren't we going to need to do that anyway, though, for many other functions, for wow64 support? It seems like it'd be more worthwhile to add infrastructure for automatically generating this, or at least some of it.
+static void fixup_pipeline_feedback(VkPipelineCreationFeedback *feedback, uint32_t count) +{ +#if defined(USE_STRUCT_CONVERSION) + struct host_pipeline_feedback + { + VkPipelineCreationFeedbackFlags flags; + uint64_t duration; + } *host_feedback; + int64_t i; + + host_feedback = (void *) feedback; + + for (i = count - 1; i >= 0; i--) + { + memmove(&feedback[i].duration, &host_feedback[i].duration, sizeof(uint64_t));
Do we need memmove() here?
+ feedback[i].flags = host_feedback[i].flags; + } +#else + (void)feedback; + (void)count;
This is unnecessary; we don't use -Wunused-parameter.
On 09.02.22 18:07, Zebediah Figura wrote:
On 2/9/22 10:39, Georg Lehmann wrote:
We can't reasonably auto generate this because it's output in an otherwise input pNext chain.
Aren't we going to need to do that anyway, though, for many other functions, for wow64 support?
It seems like it'd be more worthwhile to add infrastructure for automatically generating this, or at least some of it.
As far as I can tell this is the only pNext chain like this. We don't handle pure output chains, that's something that needs to be automated for wow64. But this issue is an unique edge case.
+static void fixup_pipeline_feedback(VkPipelineCreationFeedback *feedback, uint32_t count) +{ +#if defined(USE_STRUCT_CONVERSION) + struct host_pipeline_feedback + { + VkPipelineCreationFeedbackFlags flags; + uint64_t duration; + } *host_feedback; + int64_t i; + + host_feedback = (void *) feedback; + + for (i = count - 1; i >= 0; i--) + { + memmove(&feedback[i].duration, &host_feedback[i].duration, sizeof(uint64_t));
Do we need memmove() here?
These partially overlap so I think memmove is only way to do it safely.
+ feedback[i].flags = host_feedback[i].flags; + } +#else + (void)feedback; + (void)count;
This is unnecessary; we don't use -Wunused-parameter.
Personally I like being explicit here, but if you want I can send a new version with these removed.
On 2/9/22 11:20, Georg Lehmann wrote:
On 09.02.22 18:07, Zebediah Figura wrote:
On 2/9/22 10:39, Georg Lehmann wrote:
We can't reasonably auto generate this because it's output in an otherwise input pNext chain.
Aren't we going to need to do that anyway, though, for many other functions, for wow64 support?
It seems like it'd be more worthwhile to add infrastructure for automatically generating this, or at least some of it.
As far as I can tell this is the only pNext chain like this. We don't handle pure output chains, that's something that needs to be automated for wow64. But this issue is an unique edge case.
I'll admit this doesn't make me feel comfortable, not when Vulkan is a fast developing API that we are making an effort to keep up with. I.e. even if there aren't any others now it seems not unlikely we'll come across more of these in the future. And if we can find a solution that allows handling partial and wholly output chains in one breath, that seems nice... It seems like most such structs are marked "returnedonly"; can we use that to determine what needs output conversion? It's missing from VkPipelineCreationFeedbackCreateInfo, but maybe that's an error.
+static void fixup_pipeline_feedback(VkPipelineCreationFeedback *feedback, uint32_t count) +{ +#if defined(USE_STRUCT_CONVERSION) + struct host_pipeline_feedback + { + VkPipelineCreationFeedbackFlags flags; + uint64_t duration; + } *host_feedback; + int64_t i; + + host_feedback = (void *) feedback; + + for (i = count - 1; i >= 0; i--) + { + memmove(&feedback[i].duration, &host_feedback[i].duration, sizeof(uint64_t));
Do we need memmove() here?
These partially overlap so I think memmove is only way to do it safely.
Right, of course.
+ feedback[i].flags = host_feedback[i].flags; + } +#else + (void)feedback; + (void)count;
This is unnecessary; we don't use -Wunused-parameter.
Personally I like being explicit here, but if you want I can send a new version with these removed.
We don't do this elsewhere in Wine, for whatever that's worth. (Nor do I think it's worthwhile, given how many unused parameters there are to Windows functions.) But regardless winevulkan isn't code I work on, so I don't really intend to prescribe coding style for it.
On 15.02.22 19:49, Zebediah Figura wrote:
On 2/9/22 11:20, Georg Lehmann wrote:
On 09.02.22 18:07, Zebediah Figura wrote:
On 2/9/22 10:39, Georg Lehmann wrote:
We can't reasonably auto generate this because it's output in an otherwise input pNext chain.
Aren't we going to need to do that anyway, though, for many other functions, for wow64 support?
It seems like it'd be more worthwhile to add infrastructure for automatically generating this, or at least some of it.
As far as I can tell this is the only pNext chain like this. We don't handle pure output chains, that's something that needs to be automated for wow64. But this issue is an unique edge case.
I'll admit this doesn't make me feel comfortable, not when Vulkan is a fast developing API that we are making an effort to keep up with. I.e. even if there aren't any others now it seems not unlikely we'll come across more of these in the future. And if we can find a solution that allows handling partial and wholly output chains in one breath, that seems nice...
It seems like most such structs are marked "returnedonly"; can we use that to determine what needs output conversion? It's missing from VkPipelineCreationFeedbackCreateInfo, but maybe that's an error.
It's not quite as simple because some structs are input in one chain but output in another. Like all the feature structs, which you use to query the available features but also to enable them on device creation. Generally everything in a const void* pNext chain is input and everything in a void* pNext chain is output, but VkPipelineCreationFeedbackCreateInfo is the exception to that rule.
+static void fixup_pipeline_feedback(VkPipelineCreationFeedback *feedback, uint32_t count) +{ +#if defined(USE_STRUCT_CONVERSION) + struct host_pipeline_feedback + { + VkPipelineCreationFeedbackFlags flags; + uint64_t duration; + } *host_feedback; + int64_t i; + + host_feedback = (void *) feedback; + + for (i = count - 1; i >= 0; i--) + { + memmove(&feedback[i].duration, &host_feedback[i].duration, sizeof(uint64_t));
Do we need memmove() here?
These partially overlap so I think memmove is only way to do it safely.
Right, of course.
+ feedback[i].flags = host_feedback[i].flags; + } +#else + (void)feedback; + (void)count;
This is unnecessary; we don't use -Wunused-parameter.
Personally I like being explicit here, but if you want I can send a new version with these removed.
We don't do this elsewhere in Wine, for whatever that's worth. (Nor do I think it's worthwhile, given how many unused parameters there are to Windows functions.) But regardless winevulkan isn't code I work on, so I don't really intend to prescribe coding style for it.
On 2/15/22 14:13, Georg Lehmann wrote:
On 15.02.22 19:49, Zebediah Figura wrote:
On 2/9/22 11:20, Georg Lehmann wrote:
On 09.02.22 18:07, Zebediah Figura wrote:
On 2/9/22 10:39, Georg Lehmann wrote:
We can't reasonably auto generate this because it's output in an otherwise input pNext chain.
Aren't we going to need to do that anyway, though, for many other functions, for wow64 support?
It seems like it'd be more worthwhile to add infrastructure for automatically generating this, or at least some of it.
As far as I can tell this is the only pNext chain like this. We don't handle pure output chains, that's something that needs to be automated for wow64. But this issue is an unique edge case.
I'll admit this doesn't make me feel comfortable, not when Vulkan is a fast developing API that we are making an effort to keep up with. I.e. even if there aren't any others now it seems not unlikely we'll come across more of these in the future. And if we can find a solution that allows handling partial and wholly output chains in one breath, that seems nice...
It seems like most such structs are marked "returnedonly"; can we use that to determine what needs output conversion? It's missing from VkPipelineCreationFeedbackCreateInfo, but maybe that's an error.
It's not quite as simple because some structs are input in one chain but output in another. Like all the feature structs, which you use to query the available features but also to enable them on device creation. Generally everything in a const void* pNext chain is input and everything in a void* pNext chain is output, but VkPipelineCreationFeedbackCreateInfo is the exception to that rule.
I see. Do you think this is the sort of rich information we could introduce into the official XML? It seems potentially useful for other purposes than ours. E.g. other translation layers, such as language bindings, could potentially make use of it. Granted, if it's only the one structure for now, and we can reliably use the "const" pointer, maybe it's not worthwhile, but I also don't want to get caught off guard again.
participants (2)
-
Georg Lehmann -
Zebediah Figura