search

KhronosGroup / Vulkan-Loader

Vulkan Loader
https://vulkan.lunarg.com/doc/sdk/latest/linux/LoaderInterfaceArchitecture.html
Other
513 stars 279 forks source link

dead fields in VkIcdSurface #1533

Closed jjulianoatnv closed 1 month ago

jjulianoatnv commented 2 months ago

There are fields at the end of struct VkIcdSurface that appear to be write-once, never consumed. Are these dead fields that used to be meaningful, but have now become dead code?

Excerpt from https://github.com/KhronosGroup/Vulkan-Loader/blob/83cd92c725987857a84c871bd8bc11b249d239b1/loader/wsi.h#L65

    uint32_t base_size;            // Size of VkIcdSurfaceBase
    uint32_t platform_size;        // Size of corresponding VkIcdSurfaceXXX
    uint32_t non_platform_offset;  // Start offset to base_size
    uint32_t entire_size;          // Size of entire VkIcdSurface
charles-lunarg commented 2 months ago

Looking at the code, I'm definitely inclined to agree that these fields aren't necessary anymore. On-top of that it appears the entire struct save for the 'surface_index' field is unused - but I do not know if the VkIcdSurfaceFoo structs are used internally by drivers. I don't know how they could, as the VkSurfaceKHR that the driver see's is always it's own handle because the loader unwraps the VkSurfaceKHR before calling down, and stores the VkSurfaceKHR the driver returns when the surface is created.

charles-lunarg commented 2 months ago

I have a PR up which removes all of that code. It may be a prudent idea to coordinate with the SI working group in case there is a reason for their existence.

jjulianoatnv commented 2 months ago

Looking at the code, I'm definitely inclined to agree that these fields aren't necessary anymore. On-top of that it appears the entire struct save for the 'surface_index' field is unused - but I do not know if the VkIcdSurfaceFoo structs are used internally by drivers. I don't know how they could, as the VkSurfaceKHR that the driver see's is always it's own handle because the loader unwraps the VkSurfaceKHR before calling down, and stores the VkSurfaceKHR the driver returns when the surface is created.

I agree with this analysis.

Let's add this issue to the agenda for upcoming Vulkan SI TSG meeting. I won't be at today's meeting, but it's OK if you represent this issue in my absence. Or I'll do it at a subsequent meeting.

jjulianoatnv commented 2 months ago

I did an analysis comparing the loader's behavior for this instance-level object against the other three instance level objects. The analysis is in this PR comment: https://github.com/KhronosGroup/Vulkan-Loader/pull/1534#issuecomment-2288544707

My conclusion is that the changes proposed in the PR make sense.