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

Refactor command-buffer queue compatability #1292

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

EwanC
Copy link
Contributor

@EwanC EwanC commented Dec 16, 2024

As proposed in #1142 the PR changes the semantics of the command-queues parameters used for command-buffer creation and enqueue.

The queues used on command-buffer creation now only inform the device and dependencies of commands, rather than restricting the properties set on the queues used for command-buffer enqueue.

Related PRs:

api/cl_khr_command_buffer.asciidoc Show resolved Hide resolved
creation, with the exception of any vendor extension defined queue properties
that explcitly define semantics for this purpose.

The command-queues used on command-buffer creation must be replaced on
Copy link
Contributor

Choose a reason for hiding this comment

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

Why say "must be replaced"? Can't the user use the same queue objects for creation and execution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try reword this. I was trying to communicate that since the queues used on command-buffer creation are really placeholders used to inform the device and dependencies of commands, those placeholders must be replaced with queues which will actually submit work to the device.

Copy link
Contributor

Choose a reason for hiding this comment

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

But must these queues be placeholders? Can't they be real queues to be used later for enqueuing 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.

The queue objects throughout the API must be valid cl_command_queue objects, so everything is a real queue, but their role in command creation API is placeholder in the sense that the purpose of using the queue object in the API is not for submitting work to the device but representing the devices and dependencies of commands.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand, but the question is: must the queues used for recording and replaying be different? If yes, then your wording is OK; if not, you should replace "must be replaced" with "may be replaced".

Copy link
Contributor

Choose a reason for hiding this comment

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

I think besides "must" and "may", there can also be "recommended". As in, it is, for some reason, worse if the queue is reused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no requirement that the queues must be different, they only may be different as I say in the next sentence, so I'll reword this sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this line to say "When enqueuing a command-buffer, a list of command-queues to execute the command-buffer on can be passed by the user, otherwise the command-queues set on command-buffer creation are used by default for execution. A user passed list may contain different command-queues, ..."

EwanC added 2 commits January 14, 2025 17:33
As proposed in KhronosGroup#1142
the PR changes the semantics of the command-queues parameters used for
command-buffer creation and enqueue.

The queues used on command-buffer creation now only inform the
device and dependencies of commands, rather than restricting the
properties set on the queues used for command-buffer enqueue.

This is based ontop on the change in KhronosGroup#850
to add supported queue property semantics.
Clarify wording around default list of command-queues used for command-buffer
enqueue.
@EwanC EwanC force-pushed the refactor_queue_compatability branch from aad1eef to ffd3647 Compare January 14, 2025 17:33
@EwanC EwanC changed the base branch from EwanC/tmp/command-buffer_supported_queue_props to main January 14, 2025 17:33
EwanC added a commit to EwanC/OpenCL-Headers that referenced this pull request Jan 14, 2025
Generated changes from XML update in
matching PR KhronosGroup/OpenCL-Docs#1292
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

@EwanC EwanC marked this pull request as ready for review January 16, 2025 09:51
EwanC added a commit to EwanC/OpenCL-CTS that referenced this pull request Jan 16, 2025
Update cl_khr_command_buffer tests to reflect changes from
KhronosGroup/OpenCL-Docs#1292

Required headers change
KhronosGroup/OpenCL-Headers#271 to enable
tests with version 0.9.7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants