Skip to content

Conversation

@fabiomestre
Copy link
Contributor

@fabiomestre fabiomestre commented Jul 8, 2024

Updates the implementation of command-buffers to use translated command-list handles in every API call instead of only using them for experimental L0 API's. This avoids having to keep track of 2 separate command-list handles which can be confusing.

@github-actions github-actions bot added level-zero L0 adapter specific issues command-buffer Command Buffer feature addition/changes/specification labels Jul 8, 2024
@EwanC
Copy link
Contributor

EwanC commented Jul 9, 2024

@nrspruit You originally authored #1778, does this change make sense to you?

@fabiomestre fabiomestre marked this pull request as ready for review July 11, 2024 10:19
@fabiomestre fabiomestre requested a review from a team as a code owner July 11, 2024 10:19
@fabiomestre fabiomestre requested a review from nrspruit July 11, 2024 10:19
Copy link
Contributor

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

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

please test this with ZE_ENABLE_LOADER_INTERCEPT=1 set. I have a suspicion that you can't just use a translated handles in regular APIs.

auto Platform = CommandBuffer->Context->getPlatform();
ZE2UR_CALL(
Platform->ZeMutableCmdListExt.zexCommandListUpdateMutableCommandsExp,
(CommandBuffer->ZeComputeCommandListTranslated, &MutableCommandDesc));
Copy link
Contributor

@nrspruit nrspruit Jul 30, 2024

Choose a reason for hiding this comment

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

You still need the translated compute command list handle. Any call to an extension function pointer requires all handles used are translated.

&ZeMutableCommandDesc, &CommandId));
ZE2UR_CALL(
Platform->ZeMutableCmdListExt.zexCommandListGetNextCommandIdExp,
(CommandBuffer->ZeComputeCommandList, &ZeMutableCommandDesc, &CommandId));
Copy link
Contributor

Choose a reason for hiding this comment

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

This will fail, you need to translate both handles, not just the copy above. This will break the code.

Copy link
Contributor

@nrspruit nrspruit left a comment

Choose a reason for hiding this comment

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

This is incorrect, translated handles can only be used on EXP function pointers retrieved from the driver, they will cause segfaults if used on normal apis and one is running with multiple drivers.

@fabiomestre
Copy link
Contributor Author

Closing due to review feedback mentioning that this won't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

command-buffer Command Buffer feature addition/changes/specification level-zero L0 adapter specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants