KhronosGroup / Vulkan-LoaderAndValidationLayers

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

Potentially false validation error if image size > maxResourceSize #2486

Closed pppessi closed 6 years ago

pppessi commented 6 years ago

Running dEQP-VK.pipeline.render_to_image.core.2d_array.huge.width_height_layers.r8g8b8a8_unorm produces a validation error:

ERROR: Object: 0x0 | CreateImage resource size exceeds allowable maximum Image resource size = 0x400000000, maximum resource size = 0x80000000 (code 0x00000000 from Image at Image:867)

when trying to create an image using the following info:

VkImageCreateInfo
{
    VK_STRUCTURE_TYPE_IMAGE_CREATE_INFO,
    0,
    0,
    VK_IMAGE_TYPE_2D,
    VK_FORMAT_R8G8B8A8_UNORM,
    { 4096, 4096, 1 },
    1,
    256,
    VK_SAMPLE_COUNT_1_BIT,
    VK_IMAGE_TILING_OPTIMAL,
    VK_IMAGE_USAGE_TRANSFER_SRC_BIT | VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT,
    VK_SHARING_MODE_EXCLUSIVE,
    0,
    0,
    VK_IMAGE_LAYOUT_UNDEFINED
}

I don't think this is quite right. Specification says:

There is no mechanism to query the size of an image before creating it, to compare that size against maxResourceSize. If an application attempts to create an image that exceeds this limit, the creation will fail and vkCreateImage will return VK_ERROR_OUT_OF_DEVICE_MEMORY.

Even if vkGetPhysicalDeviceImageFormatProperties2 returns success and the parameters to vkCreateImage are all within the returned limits, vkCreateImage must fail and return VK_ERROR_OUT_OF_DEVICE_MEMORY if the resulting size of the image would be larger than maxResourceSize.

On this device imageGranularity is 1 so it should not affect this in any way.

krOoze commented 6 years ago

4096 x 4096 x 256 x 4 = 0x4 0000 0000, which is greater than 0x8000 0000(=231), so that checks out.

It should be a WARNING, not ERROR, oterwise I don't see what the problem is?

tobine commented 6 years ago

Spec quote from @pppessi is out of date, potentially. Current language is:

There is no mechanism to query the size of an image before creating it, to compare that size against maxResourceSize. If an application attempts to create an image that exceeds this limit, the creation will fail or the image will be invalid. While the advertised limit must be at least 2^(31), it may not be possible to create an image that approaches that size, particularly for VK_IMAGE_TYPE_1D.

Since this causes either failure or an invalid image I like having it as an ERROR.

Tagging @chrisforbes for input as this is a deqp test.

krOoze commented 6 years ago

@tobine Is that unreleased version of the spec? Sounds like nonsense though; "no mechanism to query the size" vs "or the image will be invalid", which makes practically all images into quantum superposition of being valid and invalid.

tobine commented 6 years ago

Yes, I'm sorry. I was quoting the last version of the 1.0 spec. I see language @pppessi is quoting from the latest 1.1 spec.

Given this requirement:

Even if vkGetPhysicalDeviceImageFormatProperties2. returns success and the parameters to vkCreateImage are all within the returned limits, vkCreateImage must fail and return VK_ERROR_OUT_OF_DEVICE_MEMORY if the resulting size of the image would be larger than maxResourceSize.

I agree that the validation check should be changed to a warning as in #2487. Thanks!

krOoze commented 6 years ago

@tobine Right, it was apparently fixed recently in the 70, which is not exposed in 1.0 for technical reasons. The new quote is unguarded by macro though, so it applies to 1.0 too.

chrisforbes commented 6 years ago

@tobine I'm torn between converting this to a warning and getting rid of it entirely.

Reasons for the latter:

tobine commented 6 years ago

@chrisforbes good point. After a bit of thought I still slightly favor WARNING over killing the check. I just have a fear that some driver, esp. older ones, may not return error correctly so the warning is a nice backup. I agree that validation layers really shouldn't have to check the condition and even the existing check is just a guess, though, so I'm fine killing if that's your preference. For the near term I'll approve/push #2487, but if you want me to kill I can follow-on with another PR.

krOoze commented 6 years ago

The drivers return cookie-cutter error for the case. It is still nice to know the real reason for the error. Assuming some good lower-bound for the resource use can be "guessed". It might better fit one of the proposed "guidelines" layer, though trying to make a resource of > 231 bytes (or as in the OP even 16 GB) on a contemporary desktop could in of itself be considered likely error (i.e. warning).

chrisforbes commented 6 years ago

:+1:

tobine commented 6 years ago

WARNING update #2487 from @krOoze was pushed. Closing this issue. Thanks, everyone!