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.

Sign up for GitHub

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

Jump to bottom

The current specification makes it impossible to support a fully read-only image format (and related cl_mem_flags issues) #1110

Open
kpet opened this issue Mar 27, 2024 · 4 comments · May be fixed by #1280
Open

The current specification makes it impossible to support a fully read-only image format (and related cl_mem_flags issues) #1110

kpet opened this issue Mar 27, 2024 · 4 comments · May be fixed by #1280
Assignees
@kpet
Labels
agenda Needs Working Group Discussion

Comments

@kpet
Copy link
Contributor

kpet commented Mar 27, 2024

As part of adding support for a new image format for which we can only support read access, we're bumping into the following rule (there are similar notes for clEnqueueFillBuffer, clCommandFillBufferKHR, and clCommandFillImageKHR; these notes have been there since the beginning of our git history for clEnqueueFill* commands):

The usage information which indicates whether the memory object can be read or written by a kernel and/or the host and is given by the cl_mem_flags argument value specified when image is created is ignored by clEnqueueFillImage.

Looking at Table 19. List of supported memory flag values, it seems access control flags fall into two groups:

  • CL_MEM_READ_WRITE, CL_MEM_READ_ONLY, and CL_MEM_WRITE_ONLY specify what accesses kernels are allowed to perform on a memory object.
  • CL_MEM_HOST_WRITE_ONLY, CL_MEM_HOST_READ_ONLY, and CL_MEM_HOST_NO_ACCESS specify what accesses the host is allowed to perform on a memory object.
  1. The current state of the specification looks inconsistent
  • Do we need notes similar to the ones quoted above for copy commands? They are neither kernels nor require host access, which Table 19 suggests applies to read, write, or map commands.
  • Should the notes for fill commands be removed? There is nothing specific to fill commands that I can spot that justifies the presence of the notes in the absence of similar notes for copy commands for example.
  1. This strikes me as an odd split of capabilities. On many devices access to memory is controlled using settings that apply to all device accesses that often would cover fill commands too. This means that it is impossible on these devices to enforce read-only access permissions for kernels if fill commands run on the device and need write permissions to the same memory. One possible change we could consider would be to change the CL_MEM_* flags so they apply to all device commands and remove the notes on fill commands.
  2. If neither 2 nor removing notes on fill commands are an acceptable resolution, what mechanism do we think should be used by implementations that wish to support a truly read-only image format?
@bashbaug
Copy link
Contributor

I think there is some related discussion here: #770

@kpet
Copy link
Contributor Author

kpet commented Mar 27, 2024

Thanks Ben! I won't try to join both discussions in a comment here. I think we're more likely to make progress in a teleconference.

@kpet kpet added the agenda Needs Working Group Discussion label Mar 27, 2024
@kpet
Copy link
Contributor Author

kpet commented May 7, 2024

Discussed in the 2024/05/07 teleconference:

  • Agreement that there seems to be an inconsistency between the rules for fill and copy commands. Pending a full review of how do cl_mem_flags affect fills and copies #770, we likely want to align the rules for copy commands to those that apply to fill commands as a specification clarification.
  • There are concerns with changing the rules for fill commands to forbid calling them on images created with CL_MEM_READ_ONLY. Applications might have relied on this behaviour that has been an explicit part of the specification for many versions.
  • There are similar concerns with generally extending the meaning of the CL_MEM_READ_ONLY, CL_MEM_WRITE_ONLY, and CL_MEM_READ_WRITE flags.
  • There seems to be a consensus for adding a new cl_mem_flags (which is the only viable mechanism given the need to provide the information to clGetSupportedImageFormats) via an extension to add the concept of "device-immutable images" that could not be modified by any commands. No definitive consensus on whether mapping operations would be supported on such immutable images; leaning towards "no".
  • We have a long-term aspiration to introduce a more extensible version of clGetSupportedImageFormats to query supported image formats. We noted that cl_ext_image_requirements_info provides some related functionality.

@kpet kpet self-assigned this May 7, 2024
@bashbaug
Copy link
Contributor

Arm has an idea how to proceed. No longer needs WG discussion.

kpet added a commit to kpet/OpenCL-Docs that referenced this issue Nov 6, 2024
@kpet
Add specification for cl_ext_immutable_memory_objects
1334f2e 
Fixes KhronosGroup#1110

Change-Id: I120f66ad20f977c251b77cc42a32021cb407518e
Signed-off-by: Kevin Petit <[email protected]>
@kpet kpet linked a pull request Nov 6, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kpet kpet

Labels
agenda Needs Working Group Discussion
Projects
OpenCL specification maintenance
Status: To do
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add specification for cl_ext_immutable_memory_objects kpet/OpenCL-Docs
2 participants
@kpet @bashbaug