Semaphore types bug fixes revised #1822

joshqti · 2023-09-28T21:57:21Z

This PR fixes a number of issues, all of which we need to fix to fully pass.

Detect semaphore handle types that are supported by both OpenCL and Vulkan implementations, using API queries. Test all of the detected handle types.
Update the clExternalSemaphore class to handle signaling/waiting for SYNC_FD semaphore handle type.
Delete obsolete semaphore test for double signal/wait. As per the current version of the spec, this behavior is undefined and cannot be tested.
Delete inapplicable clCreateImageWithProperties test cases not related to the semaphore or external memory extensions.
Fix error handling. The multi-import tests, and possibly others did not fail when the test result was incorrect
Delete dead code in the multi-import external memory tests related to "offset". clExternalMemory had an unused parameter called "offset", deleted all test code directly related to that parameter. Offset is no longer part of the spec.

Joshua Kelly, Qualcomm

nikhiljnv

Some early comments.

nikhiljnv · 2023-11-10T04:25:07Z

test_conformance/vulkan/test_vulkan_interop_buffer.cpp

+                "Device %u does not support cl_khr_external_memory_opaque_fd, "
+                "which is required for this test. Skipping\n",
+                device_no);
+            continue;


What if cl_khr_external_memory_opaque_fd is not present, but some other handle type extension is present (e.g. cl_khr_external_memory_win32)? Consider either removing the check or adding checks for other handle types too.

nikhiljnv · 2023-11-10T04:28:11Z

test_conformance/vulkan/test_vulkan_interop_image.cpp

+                "Device %u does not support cl_khr_external_memory_opaque_fd, "
+                "which is required for this test. Skipping\n",
+                device_no);
+            continue;


What if cl_khr_external_memory_opaque_fd is not present, but some other handle type extension is present (e.g. cl_khr_external_memory_win32)? Consider either removing the check or adding checks for other handle types too.

nikhiljnv

Some more comments.

nikhiljnv · 2023-11-10T04:35:47Z

test_conformance/common/vulkan_wrapper/vulkan_utility.cpp

+        all_known_handle_types.push_back(
+            { "VK_KHR_external_semaphore_win32",
+              VK_EXTERNAL_SEMAPHORE_HANDLE_TYPE_OPAQUE_WIN32_BIT_KHR,
+              VULKAN_EXTERNAL_SEMAPHORE_HANDLE_TYPE_OPAQUE_WIN32_NT });


Is there any reason why we push opaque_fd, sync_fd and win32 KMT unconditionally, but need to push win32 NT conditionally? If we are anyway going to check against supported handle types later and this is merely a reference for all known handle types, don't see a need to check for Windows version condition here.

The original structure of the code used conditional compilation, I tried to maintain the original structure, as I do not understand or have the ability to test windows handles. For example, we still have this code in the test from the original PR:

`
const std::vector
getSupportedVulkanExternalMemoryHandleTypeList()
{
std::vector externalMemoryHandleTypeList;

#if _WIN32
if (IsWindows8OrGreater())
{
externalMemoryHandleTypeList.push_back(
VULKAN_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_WIN32_NT);
}
externalMemoryHandleTypeList.push_back(
VULKAN_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_WIN32_KMT);
#else
externalMemoryHandleTypeList.push_back(
VULKAN_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_FD);
#endif

return externalMemoryHandleTypeList;

}
`

I was unclear on how to query Vulkan for the appropriate handle type. For example, the NT enum is a bitwise or of Vulkan flags:

VULKAN_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_WIN32_NT_KMT = VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_WIN32_BIT_KHR | VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_WIN32_KMT_BIT_KHR

I am not sure how to interpret this, in the context of querying support from Vulkan for semaphore or memory handles.

Can you advise?

Since the test was iterating over all handle types pushed here, it made sense to push them conditionally here earlier so that we don't accidentally test for unsupported handle type based on OS version. Now that we anyway cross check this against supported handle types and use this just as reference of all known handle types, I think we can clean this up and push the types unconditionally.

Should I enable OPAQUE_WIN32_KMT when either VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_WIN32_BIT_KHR | VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_WIN32_KMT_BIT_KHR are supported? That enum is a bit hard to understand, as it is defined as a bitwise or of two Vulkan types

nikhiljnv · 2023-11-10T04:44:21Z

test_conformance/vulkan/test_vulkan_interop_image.cpp

@@ -1299,38 +1313,29 @@ int test_image_common(cl_device_id device_, cl_context context_,
    cl_kernel kernel_unsigned[num_kernels] = { NULL, NULL, NULL, NULL };


Since you are at it, can you also initialize program array above to NULL for the sake of consistency and completeness?

nikhiljnv · 2023-11-10T04:48:39Z

test_conformance/extensions/cl_khr_external_semaphore/test_external_semaphore.cpp

-    // value
-    SEMAPHORE_PARAM_TEST(CL_SEMAPHORE_REFERENCE_COUNT_KHR, cl_uint, 1);
+        SEMAPHORE_PARAM_TEST(
+            CL_SEMAPHORE_EXPORT_HANDLE_TYPES_KHR, cl_uint,


A couple of things -

If we are creating semaphore by importing it from Vulkan, export handle types list can be NULL/absent. So, this place doesn't seem about right for me to check for exportable handle types query.

The return type for CL_SEMAPHORE_EXPORT_HANDLE_TYPES_KHR query is cl_external_semaphore_handle_type_khr[], not cl_uint.

Agreed. This isn't new code for my change. Lets raise an issue to ensure we properly test this query, I would rather not add a new subtest in this change.

joshqti · 2023-11-21T16:01:12Z

Pushed patch to address comments

Deleted test cases that are no longer testable according to the spec.

-Delete obsolete code relating to offsets -Propagate dedicated memory change

Some subtests did not fail on incorrect result. Changes to macros to fail, so this does not occur again.

Test cases are not related to this extension.

Add support for any handle type supported by the platform. Change-Id: I6765fde5e7929988f49bfbf2df2f41d5263b6abc

…YPE_OPAQUE_WIN32_NT_KMT as it appears to be redundant.

nikhiljnv

LGTM.

nikhiljnv · 2023-11-29T10:32:53Z

Merging as agreed in Memory subgroup call on November 28th, 2023.

lakshmih added the focused review label Sep 28, 2023

lakshmih requested review from nikhiljnv, paulfradgley, bashbaug and kpet September 28, 2023 23:47

nikhiljnv requested changes Nov 10, 2023

View reviewed changes

joshqti added 8 commits November 27, 2023 16:28

Added support for SYNC_FD and other handle types

077dd5f

Fix consistency test

300df4f

Deleted test cases that are no longer testable according to the spec.

Fix multi-import tests

1e8e12f

-Delete obsolete code relating to offsets -Propagate dedicated memory change

Fix error handling

5522348

Some subtests did not fail on incorrect result. Changes to macros to fail, so this does not occur again.

Delete invalid test cases

a3b4fae

Test cases are not related to this extension.

External memory test

2e08d7e

Add support for any handle type supported by the platform. Change-Id: I6765fde5e7929988f49bfbf2df2f41d5263b6abc

Update multi-import tests to use new semaphore types

04c4e3a

Fix formatting

b4293de

joshqti force-pushed the semaphore_types_bug_fixes_revised branch from aa2d3b4 to affaad6 Compare November 28, 2023 00:39

Addressed review comments. Deleted VULKAN_EXTERNAL_SEMAPHORE_HANDLE_T…

2f83bca

…YPE_OPAQUE_WIN32_NT_KMT as it appears to be redundant.

joshqti force-pushed the semaphore_types_bug_fixes_revised branch from affaad6 to 2f83bca Compare November 28, 2023 01:31

nikhiljnv approved these changes Nov 29, 2023

View reviewed changes

nikhiljnv merged commit f5bd92b into KhronosGroup:main Nov 29, 2023
7 checks passed

bcalidas mentioned this pull request Dec 7, 2023

Consistency tests for CL_DEVICE_EXTERNAL_MEMORY_IMPORT_ASSUME_LINEAR_HANDLE_TYPES_KHR #1820

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Semaphore types bug fixes revised #1822

Semaphore types bug fixes revised #1822

joshqti commented Sep 28, 2023 •

edited

Loading

nikhiljnv left a comment

nikhiljnv Nov 10, 2023

joshqti Nov 16, 2023

nikhiljnv Nov 10, 2023

joshqti Nov 16, 2023

nikhiljnv left a comment

nikhiljnv Nov 10, 2023

joshqti Nov 16, 2023 •

edited

Loading

nikhiljnv Nov 21, 2023

joshqti Nov 21, 2023

nikhiljnv Nov 10, 2023

nikhiljnv Nov 10, 2023

joshqti Nov 16, 2023

joshqti commented Nov 21, 2023

nikhiljnv left a comment

nikhiljnv commented Nov 29, 2023

		@@ -1299,38 +1313,29 @@ int test_image_common(cl_device_id device_, cl_context context_,
		cl_kernel kernel_unsigned[num_kernels] = { NULL, NULL, NULL, NULL };

Semaphore types bug fixes revised #1822

Semaphore types bug fixes revised #1822

Conversation

joshqti commented Sep 28, 2023 • edited Loading

nikhiljnv left a 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

nikhiljnv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshqti Nov 16, 2023 • 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

joshqti commented Nov 21, 2023

nikhiljnv left a comment

Choose a reason for hiding this comment

nikhiljnv commented Nov 29, 2023

joshqti commented Sep 28, 2023 •

edited

Loading

joshqti Nov 16, 2023 •

edited

Loading