OpenCL-CTS icon indicating copy to clipboard operation
OpenCL-CTS copied to clipboard

Improvements to the External Memory and Semaphores conformance tests

Open bcalidas opened this issue 3 years ago • 14 comments

  1. External Memory Sharing a) Write a test that does external memory sharing only and is not dependent on semaphores being supported. b) Confirm that EnqueueAcquire and EnqueueRelease calls are being made correctly for all tests.

  2. Semaphores a) Do cross OpenCL context tests for multiple semaphore handle types such as sync_fd and opaque_fd . b) Add a multiple signal - multiple wait test for in-order queues. c) Test the following queries - CL_DEVICE_HANDLE_LIST_KHR, CL_SEMAPHORE_EXPORT_HANDLE_TYPES_KHR

bcalidas avatar Nov 29 '22 01:11 bcalidas

My comment on semaphore PR https://github.com/KhronosGroup/OpenCL-CTS/pull/1587#issuecomment-1331013946 was mentioned on the 06/12/22 WG teleconference. I think it would make sense before starting any of this semaphore work to have a first step which moves the existing cl_khr_semaphore tests from test_api into its own directory in test_conformance/extensions/

EwanC avatar Dec 07 '22 12:12 EwanC

Some comments - Agree with Ewan that we should move these tests to test_conformance. Filed #1624 to track the same so that it doesn't get lost in comments.

In terms of tests suggested on this issue - I have updated the description for first item from "External sharing" to "External memory sharing" for better clarity. I think the tests requested part of 1a) and 1b) can be developed independent cl_khr_semaphore tests under test_conformance/api and need not be blocked by #1624.

Typically external memory and semaphore sharing tests can be broadly categorized as below -

  1. Core semaphore behavior described by cl_khr_semaphore - This is what cl_khr_semaphore tests under test_conformance/api/ test.
  • The test scenario described by "2b)Add a multiple signal - multiple wait test for in-order queues." is a good example of this category and can be an extension to tests under test_conformance/api/. These tests currently rely on semaphores created by OpenCL and used within OpenCL (largely same context)
  • cl_khr_external semaphore provides another mechanism to create semaphores – by importing semaphores exported by other APIs. See below for details. Same tests under test_conformance/api/ can be extended to cover external semaphores as well. We have some changes to address this to consume semaphores imported from test_vulkan. Will update here once have PR created.
  1. Core external memory/semaphore behavior described by cl_khr_external_memory/cl_khr_external_semaphore. These tests along with one or more handle specific extensions (cl_khr_external_memory_opaque_fd, cl_khr_external_semaphore_win32 etc) can be written in following different settings
  • OpenCL as producer and OpenCL as consumer (Same API / different contexts): Create memory/semaphores directly in one OpenCL context/process, export it as fd/win32 handle and then import it in another OpenCL context/process using this handle. This is what Balaji is referring to as part of 2.a. in the context of semaphores. This will again require ability to create semaphores within OpenCL as well as export them from OpenCL. We can extend existing tests under test_conformance/api/ to cover this request. 1a) is equivalent example for cl_khr_external_memory. However, this setting will require ability to export memory which is not covered by current specs. So, not sure how we can cover this under this setting. May be @bcalidas has some ideas around this.
  • Different APIs - Create semaphores in one API (e.g. Vulkan or OpenCL) (Producer), export them as opaque_fd/sync_fd/win32 handle and then import it in yet another API (e.g. OpenCL or Vulkan) using this handle. This again can be subdivided into two
  • a. With Vulkan or some other API as producer and OpenCL as consumer : test_vulkan here https://github.com/KhronosGroup/OpenCL-CTS/tree/main/test_conformance/vulkan where Vulkan is producer of memory and semaphores.
  • b. OpenCL as producer and Vulkan or some other API as consumer. This is still missing coverage.

nikhiljnv avatar Jan 24 '23 09:01 nikhiljnv

Also, it may be a good idea to review existing tests for missing coverage and file separate issues to track them.

nikhiljnv avatar Jan 24 '23 09:01 nikhiljnv

@bcalidas Can we split this issue into separate issues - One to track external_memory test suggestions in (1) and another one to track external_semaphore test suggestions in (2)? I am assuming these will end up being separate tests and (1) need not be blocked on semaphore tests as such.

nikhiljnv avatar Jan 24 '23 10:01 nikhiljnv

https://github.com/KhronosGroup/OpenCL-CTS/pull/1629 have been merged.

pj87 avatar Feb 13 '23 17:02 pj87

@bcalidas, @nikhiljnv: Friendly remainder about pending review for 1a (https://github.com/KhronosGroup/OpenCL-CTS/commit/9e83f008f5469af2e13ff4f1c7602351485d14ea).

pj87 avatar Feb 16 '23 12:02 pj87

@bcalidas, @nikhiljnv, @EwanC, @bashbaug :

Created cl_khr_external_semaphore PR: https://github.com/KhronosGroup/OpenCL-CTS/pull/1645 Created cl_klh_semaphore PR: https://github.com/KhronosGroup/OpenCL-CTS/pull/1646

Please review.

pj87 avatar Feb 18 '23 20:02 pj87

@bcalidas, @nikhiljnv, @EwanC, @bashbaug

Created a draft PR for External Sharing 1a task: https://github.com/KhronosGroup/OpenCL-CTS/pull/1648

In general it is just the same work done in: https://github.com/KhronosGroup/OpenCL-CTS/compare/main...pj87:OpenCL-CTS:cl_vk_external_sharing?expand=1

but without the proposed changes for test_vulkan_interop_image. Please review.

pj87 avatar Feb 21 '23 11:02 pj87

1 a) is covered by https://github.com/KhronosGroup/OpenCL-CTS/pull/1648

For item 1 b) , https://github.com/KhronosGroup/OpenCL-CTS/pull/1629 provides partial coverage. However, we need to follow-up by re-reviewing external memory and external semaphore tests and adding any missing clEnqueueAcquire, clEnqueueRelease calls.

bcalidas avatar Aug 23 '23 17:08 bcalidas

2 a) needs more enhancements to existing tests. 2 b) is not needed after clarification to semaphore reset behavior (https://github.com/KhronosGroup/OpenCL-Docs/issues/883) 2 c) is already covered

bcalidas avatar Aug 23 '23 17:08 bcalidas

Qualcomm will follow up with PRs for 1 b) and 2 a ).

bcalidas avatar Dec 05 '23 16:12 bcalidas

https://github.com/KhronosGroup/OpenCL-CTS/pull/1854 updates semaphore behavior to avoid export of imported semaphores and adds support for the SemaphoreReimportSyncFD command.

bcalidas avatar Dec 05 '23 16:12 bcalidas

Qualcomm will also push a PR to test the isExportable query for semaphores.

bcalidas avatar Dec 05 '23 16:12 bcalidas

Qualcomm will follow up with PRs for 1 b) and 2 a ).

@bcalidas is it possible to sum up current status of this issue ? It looks to me like part of it is covered meanwhile remaining part is in progress, could you confirm that ? Thanks !

shajder avatar Mar 20 '24 08:03 shajder

https://github.com/KhronosGroup/OpenCL-CTS/pull/1899 covers 1b, https://github.com/KhronosGroup/OpenCL-CTS/pull/1886 covers 2a

This issue can now be closed.

bcalidas avatar Jun 02 '24 01:06 bcalidas

Closing as discussed in memory subgroup call of June 4th, 2024.

nikhiljnv avatar Jun 04 '24 17:06 nikhiljnv