KhronosGroup / Vulkan-LoaderAndValidationLayers

**Deprecated repository** for Vulkan loader and validation layers
Apache License 2.0
414 stars 172 forks source link

layers: add missing VkFormatFeatureFlags checks #2642

Closed cdwfs closed 6 years ago

cdwfs commented 6 years ago

Checks for TRANSFER_SRC / TRANSFER_DST format features were added to:

Existing format feature checks were adapted to use the new helper function:

cdwfs commented 6 years ago

I found the error here; will push a fix shortly.

cdwfs commented 6 years ago

Having implemented these checks, I'm not convinced they're useful. I haven't found a way to create an image that fails at copy/resolve/blit /clear time (due to missing TRANSFER support in the format features) that doesn't also fail at create time due to missing support for TRANSFER usages. These VUs were added in KHR_maintenance1, and I assume there was a good reason; does anybody happen to know the expected (mis)use case that wasn't covered by existing checks?

tobine commented 6 years ago

Based on your description it does sound like they're redundant and I don't know of expected misuse cases that would hit at use-time and not at create time. Your code updates LGTM. Thinking about how best to proceed... Seems like two best options are to either kill the redundant VUs along w/ checks OR to leave as-is and live with probable redundancy. After a bit of thought it makes sense to me to have the VUs in the use functions as it can be helpful to someone just referencing those functions in the spec. And if the VUs make sense in the spec I think we should check them in validation. The downside of a redundant check is extra code/complexity. For this code, at least, that price is small and my feeling is that it reasonable to add these checks.

Putting this comment out there for any contrary opinions. Anyone have examples of where these bad usages wouldn't be hit at creation time? Otherwise does anyone feel strongly that these checks are redundant and should be killed in spec?

mark-lunarg commented 6 years ago

@cdwfs, this repository will close for write access on Sunday, 5/13/2018. If it is pushed before that time it will be present in the follow-on Vulkan-ValidationLayers repository on Monday, otherwise a new PR will be required in the new repo.

cdwfs commented 6 years ago

Thanks Mark, I'd been wondering when the cutoff was. I have some other PRs in progress that won't be ready before then, so I'll prepare them for migration next week.

I'll give this PR another couple days to see if anybody wants to disagree with Tobin's conclusion. Do you have a strong opinion one way or the other?

mark-lunarg commented 6 years ago

@cdwfs, add the checks. If it's in the spec, they should be there.