Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Command-buffer query for supported queue properties #850

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

EwanC
Copy link
Contributor

@EwanC EwanC commented Sep 28, 2022

This change introduces a new device query related to the command-buffer extension - CL_DEVICE_COMMAND_BUFFER_SUPPORTED_QUEUE_PROPERTIES_KHR. This is different from CL_DEVICE_COMMAND_BUFFER_REQUIRED_QUEUE_PROPERTIES_KHR, as we want to convey to the user that an implementation allows using a queue property with a command-buffer, but it is not mandatory to use the property with a command-buffer.

This mechanism supersedes reporting queue related values from the CL_DEVICE_COMMAND_BUFFER_CAPABILITIES_KHR query. The flaw with CL_DEVICE_COMMAND_BUFFER_CAPABILITIES_KHR is that it contains bits explicitly added by the command-buffer extension for reporting support for queue properties. This is a brittle design, as any new queue property added in future would need to have a new bit added here in the command-buffer extension to report support when used with command-buffers.

Instead, a better design is to have a new query reporting queue properties supported, CL_DEVICE_COMMAND_BUFFER_SUPPORTED_QUEUE_PROPERTIES_KHR, and keeping CL_DEVICE_COMMAND_BUFFER_CAPABILITIES_KHR for capabilities unrelated to the command-queue properties.

The CL_COMMAND_BUFFER_CAPABILITY_OUT_OF_ORDER_KHR use-case can now be covered by returning CL_QUEUE_OUT_OF_ORDER_EXEC_MODE_ENABLE from CL_DEVICE_COMMAND_BUFFER_SUPPORTED_QUEUE_PROPERTIES_KHR, so it is removed.

Make CL_QUEUE_PROFILING_ENABLE the mandated minimum capability reported from CL_DEVICE_COMMAND_BUFFER_SUPPORTED_QUEUE_PROPERTIES_KHR, to keep existing command-buffer extension requirement to implement profiling, which is inline with minimum requirements for host queues.

Related OpenCL repo PRs:

@EwanC EwanC added the OpenCL Extension Spec Issues related to the OpenCL Extension specification. label Sep 28, 2022
@EwanC EwanC force-pushed the command-buffer_supported_queue_props branch from 3a44a7b to 74655d9 Compare November 1, 2022 08:55
@bashbaug
Copy link
Contributor

I lost track of this one, my apologies. Is it ready to go?

@EwanC
Copy link
Contributor Author

EwanC commented Nov 30, 2022

I lost track of this one, my apologies. Is it ready to go?

No worries, I've deliberately been holding back from adding it to the WG agenda because of the Mobica CTS project adding command-buffer tests. I think doing an API change in the middle of that work is a complication we should avoid. So once that's done, I think we could make this API change and I can make the according CTS update myself to reflect it.

EwanC added a commit to EwanC/OpenCL-Docs that referenced this pull request Mar 31, 2023
Introduce query `CL_COMMAND_BUFFER_CONTEXT_KHR` for the
`cl_context` associated with a command-buffer.

This PR uses enum value `0x1299` which is also attempting
be used by another command-buffer PR
KhronosGroup#850
However, that PR still needs review so would prefer
to use that enum here as `0x1299` is contiguous with
the other command-buffer query enums, and I can find a new
value for PR KhronosGroup#850

Closes KhronosGroup#898
EwanC added a commit to EwanC/OpenCL-Docs that referenced this pull request Mar 31, 2023
Introduce query `CL_COMMAND_BUFFER_CONTEXT_KHR` for the
`cl_context` associated with a command-buffer.

This PR uses enum value `0x1299` which is also attempting
be used by another command-buffer PR
KhronosGroup#850
However, that PR still needs review so would prefer
to use that enum here as `0x1299` is contiguous with
the other command-buffer query enums, and I can find a new
value for PR KhronosGroup#850

Closes KhronosGroup#898
xml/cl.xml Outdated Show resolved Hide resolved
bashbaug pushed a commit that referenced this pull request Apr 4, 2023
Introduce query `CL_COMMAND_BUFFER_CONTEXT_KHR` for the
`cl_context` associated with a command-buffer.

This PR uses enum value `0x1299` which is also attempting
be used by another command-buffer PR
#850
However, that PR still needs review so would prefer
to use that enum here as `0x1299` is contiguous with
the other command-buffer query enums, and I can find a new
value for PR #850

Closes #898
@EwanC EwanC force-pushed the command-buffer_supported_queue_props branch from 74655d9 to bcd5836 Compare April 5, 2023 08:04
@EwanC EwanC added the cl_khr_command_buffer Relating to the command-buffer family of extension label Apr 27, 2023
@EwanC EwanC force-pushed the command-buffer_supported_queue_props branch from bcd5836 to 6b5aae7 Compare May 17, 2023 05:06
| Bitmask of the supported properties with which a command-queue may be created
to allow a command-buffer to be executed on it. It is invalid for a
command-queue to be created with a property not reported and still be
compatible with command-buffer execution.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the queue properties for executing a command buffer mandated to be the same as the queue properties while recording the command buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we currently have this wording in the spec about being compatible

Command-buffers are created using an ordered list of command-queues that commands are recorded to and execute on by default. These command-queues can be replaced on command-buffer enqueue with different command-queues, provided for each element in the replacement list the substitute command-queue is compatible with the command-queue used on command-buffer creation. Where a compatible command-queue is defined as a command-queue with identical properties targeting the same device and in the same OpenCL context.

And then the actual error condition to clEnqueueCommandBufferKHR is

CL_INCOMPATIBLE_COMMAND_QUEUE_KHR if any element of queues is not compatible with the command-queue set on command_buffer creation at the same list index.

I've ignored that clSetCommandQueueProperty exists because its been depreciated since OpenCL 1.1, as that could change a command-queue's properties after creation. Maybe I should put a note about not using that.

@EwanC EwanC force-pushed the command-buffer_supported_queue_props branch 2 times, most recently from fc422e1 to 731342c Compare August 2, 2023 09:20
This change introduces a new device query related to the
command-buffer extension -
`CL_DEVICE_COMMAND_BUFFER_SUPPORTED_QUEUE_PROPERTIES_KHR`.

This is different from
`CL_DEVICE_COMMAND_BUFFER_REQUIRED_QUEUE_PROPERTIES_KHR`, as we want
to convey to the user that an implementation supports using a queue
property with a command-buffer, but is not *required* to use the
property.

This supersedes reporting queue related values from the
`CL_DEVICE_COMMAND_BUFFER_CAPABILITIES_KHR` query. The flaw
with `CL_DEVICE_COMMAND_BUFFER_CAPABILITIES_KHR` is that it contains bits
explicitly added by the command-buffer extension for reporting support for
queue properties. This is a brittle design, as any new queue property added
in future would need to have a new bit added here in the command-buffer extension
to report support when used with command-buffers.

Instead a better design is to have a new query reporting queue
properties supported,
`CL_DEVICE_COMMAND_BUFFER_SUPPORTED_QUEUE_PROPERTIES_KHR`, and keeping
`CL_DEVICE_COMMAND_BUFFER_CAPABILITIES_KHR` for capabilities unrelated
to the command-queue properties.

The `CL_COMMAND_BUFFER_CAPABILITY_OUT_OF_ORDER_KHR` use-case can now be
covered by returning
`CL_QUEUE_OUT_OF_ORDER_EXEC_MODE_ENABLE` from
`CL_DEVICE_COMMAND_BUFFER_SUPPORTED_QUEUE_PROPERTIES_KHR`, so it is
removed.
@EwanC EwanC force-pushed the command-buffer_supported_queue_props branch from 731342c to 6fa65be Compare October 2, 2024 12:51
EwanC added a commit to EwanC/OpenCL-Headers that referenced this pull request Oct 2, 2024
Header update generated from OpenCL-Docs PR XML change
KhronosGroup/OpenCL-Docs#850
EwanC added a commit to EwanC/OpenCL-CLHPP that referenced this pull request Oct 2, 2024
Add `CL_DEVICE_COMMAND_BUFFER_SUPPORTED_QUEUE_PROPERTIES_KHR`
from KhronosGroup/OpenCL-Docs#850
EwanC added a commit to EwanC/OpenCL-CTS that referenced this pull request Oct 3, 2024
CTS test update to match OpenCL-Doc & OpenCL-Header PRs:
* KhronosGroup/OpenCL-Headers#265
* KhronosGroup/OpenCL-Docs#850

Still in draft as i've not yet updated an implementation to verify these test changes
EwanC added a commit to EwanC/OpenCL-Headers that referenced this pull request Oct 4, 2024
Updates to the command-buffer emulation layer to support
changes from KhronosGroup/OpenCL-Docs#850

Requires using KhronosGroup#265
in `external/OpenCL-Headers` and can be used to test CTS changes from
KhronosGroup/OpenCL-CTS#2101
EwanC added a commit to EwanC/SimpleOpenCLSamples that referenced this pull request Oct 4, 2024
Updates to the command-buffer emulation layer to support
changes from KhronosGroup/OpenCL-Docs#850

Requires using KhronosGroup/OpenCL-Headers#265
in `external/OpenCL-Headers` and can be used to test CTS changes from
KhronosGroup/OpenCL-CTS#2101
EwanC added a commit to EwanC/SimpleOpenCLSamples that referenced this pull request Oct 4, 2024
Updates to the command-buffer emulation layer to support
changes from KhronosGroup/OpenCL-Docs#850

Requires using KhronosGroup/OpenCL-Headers#265
in `external/OpenCL-Headers` and can be used to test CTS changes from
KhronosGroup/OpenCL-CTS#2101
EwanC added a commit to EwanC/OpenCL-CLHPP that referenced this pull request Oct 9, 2024
Add `CL_DEVICE_COMMAND_BUFFER_SUPPORTED_QUEUE_PROPERTIES_KHR`
from KhronosGroup/OpenCL-Docs#850
@bashbaug bashbaug requested a review from kpet October 15, 2024 16:52
@bashbaug bashbaug mentioned this pull request Oct 22, 2024
30 tasks
@bcalidas
Copy link

bcalidas commented Nov 5, 2024

Looks ok

Copy link
Contributor

@kpet kpet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cl_khr_command_buffer Relating to the command-buffer family of extension OpenCL Extension Spec Issues related to the OpenCL Extension specification.
Projects
Status: Reviewer approved
Development

Successfully merging this pull request may close these issues.

5 participants