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

scripts: Split up chasssis.{h,cpp} #8904

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

jeremyg-lunarg
Copy link
Contributor

Move the giant blocks of 'manual' code to files under layers/chassis

DispatchObject and ValidationObject are a big mix of manual and generated code. The class definition headers #include a file defining generated methods.

chassis.h is now a manual file that only defines some shared data structures and the manual defined functions needed by the generated code.

@jeremyg-lunarg jeremyg-lunarg requested a review from a team as a code owner November 21, 2024 17:59
@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 307711.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 18094 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 18094 failed.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 307738.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 18095 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 307754.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 18096 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 18096 failed.

Copy link
Contributor

@spencer-lunarg spencer-lunarg left a comment

Choose a reason for hiding this comment

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

LGTM (assuming things are just copy-and-pasted where I expect it to be)

Did a double check locally and seems things are split up well, of course will want @aqnuep sign off as well as someone who will have to ingest this downstream

@@ -21,7 +21,6 @@

#include <vulkan/vk_enum_string_helper.h>
#include "error_message/error_location.h"
#include "generated/chassis.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

not having to whole-include the chassis here makes me happy

wrap_handles = false;
}

// Add VOs to dispatch vector. Order here will be the validation dispatch order!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part needs to go back to generated code because it is different for SC

// Note that this DEFINES THE ORDER IN WHICH THE LAYER VALIDATION OBJECTS ARE CALLED
disabled = instance_dispatch->disabled;
enabled = instance_dispatch->enabled;
if (!disabled[thread_safety]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part needs to go back to generated code because it is different for SC

bool &is_secondary) {
{
auto lock = ReadLockGuard(secondary_cb_map_mutex);
is_secondary = secondary_cb_map.find(commandBuffer) != secondary_cb_map.end();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

secondary command buffer tracking was happening in chassis.cpp. It is needed here for handle wrapping weirdness. Unfortunately, thread safety and object tracking were relying on is_secondary as well so I had to hack to pass it back out of this method :(

Copy link
Contributor

Choose a reason for hiding this comment

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

this is because VkCommandBufferBeginInfo::pInheritanceInfo can be a garbage pointer, we need to ignore it (including Safe Struct) if it is not a secondary pointer

image

Going to create an issue if we can require this to be null as the app really should clear it out and not require everyone to state track to know if we can deference it or not

Copy link
Contributor

Choose a reason for hiding this comment

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

@jeremyg-lunarg
Copy link
Contributor Author

LGTM (assuming things are just copy-and-pasted where I expect it to be)

Did a double check locally and seems things are split up well, of course will want @aqnuep sign off as well as someone who will have to ingest this downstream

The files under layers/chassis and scripts/generators need the most review IMO. But that can wait until I clean up the epic fails I'm getting in CI

@jeremyg-lunarg jeremyg-lunarg force-pushed the jeremyg-dispatch-2 branch 5 times, most recently from 3e68248 to 6fbaa52 Compare November 25, 2024 17:00
@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 309130.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 18117 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 18117 failed.

Move the giant blocks of 'manual' code to files under layers/chassis

DispatchObject and ValidationObject are a big mix of manual and
generated code. The class definition headers #include a file
defining generated methods.

chassis.h is now a manual file that only defines some shared data
structures and the manual defined functions needed by the generated
code.
@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build queued with queue ID 309181.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 18118 running.

@ci-tester-lunarg
Copy link
Collaborator

CI Vulkan-ValidationLayers build # 18118 passed.

@jeremyg-lunarg jeremyg-lunarg merged commit 71fcb0f into KhronosGroup:main Nov 26, 2024
21 checks passed
@jeremyg-lunarg jeremyg-lunarg deleted the jeremyg-dispatch-2 branch November 26, 2024 15:37
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.

3 participants