Skip to content

Conversation

@Xewar313
Copy link
Contributor

No description provided.

@Xewar313 Xewar313 requested review from a team as code owners May 13, 2025 12:55
@Xewar313 Xewar313 requested a review from EwanC May 13, 2025 12:55
@Xewar313 Xewar313 temporarily deployed to WindowsCILock May 13, 2025 12:55 — with GitHub Actions Inactive
@Xewar313 Xewar313 temporarily deployed to WindowsCILock May 13, 2025 13:27 — with GitHub Actions Inactive
@Xewar313 Xewar313 temporarily deployed to WindowsCILock May 13, 2025 13:27 — with GitHub Actions Inactive
ur_device_handle_t hDevice = nullptr;

// tells if event has been enqueued in some way (e.g. by appending to a queue)
bool isEventInUse = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, this might be a bit error-prone, especially if you're not setting it for regular operations. Could we use resetQueueAndCommand instead and query if we have ommandType set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is true that this may be error prone, however I don't think that your suggestion does what is required - we need to mark events that are signaled by command buffers, and not use them until the command buffer is enqueued. In that case the commandType is still set, and I don't think this is the way to distinguish events created by queue, from these created by command buffer

return UR_RESULT_SUCCESS;
}
void ur_exp_command_buffer_handle_t_::registerEvent(ur_event_handle_t event) {
addedEvents.push_back(event);
Copy link
Contributor

Choose a reason for hiding this comment

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

what are the addedEvents used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used to enable all events of command buffer when it is enqueued.

const std::vector<ur_device_handle_t> hDevices;
v2::command_list_cache_t commandListCache;
v2::event_pool_cache eventPoolCache;
v2::event_pool_cache eventPoolCacheImmediate, eventPoolCacheRegular;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: for consistency, please define the variables in separate lines

const ur_event_handle_t *phEventWaitList) try {
for (uint32_t i = 0; i < numEvents; ++i) {
if (!phEventWaitList[i]->getIsEventInUse()) {
// TODO: This is a workaround for the underlying inconsistency
Copy link
Contributor

Choose a reason for hiding this comment

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

So what's the state of regular and counter-based events by default right now? Is it that regular events are not signaled? If so, could we just signal them manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I understand, when regular event is signaled by command buffer that has not been enqueued, it is signaled. However, as soon as command buffer gets enqueued the event becomes not signaled, and waits for command buffer execution. However, that is not the case for counter-based events, what causes issues - if we use counter-based event, the event that is signaled by not enqueued command buffer is not signaled by default.
Maybe @pbalcer can explain it clearer.

@Xewar313
Copy link
Contributor Author

Closing this PR, since it has some functionality that does not seem to work properly with L0 driver.

@Xewar313 Xewar313 closed this May 14, 2025
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