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

Support Vulkan requirement to allow write or copy beyond descriptor binding size. #2303

Merged
merged 1 commit into from
Aug 6, 2024
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
2 changes: 2 additions & 0 deletions Docs/Whats_New.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ Released TBD
- `vkAllocateDescriptorSets()`: Per Vulkan spec, if any descriptor set allocation
fails, populate all descriptor set pointers with `VK_NULL_HANDLE`. In addition,
return `VK_ERROR_FRAGMENTED_POOL` if failure was due to pool fragmentation.
- `vkUpdateDescriptorSets()`: Per Vulkan spec, allow write or copy beyond the
end of a descriptor binding count, including inline uniform block descriptors.
- Fix rendering issue with render pass that immediately follows a kernel dispatch.
- Ensure all MoltenVK config info set by `VK_EXT_layer_settings` is used.
- Move primitive-restart-disabled warning from renderpass to pipeline creation, to reduce voluminous log noise.
Expand Down
1 change: 0 additions & 1 deletion MoltenVK/MoltenVK/Commands/MVKMTLBufferAllocation.mm
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@
}
}


MVKMTLBufferAllocationPool::MVKMTLBufferAllocationPool(MVKDevice* device, NSUInteger allocationLength, bool makeThreadSafe,
bool isDedicated, MTLStorageMode mtlStorageMode) :
MVKObjectPool<MVKMTLBufferAllocation>(true),
Expand Down
23 changes: 19 additions & 4 deletions MoltenVK/MoltenVK/GPUObjects/MVKDescriptor.h
Original file line number Diff line number Diff line change
Expand Up @@ -381,18 +381,33 @@ class MVKInlineUniformBlockDescriptor : public MVKDescriptor {

void write(MVKDescriptorSetLayoutBinding* mvkDSLBind,
MVKDescriptorSet* mvkDescSet,
uint32_t dstOffset, // Inline buffers use this parameter as an offset, not an index
uint32_t dstIdx,
uint32_t srcIdx,
size_t srcStride,
const void* pData) override;
const void* pData) override {}

void read(MVKDescriptorSetLayoutBinding* mvkDSLBind,
MVKDescriptorSet* mvkDescSet,
uint32_t srcOffset, // For inline buffers we are using this parameter as src offset not as dst descIdx
uint32_t dstIndex,
VkDescriptorImageInfo* pImageInfo,
VkDescriptorBufferInfo* pBufferInfo,
VkBufferView* pTexelBufferView,
VkWriteDescriptorSetInlineUniformBlockEXT* inlineUniformBlock) override;
VkWriteDescriptorSetInlineUniformBlockEXT* inlineUniformBlock) override {}

uint32_t writeBytes(MVKDescriptorSetLayoutBinding* mvkDSLBind,
MVKDescriptorSet* mvkDescSet,
uint32_t dstOffset,
uint32_t srcOffset,
uint32_t byteCount,
const VkWriteDescriptorSetInlineUniformBlockEXT* pInlineUniformBlock);

uint32_t readBytes(MVKDescriptorSetLayoutBinding* mvkDSLBind,
MVKDescriptorSet* mvkDescSet,
uint32_t dstOffset,
uint32_t srcOffset,
uint32_t byteCount,
const VkWriteDescriptorSetInlineUniformBlockEXT* pInlineUniformBlock);


void encodeResourceUsage(MVKResourcesCommandEncoderState* rezEncState,
MVKDescriptorSetLayoutBinding* mvkDSLBind,
Expand Down
65 changes: 39 additions & 26 deletions MoltenVK/MoltenVK/GPUObjects/MVKDescriptor.mm
Original file line number Diff line number Diff line change
Expand Up @@ -898,21 +898,26 @@ void mvkPopulateShaderConversionConfig(mvk::SPIRVToMSLConversionConfiguration& s
}
}

void MVKInlineUniformBlockDescriptor::write(MVKDescriptorSetLayoutBinding* mvkDSLBind,
MVKDescriptorSet* mvkDescSet,
uint32_t dstOffset,
uint32_t srcIdx,
size_t srcStride,
const void* pData) {
uint32_t MVKInlineUniformBlockDescriptor::writeBytes(MVKDescriptorSetLayoutBinding* mvkDSLBind,
MVKDescriptorSet* mvkDescSet,
uint32_t dstOffset,
uint32_t srcOffset,
uint32_t byteCount,
const VkWriteDescriptorSetInlineUniformBlockEXT* pInlineUniformBlock) {
uint32_t dataLen = 0;
uint32_t dstBuffSize = mvkDSLBind->_info.descriptorCount;
uint32_t srcBuffSize = pInlineUniformBlock->dataSize;
if (dstOffset < dstBuffSize && srcOffset < srcBuffSize) {
dataLen = std::min({ byteCount, dstBuffSize - dstOffset, srcBuffSize - srcOffset });
}

// Ensure there is a destination to write to
uint32_t buffSize = mvkDSLBind->_info.descriptorCount;
if ( !_mvkMTLBufferAllocation ) { _mvkMTLBufferAllocation = mvkDescSet->acquireMTLBufferRegion(buffSize); }

uint8_t* data = getData();
const auto& pInlineUniformBlock = *(VkWriteDescriptorSetInlineUniformBlockEXT*)pData;
if (data && pInlineUniformBlock.pData && dstOffset < buffSize) {
uint32_t dataLen = std::min(pInlineUniformBlock.dataSize, buffSize - dstOffset);
memcpy(data + dstOffset, pInlineUniformBlock.pData, dataLen);
if ( !_mvkMTLBufferAllocation ) { _mvkMTLBufferAllocation = mvkDescSet->acquireMTLBufferRegion(dstBuffSize); }

uint8_t* pDstData = getData();
uint8_t* pSrcData = (uint8_t*)pInlineUniformBlock->pData;
if (pDstData && pSrcData && dataLen) {
memcpy(pDstData + dstOffset, pSrcData + srcOffset, dataLen);
}

// Write resource to Metal argument buffer
Expand All @@ -923,21 +928,29 @@ void mvkPopulateShaderConversionConfig(mvk::SPIRVToMSLConversionConfiguration& s
_mvkMTLBufferAllocation ? _mvkMTLBufferAllocation->_offset : 0,
argIdx);
}

return dataLen;
}

void MVKInlineUniformBlockDescriptor::read(MVKDescriptorSetLayoutBinding* mvkDSLBind,
MVKDescriptorSet* mvkDescSet,
uint32_t srcOffset,
VkDescriptorImageInfo* pImageInfo,
VkDescriptorBufferInfo* pBufferInfo,
VkBufferView* pTexelBufferView,
VkWriteDescriptorSetInlineUniformBlockEXT* pInlineUniformBlock) {
uint8_t* data = getData();
uint32_t buffSize = mvkDSLBind->_info.descriptorCount;
if (data && pInlineUniformBlock->pData && srcOffset < buffSize) {
uint32_t dataLen = std::min(pInlineUniformBlock->dataSize, buffSize - srcOffset);
memcpy((void*)pInlineUniformBlock->pData, data + srcOffset, dataLen);
uint32_t MVKInlineUniformBlockDescriptor::readBytes(MVKDescriptorSetLayoutBinding* mvkDSLBind,
MVKDescriptorSet* mvkDescSet,
uint32_t dstOffset,
uint32_t srcOffset,
uint32_t byteCount,
const VkWriteDescriptorSetInlineUniformBlockEXT* pInlineUniformBlock) {
uint32_t dataLen = 0;
uint32_t dstBuffSize = pInlineUniformBlock->dataSize;
uint32_t srcBuffSize = mvkDSLBind->_info.descriptorCount;
if (dstOffset < dstBuffSize && srcOffset < srcBuffSize) {
dataLen = std::min({ byteCount, dstBuffSize - dstOffset, srcBuffSize - srcOffset });
}

uint8_t* pDstData = (uint8_t*)pInlineUniformBlock->pData;
uint8_t* pSrcData = getData();
if (pDstData && pSrcData && dataLen) {
memcpy(pDstData + dstOffset, pSrcData + srcOffset, dataLen);
}
return dataLen;
}

void MVKInlineUniformBlockDescriptor::encodeResourceUsage(MVKResourcesCommandEncoderState* rezEncState,
Expand Down
2 changes: 1 addition & 1 deletion MoltenVK/MoltenVK/GPUObjects/MVKDescriptorSet.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ class MVKDescriptorSetLayout : public MVKVulkanAPIDeviceObject {
void propagateDebugName() override {}
uint32_t getDescriptorCount(uint32_t variableDescriptorCount);
uint32_t getDescriptorIndex(uint32_t binding, uint32_t elementIndex = 0) { return getBinding(binding)->getDescriptorIndex(elementIndex); }
MVKDescriptorSetLayoutBinding* getBinding(uint32_t binding) { return &_bindings[_bindingToIndex[binding]]; }
MVKDescriptorSetLayoutBinding* getBinding(uint32_t binding, uint32_t bindingIndexOffset = 0);
uint32_t getBufferSizeBufferArgBuferIndex() { return 0; }
id <MTLArgumentEncoder> getMTLArgumentEncoder(uint32_t variableDescriptorCount);
uint64_t getMetal3ArgumentBufferEncodedLength(uint32_t variableDescriptorCount);
Expand Down
77 changes: 55 additions & 22 deletions MoltenVK/MoltenVK/GPUObjects/MVKDescriptorSet.mm
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,17 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf
return descCnt;
}

MVKDescriptorSetLayoutBinding* MVKDescriptorSetLayout::getBinding(uint32_t binding, uint32_t bindingIndexOffset) {
auto itr = _bindingToIndex.find(binding);
if (itr != _bindingToIndex.end()) {
uint32_t bindIdx = itr->second + bindingIndexOffset;
if (bindIdx < _bindings.size()) {
return &_bindings[bindIdx];
}
}
return nullptr;
}

MVKDescriptorSetLayout::MVKDescriptorSetLayout(MVKDevice* device,
const VkDescriptorSetLayoutCreateInfo* pCreateInfo) : MVKVulkanAPIDeviceObject(device) {

Expand Down Expand Up @@ -430,21 +441,32 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf
size_t srcStride,
const void* pData) {

MVKDescriptorSetLayoutBinding* mvkDSLBind = _layout->getBinding(pDescriptorAction->dstBinding);
VkDescriptorType descType = mvkDSLBind->getDescriptorType();
if (descType == VK_DESCRIPTOR_TYPE_INLINE_UNIFORM_BLOCK_EXT) {
// For inline buffers dstArrayElement is a byte offset
getDescriptor(pDescriptorAction->dstBinding)->write(mvkDSLBind, this, pDescriptorAction->dstArrayElement, 0, srcStride, pData);
auto* mvkDSLBind = _layout->getBinding(pDescriptorAction->dstBinding);
if (mvkDSLBind->getDescriptorType() == VK_DESCRIPTOR_TYPE_INLINE_UNIFORM_BLOCK_EXT) {
// For inline buffers, descriptorCount is a byte count and dstArrayElement is a byte offset.
// If needed, Vulkan allows updates to extend into subsequent bindings that are of the same type,
// so iterate layout bindings and their associated descriptors, until all bytes are updated.
const auto* pInlineUniformBlock = (VkWriteDescriptorSetInlineUniformBlockEXT*)pData;
uint32_t numBytesToCopy = pDescriptorAction->descriptorCount;
uint32_t dstOffset = pDescriptorAction->dstArrayElement;
uint32_t srcOffset = 0;
while (mvkDSLBind && numBytesToCopy > 0 && srcOffset < pInlineUniformBlock->dataSize) {
auto* mvkDesc = (MVKInlineUniformBlockDescriptor*)_descriptors[mvkDSLBind->_descriptorIndex];
auto numBytesMoved = mvkDesc->writeBytes(mvkDSLBind, this, dstOffset, srcOffset, numBytesToCopy, pInlineUniformBlock);
numBytesToCopy -= numBytesMoved;
dstOffset = 0;
srcOffset += numBytesMoved;
mvkDSLBind = _layout->getBinding(mvkDSLBind->getBinding(), 1); // Next binding if needed
}
} else {
uint32_t dslBindDescCnt = mvkDSLBind->getDescriptorCount(_variableDescriptorCount);
// We don't test against the descriptor count of the binding, because Vulkan allows
// updates to extend into subsequent bindings that are of the same type, if needed.
uint32_t srcElemIdx = 0;
uint32_t dstElemIdx = pDescriptorAction->dstArrayElement;
uint32_t descIdx = _layout->getDescriptorIndex(pDescriptorAction->dstBinding, dstElemIdx);
if (dstElemIdx < dslBindDescCnt) {
uint32_t elemCnt = std::min(pDescriptorAction->descriptorCount, dslBindDescCnt - dstElemIdx);
while (srcElemIdx < elemCnt) {
_descriptors[descIdx++]->write(mvkDSLBind, this, dstElemIdx++, srcElemIdx++, srcStride, pData);
}
uint32_t descIdx = _layout->getDescriptorIndex(pDescriptorAction->dstBinding, dstElemIdx);
uint32_t descCnt = pDescriptorAction->descriptorCount;
while (srcElemIdx < descCnt) {
_descriptors[descIdx++]->write(mvkDSLBind, this, dstElemIdx++, srcElemIdx++, srcStride, pData);
}
}
}
Expand All @@ -458,18 +480,29 @@ static void populateAuxBuffer(mvk::SPIRVToMSLConversionConfiguration& shaderConf
MVKDescriptorSetLayoutBinding* mvkDSLBind = _layout->getBinding(pDescriptorCopy->srcBinding);
VkDescriptorType descType = mvkDSLBind->getDescriptorType();
if (descType == VK_DESCRIPTOR_TYPE_INLINE_UNIFORM_BLOCK_EXT) {
// For inline buffers srcArrayElement is a byte offset
getDescriptor(pDescriptorCopy->srcBinding)->read(mvkDSLBind, this, pDescriptorCopy->srcArrayElement, pImageInfo, pBufferInfo, pTexelBufferView, pInlineUniformBlock);
// For inline buffers, descriptorCount is a byte count and dstArrayElement is a byte offset.
// If needed, Vulkan allows updates to extend into subsequent bindings that are of the same type,
// so iterate layout bindings and their associated descriptors, until all bytes are updated.
uint32_t numBytesToCopy = pDescriptorCopy->descriptorCount;
uint32_t dstOffset = 0;
uint32_t srcOffset = pDescriptorCopy->srcArrayElement;
while (mvkDSLBind && numBytesToCopy > 0 && dstOffset < pInlineUniformBlock->dataSize) {
auto* mvkDesc = (MVKInlineUniformBlockDescriptor*)_descriptors[mvkDSLBind->_descriptorIndex];
auto numBytesMoved = mvkDesc->readBytes(mvkDSLBind, this, dstOffset, srcOffset, numBytesToCopy, pInlineUniformBlock);
numBytesToCopy -= numBytesMoved;
dstOffset += numBytesMoved;
srcOffset = 0;
mvkDSLBind = _layout->getBinding(mvkDSLBind->getBinding(), 1); // Next binding if needed
}
} else {
uint32_t dslBindDescCnt = mvkDSLBind->getDescriptorCount(_variableDescriptorCount);
uint32_t dstElemIdx = 0;
// We don't test against the descriptor count of the binding, because Vulkan allows
// updates to extend into subsequent bindings that are of the same type, if needed.
uint32_t srcElemIdx = pDescriptorCopy->srcArrayElement;
uint32_t descIdx = _layout->getDescriptorIndex(pDescriptorCopy->srcBinding, srcElemIdx);
if (srcElemIdx < dslBindDescCnt) {
uint32_t elemCnt = std::min(pDescriptorCopy->descriptorCount, dslBindDescCnt - srcElemIdx);
while (dstElemIdx < elemCnt) {
_descriptors[descIdx++]->read(mvkDSLBind, this, dstElemIdx++, pImageInfo, pBufferInfo, pTexelBufferView, pInlineUniformBlock);
}
uint32_t dstElemIdx = 0;
uint32_t descIdx = _layout->getDescriptorIndex(pDescriptorCopy->srcBinding, srcElemIdx);
uint32_t descCnt = pDescriptorCopy->descriptorCount;
while (dstElemIdx < descCnt) {
_descriptors[descIdx++]->read(mvkDSLBind, this, dstElemIdx++, pImageInfo, pBufferInfo, pTexelBufferView, pInlineUniformBlock);
}
}
}
Expand Down
Loading