VK_EXT_external_memory_metal validation #8498

aitor-lunarg · 2024-09-05T11:29:51Z

There are some UNASSIGNED VUIDs that I thought would be useful for the user but I'm not sure under which struct they should be. Any suggestions are more than welcome!

I believe there could be a best effort check on certain VUIDs that are not implemented like The memory from which handle was exported must have been created on the same underlying physical device as device but would require adding support for having Objective-C files in the repository, unsure if there's something like this at the moment (happy to learn if there's any so I can do some testing).

I believe there's a few missing tests for some VUIDs. I'll double check everything tomorrow. Just want to start getting some eyes into this so we can move it forward for extension release. Not looking for approval for merge since that can't happen until the extension is released.

ci-tester-lunarg · 2024-09-05T11:29:55Z

CI Vulkan-ValidationLayers build queued with queue ID 249962.

ci-tester-lunarg · 2024-09-05T11:30:01Z

CI Vulkan-ValidationLayers build # 17383 running.

ci-tester-lunarg · 2024-09-05T11:37:26Z

CI Vulkan-ValidationLayers build # 17383 failed.

spencer-lunarg · 2024-09-05T12:35:57Z

layers/core_checks/cc_external_object.cpp

+                             "VK_EXTERNAL_MEMORY_HANDLE_TYPE_MTLTEXTURE_BIT_EXT requires textures to be imported as a dedicated"
+                             "allocation.");
+        }
+        return skip;


so if there is any VkExportMemoryAllocateInfo we just early return?

I would change the logic to

if (const auto export_memory_info = vku::FindStructInPNextChain<VkExportMemoryAllocateInfo>(allocate_info.pNext)) { // ... code return skip; // explain }

There's 2 potential cases when allocating a VkDeviceMemory object:

Allocate with the intent to export it.

Allocate via import.

The VUIDs checked below this point apply to point 2. While this small segment applies to point 1. This is the main reason for the early return. Although now that I think about it more, there's nothing preventing the user from a 3rd option which would be: Allocate via import with the intent to export it. Kind of redundant, but I'm not sure if there's anything in Vulkan that forbids this (I don't think so, there may be even CTS tests testing this, need to double check).

ok, I guess my only ask is to put some of this github comment information into a code comment, the external memory is already a black-box enough, any extra info is helpful

spencer-lunarg · 2024-09-05T12:37:30Z

layers/core_checks/cc_external_object.cpp

+                                            const ErrorObject& error_obj) const {
+    bool skip = false;
+    const Location get_metal_handle_info = error_obj.location.dot(Field::pGetMetalHandleInfo);
+    auto memory = Get<vvl::DeviceMemory>(pGetMetalHandleInfo->memory);


Suggested change

auto memory = Get<vvl::DeviceMemory>(pGetMetalHandleInfo->memory);

auto memory = Get<vvl::DeviceMemory>(pGetMetalHandleInfo->memory);

ASSERT_AND_RETURN_SKIP(memory)

spencer-lunarg · 2024-09-05T12:38:36Z

layers/core_checks/cc_external_object.cpp

+    if ((pGetMetalHandleInfo->handleType != VK_EXTERNAL_MEMORY_HANDLE_TYPE_MTLBUFFER_BIT_EXT) &&
+        (pGetMetalHandleInfo->handleType != VK_EXTERNAL_MEMORY_HANDLE_TYPE_MTLTEXTURE_BIT_EXT)) {
+        skip |= LogError("VUID-VK_EXT_external_memory-get-handle-incorrect-handle-type", device,
+                            get_metal_handle_info.dot(Struct::VkImportMemoryMetalHandleInfoEXT, Field::handleType),


Suggested change

get_metal_handle_info.dot(Struct::VkImportMemoryMetalHandleInfoEXT, Field::handleType),

get_metal_handle_info.dot(Field::handleType),

spencer-lunarg · 2024-09-05T12:40:44Z

tests/unit/external_memory_metal.cpp

+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ */
+


I think this file needs to wrapped in a #ifdef VK_USE_PLATFORM_METAL_EXT

ci-tester-lunarg · 2024-09-09T05:32:36Z

CI Vulkan-ValidationLayers build queued with queue ID 252306.

ci-tester-lunarg · 2024-09-09T05:35:01Z

CI Vulkan-ValidationLayers build # 17412 running.

aitor-lunarg · 2024-09-09T05:35:22Z

layers/core_checks/cc_external_object.cpp

+    // Case 1 and partially 3
+    if (const auto export_memory_info = vku::FindStructInPNextChain<VkExportMemoryAllocateInfo>(allocate_info.pNext)) {
+        if ((export_memory_info->handleTypes == VK_EXTERNAL_MEMORY_HANDLE_TYPE_MTLTEXTURE_BIT_EXT) && dedicated_allocation_info == nullptr) {
+            skip |= LogError("VUID-UNASSIGNED-VK_EXT_external_memory-no-VkMemoryDedicatedAllocateInfo-export", device,


@spencer-lunarg thoughts on where this VUID should go? Part of VkExportMemoryAllocateInfo or VkMemoryDedicatedAllocateInfo? I'm inclined towards the former since it also requires VkExportMemoryAllocateInfo::handleTypes to be VK_EXTERNAL_MEMORY_HANDLE_TYPE_MTLTEXTURE_BIT_EXT, but I'm not sure how cases where a struct is required by another struct are handled in the spec.

I would say VkExportMemoryAllocateInfo

Also not there are things such as VK_EXTERNAL_MEMORY_FEATURE_DEDICATED_ONLY_BIT, there are some extra VUs for some handleTypes that do requires VkMemoryDedicatedAllocateInfo, but its not for all to my knowledge

aitor-lunarg · 2024-09-09T05:35:59Z

layers/core_checks/cc_external_object.cpp

+    // operations on buffers.
+    if(import_memory_metal_info->handleType == VK_EXTERNAL_MEMORY_HANDLE_TYPE_MTLTEXTURE_BIT_EXT) {
+        if (dedicated_allocation_info == nullptr) {
+            skip |= LogError("VUID-UNASSIGNED-VK_EXT_external_memory-no-VkMemoryDedicatedAllocateInfo-import", device,


@spencer-lunarg similar situation as the previous one

aitor-lunarg · 2024-09-09T05:36:23Z

layers/core_checks/cc_external_object.cpp

+
+        // Unsure if there should be a VUID that enforces image to not be NULL when importing a MTLTEXTURE type
+        if (dedicated_allocation_info->image == VK_NULL_HANDLE) {
+            skip |= LogError("VUID-UNASSIGNED-VK_EXT_external_memory-dedicated-null-image-import", device,


Same as above 2

aitor-lunarg · 2024-09-09T05:38:14Z

layers/core_checks/cc_external_object.cpp

+        DispatchGetPhysicalDeviceFormatProperties2(physical_device, image_format, &format_properties);
+
+        if ((external_image_format_properties.externalMemoryProperties.externalMemoryFeatures & VK_EXTERNAL_MEMORY_FEATURE_IMPORTABLE_BIT) == 0u) {
+            skip |= LogError("VUID-UNASSIGNED-VK_EXT_external_memory-unsupported-format-import", device,


aitor-lunarg · 2024-09-09T05:38:58Z

tests/unit/external_memory_metal.cpp

+    m_errorMonitor->VerifyFound();
+}
+
+TEST_F(NegativeExternalMemoryMetal, GetResourceHandleWithoutExportStructAtCreation) {


New tests I forgot to add in the initial PR start here

ci-tester-lunarg · 2024-09-09T07:21:23Z

CI Vulkan-ValidationLayers build # 17412 failed.

ci-tester-lunarg · 2024-09-25T09:36:02Z

CI Vulkan-ValidationLayers build queued with queue ID 263839.

ci-tester-lunarg · 2024-09-25T09:40:01Z

CI Vulkan-ValidationLayers build # 17585 running.

ci-tester-lunarg · 2024-09-25T09:57:51Z

CI Vulkan-ValidationLayers build # 17585 failed.

aitor-lunarg requested a review from a team as a code owner September 5, 2024 11:29

spencer-lunarg reviewed Sep 5, 2024

View reviewed changes

aitor-lunarg added 2 commits September 9, 2024 14:32

layers: Add VK_EXT_external_memory_metal

9c483c4

tests: Add VK_EXT_external_memory_metal tests

62812ec

aitor-lunarg force-pushed the VK_EXT_external_memory_metal branch from 52e8328 to 62812ec Compare September 9, 2024 05:32

aitor-lunarg commented Sep 9, 2024

View reviewed changes

aitor-lunarg added 2 commits September 25, 2024 18:34

layers: remove UNASSIGNED VUIDs for new added VUIDs

042cb3d

tests: Replace UNASSIGNED VUIDs for new ones

76c44a4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VK_EXT_external_memory_metal validation #8498

VK_EXT_external_memory_metal validation #8498

aitor-lunarg commented Sep 5, 2024

ci-tester-lunarg commented Sep 5, 2024

ci-tester-lunarg commented Sep 5, 2024

ci-tester-lunarg commented Sep 5, 2024

spencer-lunarg Sep 5, 2024

aitor-lunarg Sep 5, 2024 •

edited

Loading

spencer-lunarg Sep 5, 2024

aitor-lunarg Sep 9, 2024

spencer-lunarg Sep 5, 2024

aitor-lunarg Sep 9, 2024

spencer-lunarg Sep 5, 2024

aitor-lunarg Sep 9, 2024

spencer-lunarg Sep 5, 2024

aitor-lunarg Sep 9, 2024

ci-tester-lunarg commented Sep 9, 2024

ci-tester-lunarg commented Sep 9, 2024

aitor-lunarg Sep 9, 2024

spencer-lunarg Sep 9, 2024

aitor-lunarg Sep 9, 2024

aitor-lunarg Sep 9, 2024

aitor-lunarg Sep 9, 2024

aitor-lunarg Sep 9, 2024

ci-tester-lunarg commented Sep 9, 2024

ci-tester-lunarg commented Sep 25, 2024

ci-tester-lunarg commented Sep 25, 2024

ci-tester-lunarg commented Sep 25, 2024

	auto memory = Get<vvl::DeviceMemory>(pGetMetalHandleInfo->memory);
	auto memory = Get<vvl::DeviceMemory>(pGetMetalHandleInfo->memory);
	ASSERT_AND_RETURN_SKIP(memory)

	get_metal_handle_info.dot(Struct::VkImportMemoryMetalHandleInfoEXT, Field::handleType),
	get_metal_handle_info.dot(Field::handleType),

VK_EXT_external_memory_metal validation #8498

Are you sure you want to change the base?

VK_EXT_external_memory_metal validation #8498

Conversation

aitor-lunarg commented Sep 5, 2024

ci-tester-lunarg commented Sep 5, 2024

ci-tester-lunarg commented Sep 5, 2024

ci-tester-lunarg commented Sep 5, 2024

Choose a reason for hiding this comment

aitor-lunarg Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ci-tester-lunarg commented Sep 9, 2024

ci-tester-lunarg commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ci-tester-lunarg commented Sep 9, 2024

ci-tester-lunarg commented Sep 25, 2024

ci-tester-lunarg commented Sep 25, 2024

ci-tester-lunarg commented Sep 25, 2024

aitor-lunarg Sep 5, 2024 •

edited

Loading