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 zelReloadDrivers(flags) API #191

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lisanna-dettwyler
Copy link
Contributor

Provides a means to re-initialize all of the drivers' library handles and DDI tables. The value of flags must match what was provided to zeInit(flags).

@lisanna-dettwyler lisanna-dettwyler force-pushed the zelreloaddriver branch 2 times, most recently from 8804e8a to 8433b6b Compare September 3, 2024 18:33
Comment on lines +84 to +89
if (drv.handle) {
auto free_result = FREE_DRIVER_LIBRARY( drv.handle );
auto failure = FREE_DRIVER_LIBRARY_FAILURE_CHECK(free_result);
if (failure)
return ZE_RESULT_ERROR_UNINITIALIZED;
}
Copy link
Contributor

@rwmcguir rwmcguir Sep 4, 2024

Choose a reason for hiding this comment

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

So overall I would ask if there is any advantage to separating this step out into it's own API of zelUnloadDriversInternal() at least initially and possibly even into user exposed API in the future. I think fine for doing a proof of concept... Down the road this would give the option of changing which drivers were re-initialized for whatever reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

2nd thought here, would be we could simplify the load step.... by removing unload... But in contrast we could then improve the unload step by adding state machine checking if any was necessary... i.e. are there any conditions in which unload was not allowed, or if a timeout/wait was necessary to kill in flight commands before catastrophic teardown.. (I realize this doesn't really need to be enforced here, as we could put that responsibility on the user and effective document 'use at your own risk')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Down the road this would give the option of changing which drivers were re-initialized for whatever reason.

I think in order to have that we would need it to operate only on a single driver, rather than separating load/unload.

are there any conditions in which unload was not allowed

I agree it's on the user to not call this if they expect in-flight things to finish, so I don't think we need to count that as one such condition. If we end up adopting this, we should document that all prior resources / handles are invalidated and that new ones will need to be obtained.

@lisanna-dettwyler
Copy link
Contributor Author

I guess the theory here is that an application could use this to guarantee a clean slate if hardware resources change out from underneath a driver library.

The only concerns I have with this are that it may be more straightforward to have it re-init only a passed in driver handle rather than all of them, but I couldn't get the handle typecasting to work inside of ze_loader_api.cpp; that I'm adding another location where I've manually written out something that is normally autogenerated; and that the debug trace logging functions aren't available for any of the failure cases.

I still need to add a null driver test to verify that this works.

@lisanna-dettwyler lisanna-dettwyler force-pushed the zelreloaddriver branch 2 times, most recently from dc926b2 to 3878b3b Compare September 17, 2024 19:18
Provides a means to re-initialize all of the drivers' library handles
and DDI tables. The value of flags must match what was provided to
zeInit(flags).

Signed-off-by: Lisanna Dettwyler <lisanna.dettwyler@intel.com>
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