Validation layer does not like AMD memory allocation strategy on Intel IGP?

[reduz]

I'm doing the following:

1. Create an Image on GPU memory with layout undefined
2. Queue a barrier change to transfer dst optimal
3. Use a stanging buffer to put image data and queue a blit to the image

I get this error:

ERROR: _debug_messenger_callback: WARNING : VALIDATION - Message Id Number: 0 | Message Id Name: UNASSIGNED-CoreValidation-DrawState-InvalidImageLayout
	Mapping an image with layout VK_IMAGE_LAYOUT_UNDEFINED can result in undefined behavior if this memory is used by the device. Only GENERAL or PREINITIALIZED should be used.

	Objects - 1
		Object[0] - VK_OBJECT_TYPE_DEVICE_MEMORY, Handle 0x18

I have the feeling that AMD allocator is putting both the image and the staging buffer together in host memory due to this being an IGP. Still, I don't know how to work around this warning so.. what is the recommended strategy?

[reduz]

I just found that the AMD memory allocator example traps this error and ignores it.. It does not make me happy to do this..

https://github.com/GPUOpen-LibrariesAndSDKs/VulkanMemoryAllocator/blob/master/src/VulkanSample.cpp#L205

[reduz]

To be honest I don't think this should be a warning at all, given how limited Vulkan default memory allocation support is.

[TonyBarbour]

This doesn't happen just with integrated graphics, it behaves the same way on my discrete graphics.
The VMA makes a large allocation that it subdivides as buffers and images are created. When a request is made to map the memory bound to a buffer or image, the entire allocation is mapped and a pointer to that object's memory within that allocation is returned, This could be to avoid the alignment restrictions when mapping arbitrarily sized memory allocations, but that's just speculation. I found nowhere in the spec that prohibits this behavior, so I agree that this probably shouldn't be a warning. Adding @tobine to get his opinion.

[TonyBarbour]

Vulkan only lets you map a memory allocation once, necessitating VMA mapping the whole allocation rather than just a subrange.

[mark-lunarg]

@tobine, can we nuke this warning, or move it to the Assistant layer maybe?

[tobine]

After reviewing the spec a few times I'm not certain what motivated that warning. I suspect the original motivation for the warning has since changed in the spec. I'm fine with just nuking it.

[mark-lunarg]

PR coming.

[mark-lunarg]

As mentioned in the abandoned PR #1006, this warning is actually called out in the spec:

Buffers, and linear image subresources in either the VK_IMAGE_LAYOUT_PREINITIALIZED or
VK_IMAGE_LAYOUT_GENERAL layouts, are host-accessible subresources. That is, the host has a well-defined addressing scheme to interpret the contents, and thus the layout of the data in memory can
be consistently interpreted across aliases if each of those aliases is a host-accessible subresource.
Non-linear images, and linear image subresources in other layouts, are not host-accessible.

So the consensus is to leave the warning as-is.

[reduz]

So, should I then report this as a bug to AMD memory allocation library? I mean, either them, the spec or the validation layer has to be wrong here..

[jeffbolznv]

IMO it is a validation layer bug if this is reported as an error, because there really is no such error in Vulkan. If it is reported a warning that's less objectionable, but it's the kind of thing where it adds more confusion than it helps.

[mark-lunarg]

As @Tony-LunarG described in his comments, this might be one of those things that the memory manager needs to do even though it generates valid warnings. But yes, that might be the best place to address it, as the layers need to cover the operation of all applications.

[mark-lunarg]

@jeffbolznv, it is a warning. I'll run it past @tobine again -- I feel similarly about all the warnings - they usually upset just as many folks as they help.

[reduz]

Opened an issue on the AMD memory allocator:
GPUOpen-LibrariesAndSDKs/VulkanMemoryAllocator#64

[darksylinc]

HOLD ON A SECOND HERE!

I see 5 distinct problems here:

1. Whether this behavior is avoidable or not. It is not avoidable because Vulkan forces to map the entire region. And if there's only one queue, we will be forced to map non-host-accessible resources.
2. Whether it is valid to map non-host-accessible resources as long as we don't read or write from/to it by the host, or if it is entirely invalid to even map this region. I suspect it is the former (valid to map, invalid to access by the host)
3. Whether this message from the validator should be a warning or an error. It is currently an "error", and this depends on whether point 2 says it's invalid to map, or it is valid to map but invalid to access the region.
4. The wording of the error message currently indicates it is invalid to map. This should be clarified if point 2 is solved as valid to map, invalid to access.
5. If point 2 is solved as "it is invalid to map", then the spec has a huge problem. Because Vulkan implementations may only provide 1 queue (point 1).

I suspect it is valid to map but invalid to access, and if that's the case then the error message should be changed to a warning, and the wording of the message should be changed to be more clear. Right now it indicates it is invalid to map if the device uses this region, regardless of whether the host touches the region.

If it's invalid to map, the Vulkan API in the spect has a serious problem.

[mark-lunarg]

@darksylinc, it is a warning, see closed PR #1006, or the original issue description at the top of these comments. If we don't remove the warning, then perhaps a rewording will help.

[jeffbolznv]

@jeffbolznv, it is a warning. I'll run it past @tobine again -- I feel similarly about all the warnings - they usually upset just as many folks as they help.

I know the warnings are meant to be helpful, but it seems like the trend is "if something is valid, somebody will do it, and the warnings will be considered a bug".

As for what the spec says, my interpretation is "it's valid to map, it's even valid to access, you just get undefined data in your image". The mapping needs to be allowed in order to share memory with buffers and I think it's weird to flag it as a warning just because some image is bound to the memory.

[darksylinc]

@mark-lunarg I see. I re-read the log and it is indeed a warning. I am not advocating for removing the warning. And indeed, a rewording would help, since the warning implies it is invalid to even map; whereas it is more likely that it is illegal to touch that region of memory from the CPU if the device is using it.

[mark-lunarg]

@jeffbolznv, I agree entirely, will advocate again for removal, and need to build some consensus with some other stakeholders -- thanks again for the information and support.

[reduz]

My view on this is that warnings are useful when they advocate best practices. This seems to have been so far the case with all the other warnings I have found in the validation layer, but it doesn't seem to be the case here.

[reduz]

According to @adam-sawicki-amd on his comment below, there isn't any possible best practice here to follow, as this is per Vulkan design:

GPUOpen-LibrariesAndSDKs/VulkanMemoryAllocator#64 (comment)

If this is the case, then I'm not sure this is useful as a warning, I believe it should be something of less importance.

[mark-lunarg]

We have removed this warning from corechecks and will add it to the assistant layer (see tracking issue #24), where best-practices and API pitfalls are collected together. Thanks for all the feedback.

[adam-sawicki-a]

My understanding is that:

• It is valid to map entire memory block that contains e.g. a depth-stencil image, it's just the data of the memory region that spans the image that shouldn't be accessed because results are undefined.
• It is impossible to avoid allocating large memory blocks and sub-allocation of resources in any nontrivial application because of the maximum allocation number limit.
• Putting mappable buffers in a separate memory blocks from textures or depth-stencil images would allow to avoid the warning, but there is no good reason to do it.

So I support removal of this warning. Thanks Mark for handling this!