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

Clean up for "ExtensionNotEnabled" with prerequisite framework fixes. #320

Merged
merged 4 commits into from
Sep 12, 2018
Merged

Clean up for "ExtensionNotEnabled" with prerequisite framework fixes. #320

merged 4 commits into from
Sep 12, 2018

Conversation

jzulauf-lunarg
Copy link
Contributor

The promotion of extensions was being treated inconsistently by layers, framework, and tests, with the result being inconsistent errors between 1.0 and 1.1 drivers.

This set of changes unties this knot, cleaning up the tests that the fixes would otherwise break.

@jzulauf-lunarg
tests: Add ability for test to request API version
d46951c 
Add interface for version aware (i.e. > 1.0) tests to specify the
preferred api version.  Acutally version used is the lesser of the
specified version and the instance version.  Test must handle getting a
lower API version than requested gracefully.

Change-Id: I7b700cfdf2c691453c66e41cf843ffe55f426ad4
@jzulauf-lunarg
tests: Modify 1.1 aware tests to request 1.1. inst
3ecd90d 
Tests that are actually designed to by able to run at > 1.0 must declare
that ability to avoid validation errors, as subsequent changes will
enforce greater validation based on the appInfo.apiVersion supplied.
I.e. validation will attempt to validate to the given apiVersion not the
driver version.

Change-Id: I86ad83eb20cd26d890c3a379f78e58b97c49dee0
@jzulauf-lunarg
layers: Init extension tables w/ requested API
482a8b5 
Make the initialization of the extension enable tables consistent
between instance and device creation.  Validation will treat extension
presence based on the lesser of the requested API level and the driver
API level.

Change-Id: I453cbf4b7da35db22c4ffdf03fdf6e361d940b2f
@jzulauf-lunarg jzulauf-lunarg changed the title Clean up for "ExtentionNotEnabled" with prerequisite framework fixes. Clean up for "ExtensionNotEnabled" with prerequisite framework fixes. Sep 10, 2018
jzulauf-lunarg
jzulauf-lunarg commented Sep 10, 2018
View reviewed changes
Copy link
Contributor Author

@jzulauf-lunarg jzulauf-lunarg left a comment

Choose a reason for hiding this comment

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

With notes...

layers/core_validation.cpp
@@ -2310,8 +2310,10 @@ VKAPI_ATTR VkResult VKAPI_CALL CreateDevice(VkPhysicalDevice gpu, const VkDevice
// Get physical device limits for this device
instance_data->dispatch_table.GetPhysicalDeviceProperties(gpu, &(device_data->phys_dev_properties.properties));

device_data->api_version = device_data->extensions.InitFromDeviceCreateInfo(
&instance_data->extensions, device_data->phys_dev_properties.properties.apiVersion, pCreateInfo);
// Setup the validation tables based on the application API version from the instance and the capabilities of the device driver.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the driver version can be more or less than the instance version. For purposes of validation, if the application version is less than the driver version, we'll validate as if the driver was the lesser version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the instance supplied version is the lesser of the instances enumerated version and the apiVersion supplied to CreateInstance. (if the apiVersion is higher than the enumerated version, CreateInstance fails)

tests/layer_validation_tests.cpp
@@ -434,6 +434,8 @@ class VkLayerTest : public VkRenderFramework {

protected:
ErrorMonitor *m_errorMonitor;
uint32_t m_instance_api_version = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "instance" version is what enumerates... (or by failing to enumerate informs us of being 1.0). The "target version" is what the framework will try to create.

tests/layer_validation_tests.cpp
m_target_api_version = app_info.apiVersion;
}

uint32_t SetTargetApiVersion(uint32_t target_api_version) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not binding on the framework, the return is what CreateInstance will request.

tests/layer_validation_tests.cpp
@@ -22053,7 +22080,7 @@ TEST_F(VkLayerTest, CopyImageTypeExtentMismatchMaintenance1) {

TEST_F(VkLayerTest, CopyImageCompressedBlockAlignment) {
// Image copy tests on compressed images with block alignment errors

SetTargetApiVersion(VK_API_VERSION_1_1);
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 test really is 1.1 aware and thus should request 1.1. We don't care about what is request, as we test below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also the case for the other tests with this call added.

tests/layer_validation_tests.cpp
@@ -22124,7 +22151,7 @@ TEST_F(VkLayerTest, CopyImageCompressedBlockAlignment) {

std::string vuid;
bool ycbcr = (DeviceExtensionEnabled(VK_KHR_SAMPLER_YCBCR_CONVERSION_EXTENSION_NAME) ||
(m_device->props.apiVersion >= VK_API_VERSION_1_1));
(DeviceValidationVersion() >= VK_API_VERSION_1_1));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The call hides the logic matching the logic the validation uses for "how we will validate"

tests/layer_validation_tests.cpp
// Do NOT enable VK_KHR_maintenance1
if (DeviceExtensionSupported(gpu(), nullptr, VK_KHR_SAMPLER_YCBCR_CONVERSION_EXTENSION_NAME)) {
m_device_extension_names.push_back(VK_KHR_SAMPLER_YCBCR_CONVERSION_EXTENSION_NAME);
if (InstanceExtensionSupported(VK_KHR_GET_PHYSICAL_DEVICE_PROPERTIES_2_EXTENSION_NAME)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests is changed is to ensure that we get the same single error we expect on 1.0 and 1.1 drivers.

@jzulauf-lunarg jzulauf-lunarg force-pushed the zulauf_apiversion_exttest_fix branch 2 times, most recently from 2f61e6c to 42f7372 Compare September 11, 2018 16:50
@jzulauf-lunarg
tests: Cleanup unexpected in ExtensionNotEnabled
42f7372 
Reduce number of missing extensions to one for consistent, clean results
on 1.1 and 1.0.

Change-Id: Ic7a3ef6de16e604e700ed46766723214106c0e3f
@jzulauf-lunarg jzulauf-lunarg merged commit 000b38a into master Sep 12, 2018
@jzulauf-lunarg jzulauf-lunarg deleted the zulauf_apiversion_exttest_fix branch September 12, 2018 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisforbes chrisforbes chrisforbes approved these changes

@tobine tobine Awaiting requested review from tobine

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

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

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

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

@jeremyk-lunarg jeremyk-lunarg Awaiting requested review from jeremyk-lunarg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jzulauf-lunarg @chrisforbes