Skip to content

Conversation

Kerilk
Copy link
Contributor

@Kerilk Kerilk commented Aug 10, 2024

This implements the loader managed dispatching strategy discussed in KhronosGroup/OpenCL-Docs#1003

I closed the old pull request to stop the CI spam, but I had to open a new one... Here is a link to the previous one: #227

For now it depends on #237 but I will rebase when necessary.

Comment on lines +354 to +355
// cl_khr_gl_sharing
ICD_GET_FUNCTION_ADDRESS ( clGetGLContextInfoKHR );
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 this is the right thing to do (because this function doesn't necessarily take a "dispatchable object"), but have we documented why this function is different and is returned by clIcdGetFunctionAddressForPlatformKHR even though it is an extension function?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, this is bugging me a little - which functions should clIcdGetFunctionAddressForPlatformKHR return? Would it make sense to return all functions supported by the platform, regardless whether they are core API functions or extension functions?

If we didn't do this then I think we need to make a decision without a clear right answer:

  1. We could only support core APIs (and maybe clGetGLContextInfoKHR) in the dispatch table, although this is different than the prior behavior.
  2. Or, if we wanted to keep the prior behavior, we would need to document the set of extension functions that is in the dispatch table, which AFAICT is rather random.

Returning all functions from clIcdGetFunctionAddressForPlatformKHR opens the door to supporting additional extension APIs in the ICD-created dispatch table in the future also, if desired.

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 that is one of the benefit. We'll need to double check things and I had several ideas along these lines. We can chat about it next week.

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 think this is the right thing to do (because this function doesn't necessarily take a "dispatchable object"), but have we documented why this function is different and is returned by clIcdGetFunctionAddressForPlatformKHR even though it is an extension function?

This exposes the exact same functionalities as the original stub driver, all of these were exposed and present in the dispatch table.

Comment on lines +393 to +394
/* cl_khr_gl_event */
ICD_GET_FUNCTION_ADDRESS ( clCreateEventFromGLsyncKHR);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this extension function need to be returned by clIcdGetFunctionAddressForPlatformKHR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this extension function need to be returned by clIcdGetFunctionAddressForPlatformKHR?

Lost track of some of the comments order. So I am reiterating here:
This exposes the exact same functionalities as the original stub driver, all of these were exposed and present in the dispatch table.

@Kerilk Kerilk force-pushed the cl_khr_icd2 branch 3 times, most recently from 10d7b3f to e916223 Compare April 1, 2025 16:51
Co-authored-by: Ben Ashbaugh <[email protected]>
@bashbaug
Copy link
Contributor

bashbaug commented Apr 1, 2025

Merging as discussed in the April 1st teleconference.

@bashbaug bashbaug merged commit 86448ce into KhronosGroup:main Apr 1, 2025
32 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.

2 participants