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.

Sign up for GitHub

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

Jump to bottom

Add push descriptor validation #460

Merged
merged 4 commits into from
Nov 30, 2018
Merged

Add push descriptor validation #460

merged 4 commits into from
Nov 30, 2018

Conversation

jzulauf-lunarg
Copy link
Contributor

@jzulauf-lunarg jzulauf-lunarg commented Nov 6, 2018 •
edited
Loading

Add validation of CmdPushDescriptorSet(WithTemplate) descriptor state updates, removing the code disabling later use of the recorded data.

Addresses #413 and #428

@tobine
Copy link
Contributor

tobine commented Nov 8, 2018

Android build broken

@jzulauf-lunarg jzulauf-lunarg force-pushed the zulauf_pd_implement_calltime_validation_428 branch 3 times, most recently from c1dae02 to 5fdc1c1 Compare November 8, 2018 22:25
@jzulauf-lunarg jzulauf-lunarg force-pushed the zulauf_pd_implement_calltime_validation_428 branch from 5fdc1c1 to dbe9f97 Compare November 27, 2018 23:46
@jzulauf-lunarg jzulauf-lunarg self-assigned this Nov 27, 2018
tobine
tobine reviewed Nov 28, 2018
View reviewed changes
Copy link
Contributor

@tobine tobine left a comment

Choose a reason for hiding this comment

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

A few things for you to check out. Not quite through this but wanted to queue up these comments and will finish the rest of the review today.

layers/descriptor_sets.cpp Outdated Show resolved Hide resolved
layers/descriptor_sets.cpp
<< "VkWriteDescriptorSetInlineUniformBlockEXT missing";
} else {
error_str << "Attempting write update to descriptor set " << set_ << " binding #" << update->dstBinding << " with "
error_str << "Attempting write update to " << StringifySetAndLayout() << " binding #" << update->dstBinding
<< " with "
Copy link
Contributor

Choose a reason for hiding this comment

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

The formatting on these lines looks a little goofy with two really short strings (1 here, 1 on 1904 below). Perhaps possible to combine a line or two from below in order to save lines?

layers/core_validation.cpp
@@ -31,6 +31,7 @@
* Author: Mike Schuchardt <mikes@lunarg.com>
* Author: Mike Weiblen <mikew@lunarg.com>
* Author: Tony Barbour <tony@LunarG.com>
* Author: John Zulauf <jzulauf@lunarg.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

YES! Finally getting all that fame you deserve! 👍

layers/core_validation.cpp Outdated
@@ -772,6 +773,15 @@ shader_module const *GetShaderModuleState(layer_data const *dev_data, VkShaderMo
return it->second.get();
}

const TEMPLATE_STATE *GetDescriptorTemplateState(const layer_data *dev_data,
VkDescriptorUpdateTemplateKHR descriptor_update_template) {
auto it = dev_data->desc_template_map.find(descriptor_update_template);
Copy link
Contributor

Choose a reason for hiding this comment

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

const auto &?

layers/core_validation.cpp
@@ -1230,14 +1240,6 @@ static bool ValidateCmdBufDrawState(layer_data *dev_data, GLOBAL_CB_NODE *cb_nod

for (const auto &set_binding_pair : pPipe->active_slots) {
uint32_t setIndex = set_binding_pair.first;

// TODO -- remove this continue path when CmdPushDescriptorSet is implemented, for now it prevents a false positive
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay!

layers/core_validation.cpp Outdated Show resolved Hide resolved
layers/core_validation.cpp Outdated Show resolved Hide resolved
layers/core_validation.cpp Show resolved Hide resolved
layers/core_validation.cpp Outdated Show resolved Hide resolved
tobine
tobine suggested changes Nov 28, 2018
View reviewed changes
Copy link
Contributor

@tobine tobine left a comment

Choose a reason for hiding this comment

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

There's a few changes that should be made before landing. A couple of minor functional issues and some typos.

layers/core_validation.cpp Show resolved Hide resolved
layers/core_validation.cpp Outdated Show resolved Hide resolved
layers/core_validation.cpp Show resolved Hide resolved
layers/core_validation.cpp Show resolved Hide resolved
layers/core_validation_error_enums.h Outdated Show resolved Hide resolved
layers/core_validation.cpp Outdated Show resolved Hide resolved
layers/core_validation_types.h Show resolved Hide resolved
layers/descriptor_sets.h Outdated Show resolved Hide resolved
@jzulauf-lunarg jzulauf-lunarg force-pushed the zulauf_pd_implement_calltime_validation_428 branch from dbe9f97 to d88f226 Compare November 28, 2018 23:35
@jzulauf-lunarg
Copy link
Contributor Author

@tobine -- I think I picked up everything from your review.

This was referenced Nov 29, 2018
Closed
Closed
tobine
tobine approved these changes Nov 29, 2018
View reviewed changes
Copy link
Contributor

@tobine tobine left a comment

Choose a reason for hiding this comment

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

Great

layers/core_validation.cpp Outdated
func_name, set, layout_u64);
const auto dsl = set_layouts[set];
if (dsl) {
if (0 == (dsl->IsPushDescriptor())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (!dsl->IsPushDescriptor())

@jzulauf-lunarg
tests: Fix push descriptor test usage flag
18ee3c1 
Tested buffer was not setting the correct usage flag.  Enabling push
descriptor validation (next commits) causes test case to fail

Change-Id: Ibd8fd98259825ae376289b4d9ed867aa1e202818
@jzulauf-lunarg
layers: Add push descriptor option to DS validation
945c7ef 
Update the write descriptor validation to be able to validate and report
a push descriptor update.

Change-Id: I8b2c0de85ae3eded211fd5ac397c9e30de3a5d34
@jzulauf-lunarg
layers: Add push descriptor validation
4f23add 
Add validation of CmdPushDescriptorSet(WithTemplate) descriptor state
updates, removing the code disabling later use of the recorded data.

Change-Id: If9c4345e67c974f36e7a5356a517f411e470c2c5
@jzulauf-lunarg
tests: Add unit test for push descriptor validation
878dad5 
Add push descriptor validation to buffer validation path test.

Change-Id: I80135f69a0ac4351b6415559403573574917b281
@jzulauf-lunarg jzulauf-lunarg force-pushed the zulauf_pd_implement_calltime_validation_428 branch from d88f226 to 878dad5 Compare November 30, 2018 00:45
@jzulauf-lunarg jzulauf-lunarg merged commit 37a3ba5 into master Nov 30, 2018
@jzulauf-lunarg jzulauf-lunarg deleted the zulauf_pd_implement_calltime_validation_428 branch November 30, 2018 15:04
@jzulauf-lunarg jzulauf-lunarg mentioned this pull request Dec 17, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobine tobine tobine approved these changes

@TonyBarbour TonyBarbour Awaiting requested review from TonyBarbour

@mikes-lunarg mikes-lunarg Awaiting requested review from mikes-lunarg

@daveh-lunarg daveh-lunarg Awaiting requested review from daveh-lunarg

@mark-lunarg mark-lunarg Awaiting requested review from mark-lunarg

@shannon-lunarg shannon-lunarg Awaiting requested review from shannon-lunarg

@chrisforbes chrisforbes Awaiting requested review from chrisforbes

Assignees

@jzulauf-lunarg jzulauf-lunarg

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jzulauf-lunarg @tobine