Fixes for pipeline multi sample issue 2159 and 2180 #2220

jzulauf-lunarg · 2017-11-20T17:01:05Z

Fixes for #2180 and #2159

Includes a common code refactor of the CreateGraphicsPipeline setup code for layer_validation_tests.cpp, used by the new cases, but presented here for review/feedback/refinement before refactoring the 20+ other copies in extant test cases to use it.

jzulauf-lunarg

Some explanatory information about the thinking behind the changes

jzulauf-lunarg · 2017-11-20T17:13:51Z

tests/layer_validation_tests.cpp

@@ -173,13 +176,18 @@ struct NearestGreaterThan {
 };
 template <typename T, typename Comp>
 T NearestNonEqual(const T value, const Comp &comp) {


The previous version had some numerical corner cases uncovered when expanding the usage. This is simpler and while it has an iterative fallback (for further missed corners), I couldn't find a test case that uses it. There are some ULP differences between this and the prior implementation that don't matter to layer validation testing.

This is awful. What does it even do?
Guessing by the name, should probably be just nextafterf();

Converting to std::nextafter

@jzulauf-lunarg From experience, it wont pass the Android CI test here. You might have to use the C version of the function.

Just google'd this... android/ndk#82 -- thanks for the heads up.

jzulauf-lunarg · 2017-11-20T17:15:33Z

tests/layer_validation_tests.cpp

@@ -946,6 +958,297 @@ struct OneOffDescriptorSet {
        return pool_ != VK_NULL_HANDLE && layout_ != VK_NULL_HANDLE && set_ != VK_NULL_HANDLE;
    }
 };
+
+struct CreatePipelineHelper {


This refactors out common setup code for testing CreateGraphicsPipelines. For typical usage see OneshotTest below

jzulauf-lunarg · 2017-11-20T17:17:48Z

tests/layer_validation_tests.cpp

+        return err;
+    }
+    template <typename Test, typename OverrideFunc, typename Error>
+    static void OneshotTest(Test *test, OverrideFunc &info_override, const VkFlags flags, Error error, bool positive_test = false) {


The usage here is similar to the rest of the framework, init, twiddle, init some more, test. Oneshot works for all the cases where simple twiddling with the create info structs (and the things they point to) in info_override is sufficient.

jzulauf-lunarg · 2017-11-20T17:18:37Z

tests/layer_validation_tests.cpp

+        }
+    };
+};
+namespace chain_util {


More common code removal for a pattern in the pNext test added below

jzulauf-lunarg · 2017-11-20T17:20:05Z

tests/layer_validation_tests.cpp

+
+    // Cause the error by enabling sample shading...
+    auto set_shading_enable = [](CreatePipelineHelper &helper) { helper.pipe_ms_state_ci_.sampleShadingEnable = VK_TRUE; };
+    CreatePipelineHelper::OneshotTest(this, set_shading_enable, VK_DEBUG_REPORT_ERROR_BIT_EXT, VALIDATION_ERROR_10000620);


Typical OneshotTest usage. The info_override is a simple one-liner.

chrisforbes · 2017-11-21T06:22:14Z

tests/layer_validation_tests.cpp

+        vkDestroyDescriptorPool(device, ds_pool_, nullptr);
+    }
+
+    void InitDescriptorPoolInfo() {


Reuse OneOffDescriptorSet?

Looking further into the test cases, VkPipelineObj should be something I'm using instead as well.

Using OneOffDescriptor, but the VkPipelineObj has some late binding smarts that could fixup intentional errors.

chrisforbes · 2017-11-21T06:23:41Z

tests/layer_validation_tests.cpp

+    VkPipelineCacheCreateInfo pc_ci_ = {};
+    VkPipeline pipeline_ = VK_NULL_HANDLE;
+    VkPipelineCache pipeline_cache_ = VK_NULL_HANDLE;
+    std::unique_ptr<VkShaderObj> vs_;


Why separate VS/FS members? The tests you want to replace use a mix of stages.

Most are VS/FS only, but looking at adapting/replacing the helper with use of the VkPipelineObj (which I didn't notice before), and that supports more stages AFAICT

tobine · 2017-11-21T16:33:38Z

tests/layer_validation_tests.cpp

+    std::vector<VkDescriptorSetLayoutBinding> dsl_bindings_;
+    VkDescriptorSetLayoutCreateInfo ds_layout_ci_ = {};
+    VkDescriptorSetLayout ds_layout_ = VK_NULL_HANDLE;
+    VkDescriptorSet descriptorSet_ = VK_NULL_HANDLE;


descriptor_set_

krOoze

I think the utility is quite useful and will simplify testing of PSO.

In future it would be nice if it was generalized. I would want to do something like Init( UPTO_DRAW ) which would get me up to the state where I can just call vkCmdDraw to test it.

Some nits:

krOoze · 2017-11-21T22:57:21Z

tests/layer_validation_tests.cpp

+    VkPipelineLayoutCreateInfo pipeline_layout_ci_ = {};
+    VkPipelineLayout pipeline_layout_ = VK_NULL_HANDLE;
+    VkPipelineViewportStateCreateInfo vp_state_ci_ = {};
+    std::vector<VkDynamicState> dyn_states_ = {VK_DYNAMIC_STATE_VIEWPORT, VK_DYNAMIC_STATE_SCISSOR, VK_DYNAMIC_STATE_LINE_WIDTH };


Why does it by default have any dynamic states?
Also: all the other members seem to be only zero initialized here. Why does this one have actual value here.

being lazy with other initialization.... fixing

krOoze · 2017-11-21T22:59:18Z

tests/layer_validation_tests.cpp

+    void InitMultisampleInfo() {
+        pipe_ms_state_ci_.sType = VK_STRUCTURE_TYPE_PIPELINE_MULTISAMPLE_STATE_CREATE_INFO;
+        pipe_ms_state_ci_.pNext = NULL;
+        pipe_ms_state_ci_.rasterizationSamples = VK_SAMPLE_COUNT_4_BIT;


Why 4 samples by default??

krOoze · 2017-11-21T23:18:32Z

tests/layer_validation_tests.cpp

+    std::unique_ptr<VkShaderObj> vs_;
+    std::unique_ptr<VkShaderObj> fs_;
+    VkLayerTest *layer_test_;
+    CreatePipelineHelper(VkLayerTest *test) : layer_test_(test) {}


Would it make sense to pass by reference, not pointer?

krOoze · 2017-11-21T23:22:46Z

tests/layer_validation_tests.cpp

+        vkDestroyPipeline(device, pipeline_, nullptr);
+        vkDestroyPipelineLayout(device, pipeline_layout_, nullptr);
+        vkDestroyDescriptorSetLayout(device, ds_layout_, nullptr);
+        vkDestroyDescriptorPool(device, ds_pool_, nullptr);


Could alternatively use Vk*Objs for automatic destruction?
There's no Vk*LayoutObj yet, but I am slowly working on those in private branch...

Some of the Vk*Obj's have built-in smarts that might correct for intentionally induced errors. As easily induced intentional errors are a design goal, avoiding these for now.

krOoze · 2017-11-23T00:50:40Z

layers/vk_validation_error_database.txt

 VALIDATION_ERROR_10000b0e~^~N~^~None~^~VkPipelineMultisampleStateCreateInfo~^~VUID-VkPipelineMultisampleStateCreateInfo-rasterizationSamples-01415~^~(VK_NV_framebuffer_mixed_samples)~^~The spec valid usage text states 'If the subpass has any color attachments and rasterizationSamples is greater than the number of color samples, then sampleShadingEnable must be VK_FALSE' (https://www.khronos.org/registry/vulkan/specs/1.0-extensions/html/vkspec.html#VUID-VkPipelineMultisampleStateCreateInfo-rasterizationSamples-01415)~^~
 VALIDATION_ERROR_10009005~^~Y~^~Unknown~^~vkCmdSetViewport~^~VUID-VkPipelineMultisampleStateCreateInfo-flags-zerobitmask~^~core~^~The spec valid usage text states 'flags must be 0' (https://www.khronos.org/registry/vulkan/specs/1.0/html/vkspec.html#VUID-VkPipelineMultisampleStateCreateInfo-flags-zerobitmask)~^~implicit, TBD in parameter validation layer.
-VALIDATION_ERROR_1001c40d~^~Y~^~Unknown~^~vkCmdSetViewport~^~VUID-VkPipelineMultisampleStateCreateInfo-pNext-pNext~^~core~^~The spec valid usage text states 'Each pNext member of any structure (including this one) in the pNext chain must be either NULL or a pointer to a valid instance of VkPipelineCoverageModulationStateCreateInfoNV, VkPipelineCoverageToColorStateCreateInfoNV, or VkPipelineSampleLocationsStateCreateInfoEXT' (https://www.khronos.org/registry/vulkan/specs/1.0/html/vkspec.html#VUID-VkPipelineMultisampleStateCreateInfo-pNext-pNext)~^~implicit, TBD in parameter validation layer.
+VALIDATION_ERROR_1001c40d~^~Y~^~InvalidPipelineSamplePNext~^~vkCmdSetViewport~^~VUID-VkPipelineMultisampleStateCreateInfo-pNext-pNext~^~core~^~The spec valid usage text states 'Each pNext member of any structure (including this one) in the pNext chain must be either NULL or a pointer to a valid instance of VkPipelineCoverageModulationStateCreateInfoNV, VkPipelineCoverageToColorStateCreateInfoNV, or VkPipelineSampleLocationsStateCreateInfoEXT' (https://www.khronos.org/registry/vulkan/specs/1.0/html/vkspec.html#VUID-VkPipelineMultisampleStateCreateInfo-pNext-pNext)~^~implicit, TBD in parameter validation layer.


Is it still TBD? You seem to have implemented it...

There are improvements needed that will best be done in enhancements to code generation

krOoze · 2017-11-23T02:41:32Z

tests/layer_validation_tests.cpp

+    std::vector<VkDescriptorPoolSize> ds_pool_sizes_;
+    VkDescriptorPoolCreateInfo ds_pool_ci_ = {};
+    VkDescriptorPool ds_pool_ = VK_NULL_HANDLE;
+    std::vector<VkDescriptorSetLayoutBinding> dsl_bindings_;


could just be private static array?
the user can use his own vector, or modify the default array through the ds_layout_ci_.pBindings.

krOoze · 2017-11-23T02:43:32Z

tests/layer_validation_tests.cpp

+        ASSERT_VK_SUCCESS(err);
+    }
+
+    void LateBindPipelineInfo() {


Should be private? The CreateGraphicsPipeline seems to call it implicitly anyway, so probably no need to call it explicitly

krOoze · 2017-11-23T02:45:23Z

tests/layer_validation_tests.cpp

+        if (do_late_bind) {
+            LateBindPipelineInfo();
+        }
+        if (implicit_destroy && (pipeline_ != VK_NULL_HANDLE)) {


vkDestroy* should be able to destroy VK_NULL_HANDLE. Though there used to be some driver bugs, so probably better.

krOoze · 2017-11-23T02:50:38Z

tests/layer_validation_tests.cpp

+        }
+    }
+
+    VkResult CreateGraphicsPipeline(bool implicit_destroy = true, bool do_late_bind = true) {


implicit_destroy is bit confusing name. I did not really expect it to do that. Maybe destroy_previous_pipeline. Even so it would be weird the old pipeline to just get leaked.
Maybe just create the pipelines into a stack?

krOoze · 2017-11-23T02:56:12Z

tests/layer_validation_tests.cpp

+        info_override(helper);
+        helper.InitState();
+
+        test->Monitor()->SetDesiredFailureMsg(flags, error);


Does this work?
The positive tests usually set something like m_errorMonitor->ExpectSuccess();.

krOoze · 2017-11-23T03:09:07Z

tests/layer_validation_tests.cpp

+        rs_state_ci_.pNext = nullptr;
+        rs_state_ci_.flags = 0;
+        rs_state_ci_.depthClampEnable = VK_FALSE;
+        rs_state_ci_.rasterizerDiscardEnable = VK_FALSE;


This may be very commonly used setting. Might be worth to make some shortcut for setting VK_TRUE.

Added validation checks for: VUID-VkPipelineMultisampleStateCreateInfo-sampleShadingEnable-00784 (VALIDATION_ERROR_10000620) If the sample rate shading feature is not enabled, sampleShadingEnable must be VK_FALSE VUID-VkPipelineMultisampleStateCreateInfo-minSampleShading-00786 (VALIDATION_ERROR_10000624) minSampleShading must be in the range [0,1] Change-Id: I21cdaf99f8970df4f2e03026a4dae00746ad8386

Added tests for validation checks: VUID-VkPipelineMultisampleStateCreateInfo-sampleShadingEnable-00784 (VALIDATION_ERROR_10000620) VUID-VkPipelineMultisampleStateCreateInfo-minSampleShading-00786 (VALIDATION_ERROR_10000624) Change-Id: I49fcfc8f55275f27a976ea88619674c3b87285ae

Updated validation check for VUID-VkPipelineMultisampleStateCreateInfo-pNext-pNext (VALIDATION_ERROR_1001c40d) to include the valid extension pNext sType values valid for the current spec: VkPipelineCoverageModulationStateCreateInfoNV, VkPipelineCoverageToColorStateCreateInfoNV, or VkPipelineSampleLocationsStateCreateInfoEXT Change-Id: Ic3e054110c2bad4b44121a67e7e61e627fa9c559

Added test for validation checks of VUID-VkPipelineMultisampleStateCreateInfo-pNext-pNext (VALIDATION_ERROR_1001c40d) Change-Id: I41fb10698d88db1e4b143007c36b5a798e1adc6b

jzulauf-lunarg

I've incorporated a good deal of the feedback, haven't migrated to the Vk*Obj, as some of them have "clean up" code that might get in the way of inducing errors, which after all is the point of the layer_validation_tests.

Undoubtedly there will be further refinement each time a test (or tranche of tests) is converted to the Helper.

jzulauf-lunarg · 2017-11-27T19:23:55Z

tests/layer_validation_tests.cpp

@@ -173,13 +176,18 @@ struct NearestGreaterThan {
 };
 template <typename T, typename Comp>
 T NearestNonEqual(const T value, const Comp &comp) {


Just google'd this... android/ndk#82 -- thanks for the heads up.

jzulauf-lunarg · 2017-11-27T19:36:15Z

tests/layer_validation_tests.cpp

+    VkPipelineLayoutCreateInfo pipeline_layout_ci_ = {};
+    VkPipelineLayout pipeline_layout_ = VK_NULL_HANDLE;
+    VkPipelineViewportStateCreateInfo vp_state_ci_ = {};
+    std::vector<VkDynamicState> dyn_states_ = {VK_DYNAMIC_STATE_VIEWPORT, VK_DYNAMIC_STATE_SCISSOR, VK_DYNAMIC_STATE_LINE_WIDTH };


being lazy with other initialization.... fixing

jzulauf-lunarg · 2017-11-27T19:38:25Z

tests/layer_validation_tests.cpp

+        vkDestroyPipeline(device, pipeline_, nullptr);
+        vkDestroyPipelineLayout(device, pipeline_layout_, nullptr);
+        vkDestroyDescriptorSetLayout(device, ds_layout_, nullptr);
+        vkDestroyDescriptorPool(device, ds_pool_, nullptr);


Some of the Vk*Obj's have built-in smarts that might correct for intentionally induced errors. As easily induced intentional errors are a design goal, avoiding these for now.

jzulauf-lunarg · 2017-11-27T19:39:27Z

tests/layer_validation_tests.cpp

+        vkDestroyDescriptorPool(device, ds_pool_, nullptr);
+    }
+
+    void InitDescriptorPoolInfo() {


Using OneOffDescriptor, but the VkPipelineObj has some late binding smarts that could fixup intentional errors.

jzulauf-lunarg · 2017-11-27T19:40:36Z

tests/layer_validation_tests.cpp

+        alloc_info_.sType = VK_STRUCTURE_TYPE_DESCRIPTOR_SET_ALLOCATE_INFO;
+        alloc_info_.descriptorSetCount = 1;
+        alloc_info_.pSetLayouts = &ds_layout_;
+    }


This is monkey-see-monkey-do from several examples... assuming they'll find a use as I expand testcase adoption...

jzulauf-lunarg · 2017-11-30T18:39:38Z

Can I get a thumbs up or a set of requested changes?

mark-lunarg · 2017-11-30T18:41:53Z

LGTM.

jzulauf-lunarg requested review from chrisforbes, mark-lunarg and tobine November 20, 2017 17:01

jzulauf-lunarg self-assigned this Nov 20, 2017

jzulauf-lunarg requested a review from mikes-lunarg November 20, 2017 17:05

jzulauf-lunarg commented Nov 20, 2017

View reviewed changes

chrisforbes reviewed Nov 21, 2017

View reviewed changes

jzulauf-lunarg mentioned this pull request Nov 21, 2017

tests: Further pipeline viewport tests cleanup #2219

Merged

tobine reviewed Nov 21, 2017

View reviewed changes

krOoze reviewed Nov 23, 2017

View reviewed changes

jzulauf-lunarg added 4 commits November 27, 2017 20:36

tests: Tests for pipeline multisample pnext check

452b738

Added test for validation checks of VUID-VkPipelineMultisampleStateCreateInfo-pNext-pNext (VALIDATION_ERROR_1001c40d) Change-Id: I41fb10698d88db1e4b143007c36b5a798e1adc6b

jzulauf-lunarg force-pushed the zulauf_multisample_2180 branch from 12f5820 to 452b738 Compare November 28, 2017 19:03

jzulauf-lunarg commented Nov 28, 2017

View reviewed changes

jzulauf-lunarg merged commit 9fc644b into master Nov 30, 2017

This was referenced Dec 1, 2017

Add validation for sample rate shading #2180

Closed

pNext chain not correctly validated #2159

Closed

jzulauf-lunarg deleted the zulauf_multisample_2180 branch January 12, 2018 15:21

Fixes for pipeline multi sample issue 2159 and 2180 #2220

Fixes for pipeline multi sample issue 2159 and 2180 #2220

Conversation

jzulauf-lunarg commented Nov 20, 2017

jzulauf-lunarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krOoze Nov 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krOoze left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krOoze Nov 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzulauf-lunarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzulauf-lunarg commented Nov 30, 2017

mark-lunarg commented Nov 30, 2017

krOoze Nov 27, 2017 •

edited

krOoze left a comment •

edited

krOoze Nov 21, 2017 •

edited