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

Add support for OpenCLOn12 ICD #103

Merged
merged 3 commits into from
Apr 16, 2020
Merged

Conversation

jenatali
Copy link
Contributor

@jenatali jenatali commented Apr 9, 2020

The OpenCLOn12 project (announced here) doesn't look like a normal ICD. On Windows, ICDs are enumerated in three ways:

  1. Explicitly belonging to a specific GPU driver installation (GPU-specific registry entries).
  2. Added to a global set of ICDs installed on the system in the registry.
  3. Set in the environment.

The OpenCLOn12 ICD won't be tied to any given hardware, and its distribution mechanism doesn't allow to modify the registry. This ICD will be shipping inside an app package, delivered by the Windows store. We'd like to add explicit logic to the ICD loader to look for this package, and if installed, load the ICD out of it.

Since this ICD doesn't have an adapter LUID associated with it, its platform will be relegated to the end of the platform list. This behavior is probably for the best.

@CLAassistant
Copy link

CLAassistant commented Apr 9, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

I'm admittedly not an expert regarding the apppackage changes, but I did build an ICD loader with these changes and verified that our ICDs still enumerate properly. Thanks!

@bashbaug
Copy link
Contributor

@nikhiljnv @b-sumner @saleelk did you want to review these changes? If so, can you please provide feedback by EOW?

@saleelk
Copy link

saleelk commented Apr 16, 2020

LGTM

@nikhiljnv
Copy link
Contributor

Looks good to me !!

@MathiasMagnus
Copy link
Contributor

This ICD will be shipping inside an app package, delivered by the Windows store.

+1 for the distribution method. Eagerly awaiting GA (or a Store link 😉).

HRESULT hrInit = Windows::Foundation::Initialize(RO_INIT_MULTITHREADED);
if (hrInit == RPC_E_CHANGED_MODE)
{
hrInit = Windows::Foundation::Initialize(RO_INIT_SINGLETHREADED);

Choose a reason for hiding this comment

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

Does this only happen in cases where the application itself is expected to be single threaded? I don't really understand the implications of falling back, or in what cases it would happen, but if the assumption above is true, seems good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, if the app has already initialized COM or WinRT as singlethreaded on the thread where the loader is being initialized, then we have to use singlethreaded as well. In all other cases (already initialized as multi-threaded, or uninitialized), we'll use multithreaded.

@@ -256,7 +256,12 @@ void khrIcdOsVendorsEnumerateOnce()
// dynamically load a library. returns NULL on failure
void *khrIcdOsLibraryLoad(const char *libraryName)
{
return (void *)LoadLibraryA(libraryName);
HMODULE hTemp = LoadLibraryExA(libraryName, NULL, LOAD_WITH_ALTERED_SEARCH_PATH);

Choose a reason for hiding this comment

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

Glad this encapsulated so well :-)

@bashbaug
Copy link
Contributor

Thanks everyone! Merging.

@bashbaug bashbaug merged commit 47f05fa into KhronosGroup:master Apr 16, 2020
@neiltrevett
Copy link

Thanks everyone!
@jenatali - we will use the weekly OpenCL calls to push reviews forward in a timely way - hopefully this one didn't hold you back too badly. If you need to alert the group to speed up a review of time sensitive merges please shout on the issue and/or ICD list and we will do our best to respond.

@jenatali jenatali deleted the openclon12 branch April 16, 2020 22:47
joekilner pushed a commit to joekilner/OpenCL-ICD-Loader that referenced this pull request Jun 20, 2020
* Add search for OpenCLOn12 mapping layer package.

* Use altered search path for ICD loading.

* Update version to 2.2.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.

None yet

8 participants