Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VulkanDeviceAlloc: Memorytype per slab #11744

Merged
merged 2 commits into from
Jan 23, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Common/Vulkan/VulkanContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,14 @@ void VulkanContext::ChooseDevice(int physical_device) {

// This is as good a place as any to do this
vkGetPhysicalDeviceMemoryProperties(physical_devices_[physical_device_], &memory_properties);
ILOG("Memory Types (%d):", memory_properties.memoryTypeCount);
for (int i = 0; i < memory_properties.memoryTypeCount; i++) {
ILOG(" %d: Heap %d; Flags: %s%s%s%s ", i, memory_properties.memoryTypes[i].heapIndex,
(memory_properties.memoryTypes[i].propertyFlags & VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT) ? "DEVICE_LOCAL_BIT" : "",
(memory_properties.memoryTypes[i].propertyFlags & VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT) ? "HOST_VISIBLE_BIT" : "",
(memory_properties.memoryTypes[i].propertyFlags & VK_MEMORY_PROPERTY_HOST_CACHED_BIT) ? "HOST_CACHED_BIT" : "",
(memory_properties.memoryTypes[i].propertyFlags & VK_MEMORY_PROPERTY_HOST_COHERENT_BIT) ? "HOST_COHERENT_BIT" : "");
}

// Optional features
vkGetPhysicalDeviceFeatures(physical_devices_[physical_device_], &featuresAvailable_);
Expand Down
6 changes: 6 additions & 0 deletions Common/Vulkan/VulkanImage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ bool VulkanTexture::CreateDirect(VkCommandBuffer cmd, int w, int h, int numMips,
VkResult res = vkCreateImage(vulkan_->GetDevice(), &image_create_info, NULL, &image_);
if (res != VK_SUCCESS) {
_assert_(res == VK_ERROR_OUT_OF_HOST_MEMORY || res == VK_ERROR_OUT_OF_DEVICE_MEMORY || res == VK_ERROR_TOO_MANY_OBJECTS);
ELOG("vkCreateImage failed: %s", VulkanResultToString(res));
return false;
}

Expand All @@ -76,6 +77,8 @@ bool VulkanTexture::CreateDirect(VkCommandBuffer cmd, int w, int h, int numMips,
if (allocator_) {
offset_ = allocator_->Allocate(mem_reqs, &mem_, Tag());
if (offset_ == VulkanDeviceAllocator::ALLOCATE_FAILED) {
vkDestroyImage(vulkan_->GetDevice(), image_, nullptr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Destroy is called in the destructor which already does this. We should at least clear image_, but I'm personally not a big fan of repeating WET cleanup code in every error case, so I try to design things so they auto-cleanup and the code is easier to read.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WET? Yeah I see your point, I'll clean it up.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We Enjoy Typing. The opposite of DRY.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah :)

ELOG("Image memory allocation failed (mem_reqs.size = %d)", (int)mem_reqs.size);
return false;
}
} else {
Expand All @@ -89,6 +92,7 @@ bool VulkanTexture::CreateDirect(VkCommandBuffer cmd, int w, int h, int numMips,

res = vkAllocateMemory(vulkan_->GetDevice(), &mem_alloc, NULL, &mem_);
if (res != VK_SUCCESS) {
ELOG("vkAllocateMemory failed: %s", VulkanResultToString(res));
_assert_msg_(G3D, res != VK_ERROR_TOO_MANY_OBJECTS, "Too many Vulkan memory objects!");
_assert_(res == VK_ERROR_OUT_OF_HOST_MEMORY || res == VK_ERROR_OUT_OF_DEVICE_MEMORY || res == VK_ERROR_TOO_MANY_OBJECTS);
vkDestroyImage(vulkan_->GetDevice(), image_, nullptr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this one is also not necessary but at least clears image_ so is fine.

Expand All @@ -101,6 +105,7 @@ bool VulkanTexture::CreateDirect(VkCommandBuffer cmd, int w, int h, int numMips,

res = vkBindImageMemory(vulkan_->GetDevice(), image_, mem_, offset_);
if (res != VK_SUCCESS) {
ELOG("vkBindImageMemory failed: %s", VulkanResultToString(res));
// This leaks the image and memory. Should not really happen though...
_assert_(res == VK_ERROR_OUT_OF_HOST_MEMORY || res == VK_ERROR_OUT_OF_DEVICE_MEMORY || res == VK_ERROR_TOO_MANY_OBJECTS);
return false;
Expand Down Expand Up @@ -144,6 +149,7 @@ bool VulkanTexture::CreateDirect(VkCommandBuffer cmd, int w, int h, int numMips,

res = vkCreateImageView(vulkan_->GetDevice(), &view_info, NULL, &view_);
if (res != VK_SUCCESS) {
ELOG("vkCreateImageView failed: %s", VulkanResultToString(res));
// This leaks the image.
_assert_(res == VK_ERROR_OUT_OF_HOST_MEMORY || res == VK_ERROR_OUT_OF_DEVICE_MEMORY || res == VK_ERROR_TOO_MANY_OBJECTS);
return false;
Expand Down
18 changes: 8 additions & 10 deletions Common/Vulkan/VulkanMemory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -191,14 +191,8 @@ size_t VulkanDeviceAllocator::Allocate(const VkMemoryRequirements &reqs, VkDevic
assert(!destroyed_);
uint32_t memoryTypeIndex;
bool pass = vulkan_->MemoryTypeFromProperties(reqs.memoryTypeBits, VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT, &memoryTypeIndex);
assert(pass);
if (!pass) {
return ALLOCATE_FAILED;
}
if (memoryTypeIndex_ == UNDEFINED_MEMORY_TYPE) {
memoryTypeIndex_ = memoryTypeIndex;
} else if (memoryTypeIndex_ != memoryTypeIndex) {
assert(memoryTypeIndex_ == memoryTypeIndex);
ELOG("Failed to pick an appropriate memory type (req: %08x)", reqs.memoryTypeBits);
return ALLOCATE_FAILED;
}

Expand All @@ -213,6 +207,8 @@ size_t VulkanDeviceAllocator::Allocate(const VkMemoryRequirements &reqs, VkDevic
// This helps us "creep forward", and also spend less time allocating.
const size_t actualSlab = (lastSlab_ + i) % numSlabs;
Slab &slab = slabs_[actualSlab];
if (slab.memoryTypeIndex != memoryTypeIndex)
continue;
size_t start = slab.nextFree;

while (start < slab.usage.size()) {
Expand All @@ -227,7 +223,7 @@ size_t VulkanDeviceAllocator::Allocate(const VkMemoryRequirements &reqs, VkDevic
}

// Okay, we couldn't fit it into any existing slabs. We need a new one.
if (!AllocateSlab(size)) {
if (!AllocateSlab(size, memoryTypeIndex)) {
return ALLOCATE_FAILED;
}

Expand Down Expand Up @@ -401,16 +397,17 @@ void VulkanDeviceAllocator::ExecuteFree(FreeInfo *userdata) {
delete userdata;
}

bool VulkanDeviceAllocator::AllocateSlab(VkDeviceSize minBytes) {
bool VulkanDeviceAllocator::AllocateSlab(VkDeviceSize minBytes, int memoryTypeIndex) {
assert(!destroyed_);
if (!slabs_.empty() && minSlabSize_ < maxSlabSize_) {
// We're allocating an additional slab, so rachet up its size.
// TODO: Maybe should not do this when we are allocating a new slab due to memoryTypeIndex not matching?
minSlabSize_ <<= 1;
}

VkMemoryAllocateInfo alloc{ VK_STRUCTURE_TYPE_MEMORY_ALLOCATE_INFO };
alloc.allocationSize = minSlabSize_;
alloc.memoryTypeIndex = memoryTypeIndex_;
alloc.memoryTypeIndex = memoryTypeIndex;

while (alloc.allocationSize < minBytes) {
alloc.allocationSize <<= 1;
Expand All @@ -427,6 +424,7 @@ bool VulkanDeviceAllocator::AllocateSlab(VkDeviceSize minBytes) {

slabs_.resize(slabs_.size() + 1);
Slab &slab = slabs_[slabs_.size() - 1];
slab.memoryTypeIndex = memoryTypeIndex;
slab.deviceMemory = deviceMemory;
slab.usage.resize((size_t)(alloc.allocationSize >> SLAB_GRAIN_SHIFT));

Expand Down
4 changes: 2 additions & 2 deletions Common/Vulkan/VulkanMemory.h
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ class VulkanDeviceAllocator {

struct Slab {
VkDeviceMemory deviceMemory;
uint32_t memoryTypeIndex = UNDEFINED_MEMORY_TYPE;
std::vector<uint8_t> usage;
std::unordered_map<size_t, size_t> allocSizes;
std::unordered_map<size_t, UsageInfo> tags;
Expand All @@ -200,7 +201,7 @@ class VulkanDeviceAllocator {
freeInfo->allocator->ExecuteFree(freeInfo); // this deletes freeInfo
}

bool AllocateSlab(VkDeviceSize minBytes);
bool AllocateSlab(VkDeviceSize minBytes, int memoryTypeIndex);
bool AllocateFromSlab(Slab &slab, size_t &start, size_t blocks, const std::string &tag);
void Decimate();
void DoTouch(VkDeviceMemory deviceMemory, size_t offset);
Expand All @@ -212,6 +213,5 @@ class VulkanDeviceAllocator {
size_t lastSlab_ = 0;
size_t minSlabSize_;
const size_t maxSlabSize_;
uint32_t memoryTypeIndex_ = UNDEFINED_MEMORY_TYPE;
bool destroyed_ = false;
};
1 change: 1 addition & 0 deletions android/jni/AndroidVulkanContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ bool AndroidVulkanContext::InitAPI() {
g_Vulkan = nullptr;
return false;
}
ILOG("Vulkan device created!");
return true;
}

Expand Down