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

External memory import of dedicated/non-dedicated memory #970

Merged
merged 4 commits into from Oct 3, 2023

Conversation

joshqti
Copy link
Contributor

@joshqti joshqti commented Sep 15, 2023

Specifies when OpenCL becomes aware of imported image memory layout metadata. This is meant to clarify how OpenCL interacts with dedicated memory.

Clarify memory re-binding
Clarify when OpenCL becomes aware of image meta data,
when importing from other APIs.
Copy link
Contributor

@nikhiljnv nikhiljnv left a comment

Choose a reason for hiding this comment

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

Left some thoughts/comments.

@@ -211,6 +211,7 @@ If {CL_MEM_DEVICE_HANDLE_LIST_KHR} is not specified as part of _properties_, the

The properties used to create a buffer or image from an external memory handle are described by related extensions.
When a buffer or image is created from an external memory handle, the _flags_ used to specify usage information for the buffer or image must not include {CL_MEM_USE_HOST_PTR}, {CL_MEM_ALLOC_HOST_PTR}, or {CL_MEM_COPY_HOST_PTR}, and the _host_ptr_ argument must be `NULL`.
For images created from an external memory handle, implementations will acquire information about image attributes such as format and layout at the time of creation and continue to apply those attributes for the lifetime of the image object.
Copy link
Contributor

Choose a reason for hiding this comment

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

This issue may affect buffers created from external memory handle as well. Also, the statement in a way is assuming kind of inference at the time of creation which may not be necessary/applicable for all implementations.

How about something like this -
"For cl_mem created from an external memory handle, the memory attributes like layout, format etc will be borrowed from external memory handle properties and the information provided by the application at the time of creating such a cl_mem. The same shall continue to apply throughout the lifetime of the cl_mem. Any changes to external memory properties happened outside the scope of defined OpenCL APIs will not be reflected in OpenCL's view of the cl_mem. A new cl_mem must be created by re-importing the external memory handle with appropriate properties in order to see these changes reflected in OpenCL."

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.

Looks generally good, I've suggested a few language tweaks.

@@ -211,6 +211,7 @@ If {CL_MEM_DEVICE_HANDLE_LIST_KHR} is not specified as part of _properties_, the

The properties used to create a buffer or image from an external memory handle are described by related extensions.
When a buffer or image is created from an external memory handle, the _flags_ used to specify usage information for the buffer or image must not include {CL_MEM_USE_HOST_PTR}, {CL_MEM_ALLOC_HOST_PTR}, or {CL_MEM_COPY_HOST_PTR}, and the _host_ptr_ argument must be `NULL`.
For images created from an external memory handle, implementations will acquire information about image attributes such as format and layout at the time of creation and continue to apply those attributes for the lifetime of the image object.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest s/will/may/ as acquiring the information is not mandatory. The second part of the sentence could be tweaked accordingly, maybe:

When images are created from an external memory handle, implementations may acquire information about image attributes such as format and layout at the time of creation. When such information is acquired at image creation time, it is used as for the lifetime of the image object.

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 agree, and have made the requested change.

Thanks,
Joshua Kelly, Qualcomm

Make it clear that import may acquire image format layout,
but also may not.
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.

Thanks!

Copy link
Contributor

@nikhiljnv nikhiljnv left a comment

Choose a reason for hiding this comment

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

LGTM.

@nikhiljnv
Copy link
Contributor

Merging as discussed on Memory subgroup call on October 3.

@nikhiljnv nikhiljnv merged commit abf4d3d into KhronosGroup:main Oct 3, 2023
2 checks passed
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.

None yet

3 participants