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

improve param_value_size consistency #1254

Merged
merged 3 commits into from
Oct 8, 2024

Conversation

bashbaug
Copy link
Contributor

@bashbaug bashbaug commented Sep 9, 2024

This fixes most of #1216.

  • Improves the spec consistency for all of the param_value_size argument descriptions.
  • Improves the spec consistency for all of the error conditions related to param_value_size.
  • Fixes a few other minor editorial bugs found while making these changes.

Copy link
Contributor

@alycm alycm left a comment

Choose a reason for hiding this comment

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

LGTM, the param value changes is consistent with #1216 (and fundamentally makes sense).

The other misc error code clean ups all look good and non-controversial.

queried with_ {clGetGLContextInfoKHR}>> table, or if the size in bytes
specified by _param_value_size_ is less than the size of the return type
shown in the table and _param_value_ is not a `NULL` value
* {CL_INVALID_VALUE} if an property name other than {CL_GL_CONTEXT_KHR},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* {CL_INVALID_VALUE} if an property name other than {CL_GL_CONTEXT_KHR},
* {CL_INVALID_VALUE} if a property name other than {CL_GL_CONTEXT_KHR},

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've fixed this, though the fix was slightly different than the suggested change.

As background, the previous text said:

if an property name other than those specified in table 4.5 is specified in properties.

However, there is no table 4.5 in the current specs.

Going back to the old OpenCL 1.2 extensions spec, there was a table describing additions to table 4.5, which included the new properites to pass the CL_CGL_SHAREGROUP_KHR, CL_EGL_DISPLAY_KHR, etc., but these were additions to an existing table, not the full table.

Since the platform needs to be passed for ICD implementations at the very least to know where to dispatch this function, clearly the list of valid properties extends beyond just the additions to the table, but it's not immediately clear to me if all of the valid context properties from clCreateContext are valid here also, or if it's just some subset.

For now, I've simply changed the text to:

if a property name specified in properties is invalid.

This isn't great, but it's better than the previous text, which referred to a nonexistant table.

api/opencl_runtime_layer.asciidoc Outdated Show resolved Hide resolved
@bashbaug bashbaug merged commit d65b739 into KhronosGroup:main Oct 8, 2024
2 checks passed
@bashbaug bashbaug deleted the param-value-size-consistency branch October 8, 2024 20:05
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.

3 participants