vkrunner: Add barriers required for copying an image to the host

Igalia / vkrunner

A shader script tester for Vulkan. Moved to https://gitlab.freedesktop.org/mesa/vkrunner

Other

43 stars 14 forks source link

vkrunner: Add barriers required for copying an image to the host #53

Closed paulthomson closed 5 years ago

paulthomson commented 5 years ago

Fixes #52 (although some barriers are probably still needed for ssbos).

Note that I have removed the automatic layout transitions for the color attachment and instead opted to keep the color attachment layout as VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL. We surround the image copy with VkImageMemoryBarriers to explicitly transition to VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL (and back) as well as ensuring sufficient synchronization. A VkBufferMemoryBarrier is also added to ensure the host can safely read the data that was copied to the buffer.

This needs careful review from those who have experience with barriers.

bpeel commented 5 years ago

Thanks for the patch. I think it looks good. I too made a similar patch locally, but in the end I couldn’t convince myself whether it was actually needed or not. However I’m happy to leave the decision to someone with more experience with barriers.

It would be nice if the patch could be modified to match the coding style of the rest of code base:

C-style / comments / instead of // C++
For a function with a lot of arguments like vkCmdPipelineBarrier, it would be good to put a short comment after each one to say what the argument name is.
Use under_scores instead of camelCase in the variable names.

paulthomson commented 5 years ago

Thanks. I made some improvements to the style. I also changed some of the barrier parameters, which I think is better. I will try to get this checked one more time just to be sure.

chrisforbes commented 5 years ago

Absolutely necessary. Paul: I think you're oversynchronizing slightly wrt later color attachment access; but it's fairly harmless.

paulthomson commented 5 years ago

Thanks @chrisforbes! Yeah I am happy to err on the side of caution here. In fact, we could have been much more cautious. @bpeel Let me know if you want more changes to the style. Otherwise, I am happy for this to be merged!

bpeel commented 5 years ago

Thanks to both of you. I have merged the patch. I went ahead and made a slight change so that null pointers are represented with NULL instead of 0 to match the rest of the code. I hope you don’t mind.

paulthomson commented 5 years ago

I hope you don’t mind.

Not at all! Thanks for the merge!