Skip to content

Commit

Permalink
layers: Clean up PHYSICAL_DEVICE_STATE usage in swapchains
Browse files Browse the repository at this point in the history 
Several fields in PHYSICAL_DEVICE_STATE are only used by
swapchain validation. Additionaly, the data in these fields
depends on both the physical device and the surface used,
so they really need to be looked up per swapchain.
  • Loading branch information
@jeremyg-lunarg
jeremyg-lunarg committed Sep 10, 2021
1 parent 383b9a3 commit c7a834a
Show file tree
Hide file tree
Showing 8 changed files with 56 additions and 193 deletions.
7 changes: 4 additions & 3 deletions layers/best_practices_utils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2834,13 +2834,12 @@ bool BestPractices::PreCallValidateGetPhysicalDeviceSurfaceFormatsKHR(VkPhysical
"vkGetPhysicalDeviceSurfaceFormatsKHR() called with non-NULL pSurfaceFormatCount; but no prior "
"positive value has been seen for pSurfaceFormats.");
} else {
auto prev_format_count = static_cast<uint32_t>(bp_pd_state->surface_formats.size());
if (*pSurfaceFormatCount > prev_format_count) {
if (*pSurfaceFormatCount > bp_pd_state->surface_formats_count) {
skip |= LogWarning(physicalDevice, kVUID_Core_DevLimit_CountMismatch,
"vkGetPhysicalDeviceSurfaceFormatsKHR() called with non-NULL pSurfaceFormatCount, and with "
"pSurfaceFormats set to a value (%u) that is greater than the value (%u) that was returned "
"when pSurfaceFormatCount was NULL.",
*pSurfaceFormatCount, prev_format_count);
*pSurfaceFormatCount, bp_pd_state->surface_formats_count);
}
}
return skip;
Expand Down Expand Up @@ -3725,6 +3724,7 @@ void BestPractices::ManualPostCallRecordGetPhysicalDeviceSurfaceFormatsKHR(VkPhy
if (call_state < QUERY_COUNT) {
call_state = QUERY_COUNT;
}
bp_pd_data->surface_formats_count = *pSurfaceFormatCount;
}
if (pSurfaceFormats) {
if (call_state < QUERY_DETAILS) {
Expand All @@ -3744,6 +3744,7 @@ void BestPractices::ManualPostCallRecordGetPhysicalDeviceSurfaceFormats2KHR(VkPh
if (bp_pd_data->vkGetPhysicalDeviceSurfaceFormatsKHRState < QUERY_COUNT) {
bp_pd_data->vkGetPhysicalDeviceSurfaceFormatsKHRState = QUERY_COUNT;
}
bp_pd_data->surface_formats_count = *pSurfaceFormatCount;
}
if (pSurfaceFormats) {
if (bp_pd_data->vkGetPhysicalDeviceSurfaceFormatsKHRState < QUERY_DETAILS) {
Expand Down
1 change: 1 addition & 0 deletions layers/best_practices_validation.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ struct PHYSICAL_DEVICE_STATE_BP : public PHYSICAL_DEVICE_STATE {
CALL_STATE vkGetPhysicalDeviceSurfaceCapabilitiesKHRState = UNCALLED;
CALL_STATE vkGetPhysicalDeviceSurfacePresentModesKHRState = UNCALLED;
CALL_STATE vkGetPhysicalDeviceSurfaceFormatsKHRState = UNCALLED;
uint32_t surface_formats_count = 0;
CALL_STATE vkGetPhysicalDeviceDisplayPlanePropertiesKHRState = UNCALLED;
};

Expand Down
97 changes: 40 additions & 57 deletions layers/core_validation.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13480,8 +13480,10 @@ bool CoreChecks::ValidateCreateSwapchain(const char *func_name, VkSwapchainCreat
}

auto physical_device_state = GetPhysicalDeviceState();
VkSurfaceCapabilitiesKHR capabilities{};
DispatchGetPhysicalDeviceSurfaceCapabilitiesKHR(physical_device_state->PhysDev(), pCreateInfo->surface, &capabilities);
bool skip = false;
VkSurfaceTransformFlagBitsKHR current_transform = physical_device_state->surfaceCapabilities.currentTransform;
VkSurfaceTransformFlagBitsKHR current_transform = capabilities.currentTransform;
if ((pCreateInfo->preTransform & current_transform) != pCreateInfo->preTransform) {
skip |= LogPerformanceWarning(physical_device, kVUID_Core_Swapchain_PreTransform,
"%s: pCreateInfo->preTransform (%s) doesn't match the currentTransform (%s) returned by "
Expand All @@ -13495,8 +13497,6 @@ bool CoreChecks::ValidateCreateSwapchain(const char *func_name, VkSwapchainCreat
const bool shared_present_mode = (VK_PRESENT_MODE_SHARED_DEMAND_REFRESH_KHR == present_mode ||
VK_PRESENT_MODE_SHARED_CONTINUOUS_REFRESH_KHR == present_mode);

VkSurfaceCapabilitiesKHR capabilities{};
DispatchGetPhysicalDeviceSurfaceCapabilitiesKHR(physical_device_state->PhysDev(), pCreateInfo->surface, &capabilities);
// Validate pCreateInfo->minImageCount against VkSurfaceCapabilitiesKHR::{min|max}ImageCount:
// Shared Present Mode must have a minImageCount of 1
if ((pCreateInfo->minImageCount < capabilities.minImageCount) && !shared_present_mode) {
Expand Down Expand Up @@ -13621,26 +13621,18 @@ bool CoreChecks::ValidateCreateSwapchain(const char *func_name, VkSwapchainCreat
}
}

std::vector<VkSurfaceFormatKHR> surface_formats;
const auto *surface_formats_ref = &surface_formats;

// Validate pCreateInfo values with the results of vkGetPhysicalDeviceSurfaceFormatsKHR():
if (physical_device_state->surface_formats.empty()) {
uint32_t surface_format_count = 0;
DispatchGetPhysicalDeviceSurfaceFormatsKHR(physical_device, pCreateInfo->surface, &surface_format_count, nullptr);
surface_formats.resize(surface_format_count);
DispatchGetPhysicalDeviceSurfaceFormatsKHR(physical_device, pCreateInfo->surface, &surface_format_count,
&surface_formats[0]);
} else {
surface_formats_ref = &physical_device_state->surface_formats;
}

std::vector<VkSurfaceFormatKHR> surface_formats;
uint32_t surface_format_count = 0;
DispatchGetPhysicalDeviceSurfaceFormatsKHR(physical_device, pCreateInfo->surface, &surface_format_count, nullptr);
surface_formats.resize(surface_format_count);
DispatchGetPhysicalDeviceSurfaceFormatsKHR(physical_device, pCreateInfo->surface, &surface_format_count, &surface_formats[0]);
{
// Validate pCreateInfo->imageFormat against VkSurfaceFormatKHR::format:
bool found_format = false;
bool found_color_space = false;
bool found_match = false;
for (const auto &format : *surface_formats_ref) {
for (const auto &format : surface_formats) {
if (pCreateInfo->imageFormat == format.format) {
// Validate pCreateInfo->imageColorSpace against VkSurfaceFormatKHR::colorSpace:
found_format = true;
Expand Down Expand Up @@ -13672,23 +13664,17 @@ bool CoreChecks::ValidateCreateSwapchain(const char *func_name, VkSwapchainCreat
}
}

std::vector<VkPresentModeKHR> present_modes;
const auto *present_modes_ref = &present_modes;

// Validate pCreateInfo values with the results of vkGetPhysicalDeviceSurfacePresentModesKHR():
if (physical_device_state->present_modes.empty()) {
uint32_t present_mode_count = 0;
DispatchGetPhysicalDeviceSurfacePresentModesKHR(physical_device_state->PhysDev(), pCreateInfo->surface,
&present_mode_count, nullptr);
present_modes.resize(present_mode_count);
DispatchGetPhysicalDeviceSurfacePresentModesKHR(physical_device_state->PhysDev(), pCreateInfo->surface,
&present_mode_count, &present_modes[0]);
} else {
present_modes_ref = &physical_device_state->present_modes;
}
std::vector<VkPresentModeKHR> present_modes;
uint32_t present_mode_count = 0;
DispatchGetPhysicalDeviceSurfacePresentModesKHR(physical_device_state->PhysDev(), pCreateInfo->surface, &present_mode_count,
nullptr);
present_modes.resize(present_mode_count);
DispatchGetPhysicalDeviceSurfacePresentModesKHR(physical_device_state->PhysDev(), pCreateInfo->surface, &present_mode_count,
&present_modes[0]);

// Validate pCreateInfo->presentMode against vkGetPhysicalDeviceSurfacePresentModesKHR():
bool found_match = std::find(present_modes_ref->begin(), present_modes_ref->end(), present_mode) != present_modes_ref->end();
bool found_match = std::find(present_modes.begin(), present_modes.end(), present_mode) != present_modes.end();
if (!found_match) {
if (LogError(device, "VUID-VkSwapchainCreateInfoKHR-presentMode-01281",
"%s called with a non-supported presentMode (i.e. %s).", func_name, string_VkPresentModeKHR(present_mode))) {
Expand Down Expand Up @@ -14151,34 +14137,31 @@ bool CoreChecks::ValidateAcquireNextImage(VkDevice device, const CommandVersion
func_name);
}

auto physical_device_state = GetPhysicalDeviceState();
// TODO: this is technically wrong on many levels, but requires massive cleanup
if (physical_device_state->vkGetPhysicalDeviceSurfaceCapabilitiesKHR_called) {
const uint32_t acquired_images = static_cast<uint32_t>(
std::count_if(swapchain_data->images.begin(), swapchain_data->images.end(),
[](const SWAPCHAIN_IMAGE &image) { return (image.image_state && image.image_state->acquired); }));

const uint32_t swapchain_image_count = static_cast<uint32_t>(swapchain_data->images.size());
const auto min_image_count = physical_device_state->surfaceCapabilities.minImageCount;
const bool too_many_already_acquired = acquired_images > swapchain_image_count - min_image_count;
if (timeout == UINT64_MAX && too_many_already_acquired) {
const char *vuid = "INVALID-vuid";
if (cmd_version == CMD_VERSION_1) {
vuid = "VUID-vkAcquireNextImageKHR-swapchain-01802";
} else if (cmd_version == CMD_VERSION_2) {
vuid = "VUID-vkAcquireNextImage2KHR-swapchain-01803";
} else {
assert(false);
}

const uint32_t acquirable = swapchain_image_count - min_image_count + 1;
skip |= LogError(swapchain, vuid,
"%s: Application has already previously acquired %" PRIu32 " image%s from swapchain. Only %" PRIu32
" %s available to be acquired using a timeout of UINT64_MAX (given the swapchain has %" PRIu32
", and VkSurfaceCapabilitiesKHR::minImageCount is %" PRIu32 ").",
func_name, acquired_images, acquired_images > 1 ? "s" : "", acquirable,
acquirable > 1 ? "are" : "is", swapchain_image_count, min_image_count);
const uint32_t acquired_images = static_cast<uint32_t>(
std::count_if(swapchain_data->images.begin(), swapchain_data->images.end(),
[](const SWAPCHAIN_IMAGE &image) { return (image.image_state && image.image_state->acquired); }));

const uint32_t swapchain_image_count = static_cast<uint32_t>(swapchain_data->images.size());
const auto min_image_count = swapchain_data->surface_capabilities.minImageCount;
const bool too_many_already_acquired = acquired_images > swapchain_image_count - min_image_count;
if (timeout == UINT64_MAX && too_many_already_acquired) {
const char *vuid = "INVALID-vuid";
if (cmd_version == CMD_VERSION_1) {
vuid = "VUID-vkAcquireNextImageKHR-swapchain-01802";
} else if (cmd_version == CMD_VERSION_2) {
vuid = "VUID-vkAcquireNextImage2KHR-swapchain-01803";
} else {
assert(false);
}

const uint32_t acquirable = swapchain_image_count - min_image_count + 1;
skip |= LogError(swapchain, vuid,
"%s: Application has already previously acquired %" PRIu32 " image%s from swapchain. Only %" PRIu32
" %s available to be acquired using a timeout of UINT64_MAX (given the swapchain has %" PRIu32
", and VkSurfaceCapabilitiesKHR::minImageCount is %" PRIu32 ").",
func_name, acquired_images, acquired_images > 1 ? "s" : "", acquirable, acquirable > 1 ? "are" : "is",
swapchain_image_count, min_image_count);
}
}
return skip;
Expand Down
9 changes: 2 additions & 7 deletions layers/device_state.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,18 +99,13 @@ class PHYSICAL_DEVICE_STATE : public BASE_NODE {
public:
uint32_t queue_family_known_count = 1; // spec implies one QF must always be supported
const std::vector<VkQueueFamilyProperties> queue_family_properties;
VkSurfaceCapabilitiesKHR surfaceCapabilities = {};
std::vector<VkPresentModeKHR> present_modes;
std::vector<VkSurfaceFormatKHR> surface_formats;
// TODO These are currently used by CoreChecks, but should probably be refactored
bool vkGetPhysicalDeviceDisplayPlanePropertiesKHR_called = false;
uint32_t display_plane_property_count = 0;

// Map of queue family index to QUEUE_FAMILY_PERF_COUNTERS
layer_data::unordered_map<uint32_t, std::unique_ptr<QUEUE_FAMILY_PERF_COUNTERS>> perf_counters;

// TODO These are currently used by CoreChecks, but should probably be refactored
bool vkGetPhysicalDeviceSurfaceCapabilitiesKHR_called = false;
bool vkGetPhysicalDeviceDisplayPlanePropertiesKHR_called = false;

PHYSICAL_DEVICE_STATE(VkPhysicalDevice phys_dev)
: BASE_NODE(phys_dev, kVulkanObjectTypePhysicalDevice), queue_family_properties(GetQueueFamilyProps(phys_dev)) {}

Expand Down
9 changes: 8 additions & 1 deletion layers/image_state.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,12 @@ static safe_VkImageCreateInfo GetImageCreateInfo(const VkSwapchainCreateInfoKHR
return safe_VkImageCreateInfo(&image_ci);
}

static VkSurfaceCapabilitiesKHR GetSurfaceCaps(VkPhysicalDevice physical_device, VkSurfaceKHR surface) {
VkSurfaceCapabilitiesKHR result{};
DispatchGetPhysicalDeviceSurfaceCapabilitiesKHR(physical_device, surface, &result);
return result;
}

SWAPCHAIN_NODE::SWAPCHAIN_NODE(ValidationStateTracker *dev_data_, const VkSwapchainCreateInfoKHR *pCreateInfo,
VkSwapchainKHR swapchain)
: BASE_NODE(swapchain, kVulkanObjectTypeSwapchainKHR),
Expand All @@ -448,7 +454,8 @@ SWAPCHAIN_NODE::SWAPCHAIN_NODE(ValidationStateTracker *dev_data_, const VkSwapch
get_swapchain_image_count(0),
max_present_id(0),
image_create_info(GetImageCreateInfo(pCreateInfo)),
dev_data(dev_data_) {}
dev_data(dev_data_),
surface_capabilities(GetSurfaceCaps(dev_data->physical_device, pCreateInfo->surface)) {}

void SWAPCHAIN_NODE::PresentImage(uint32_t image_index) {
if (image_index >= images.size()) return;
Expand Down
1 change: 1 addition & 0 deletions layers/image_state.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ class SWAPCHAIN_NODE : public BASE_NODE {
const safe_VkImageCreateInfo image_create_info;
std::shared_ptr<SURFACE_STATE> surface;
ValidationStateTracker *dev_data;
const VkSurfaceCapabilitiesKHR surface_capabilities;

SWAPCHAIN_NODE(ValidationStateTracker *dev_data, const VkSwapchainCreateInfoKHR *pCreateInfo, VkSwapchainKHR swapchain);

Expand Down
Loading

0 comments on commit c7a834a

Please sign in to comment.