extensions/ext: Add VK_EXT_image_drm_format_modifier

[i509VCB]

No description provided.

[MarijnS95]

When resubmitting this, can you also retitle the commit to extensions/ext: Add VK_EXT_image_drm_format_modifier?

[i509VCB]

boop

[i509VCB]

@MarijnS95 soft reminder that I have resolved the review comments.

[MarijnS95]

Regardless of previous review points, ash so far has been completely unopinionated in terms of not providing any helper functions, and get_drm_format_properties_list appears to be the first to break that "contract".

In addition the function signature and implementation make it so that I don't see any real advantage over having it, in favour of letting users implement it directly themselves.

@Ralith What's your though on this? My feeling is that we can probably merge this PR straight away without it, and I'm pretty opposed to having it.

[Ralith]

I agree that it would be better to omit it. I don't know that it's a significant usability benefit, and regardless of its merits, it's inconsistent with the rest of ash.

[i509VCB]

The primary reason for the helper is very much for convince. Since the return value is effectively an array of variable size, getting a Vec makes sense.

The readme does agree with you two regarding the opinionated function.

Note: Functions don't return Vec if this would limit the functionality. See p_next.

Since get_drm_format_properties_list requires extension via the p_next field and may in fact be influenced by other p_next members, it is quite hard to provide that utility in a way that isn't opinionated.

There is some precedence for helpers like those in PhysicalDeviceDrm for getting the physical device property but that does not use the same commands as this case and does not need two implementations either.

[i509VCB]

A further question, if the user is expected to implement that part of the api since it is opinionated, should the helper functions in ash for things such as uninitialized vectors be publicly available?

[MarijnS95]

The readme does agree with you two regarding the opinionated function.

Note: Functions don't return Vec if this would limit the functionality. See p_next.

Since get_drm_format_properties_list requires extension via the p_next field and may in fact be influenced by other p_next members, it is quite hard to provide that utility in a way that isn't opinionated.

This note specifically applies to the return type; vk::DrmFormatModifierPropertiesEXT does not have a p_next field and is as such fine to be returned in a Vec rather than through a mutable slice that is to be filled by the called like the vk::FormatProperties2 bit that you subsequently have to modify. That is by the way the part which makes this weird, because it's mainly mutable to let them influence the p_next chain in which you insert another object, and perhaps retrieve the fields in here at the same time.

Opinionated here can be interpreted in two ways:

1. The choice to not provide any helpers that do not 1:1 map to a single Vulkan function and/or restrict functionality of that function;
2. Helpers that have a specific or peculiar way to be used, the way you described.

There is some precedence for helpers like those in PhysicalDeviceDrm for getting the physical device property but that does not use the same commands as this case and does not need two implementations either.

This is a very simple helper that was apparently introduced in 0ed0a06 and I've adopted it in the KHR Ray Tracing and later PhysicalDeviceDrm extension without thinking much of it. In hindsight I'd rather get rid of it since it's probably more efficient to just chain all necessary properties into a single vkGetPhysicalDeviceProperties2() call at once.

A further question, if the user is expected to implement that part of the api since it is opinionated, should the helper functions in ash for things such as uninitialized vectors be publicly available?

Not sure, it's mostly been an internal helper for now. At the same time it doesn't really fit these functions. It's great that it automagically calls the function twice, once with a nullptr to acquire the length, but this particular use of get_physical_device_format_properties2() doesn't return errors that require a retry (what I originally added it for) nor have the pointer/count in a place that makes it convenient to use.

[i509VCB]

I have removed get_drm_format_properties_list then. It's not a whole lot to implement getting the list yourself. The use cases for that function being in ash is arguably quite niche (Wayland compositors, implementing WSI, KMS instead of using VkDisplayKHR).