Skip to content

Commit

Permalink
Merge pull request #2303 from billhollings/multi-desc-write-copy
Browse files Browse the repository at this point in the history 
Support Vulkan requirement to allow write or copy beyond descriptor binding size.
  • Loading branch information
@billhollings
billhollings authored Aug 6, 2024
2 parents 5b8bb33 + 4a8ed61 commit 3b9e335
Show file tree
Hide file tree
Showing 6 changed files with 116 additions and 54 deletions.
2 changes: 2 additions & 0 deletions Docs/Whats_New.md
View file
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
View file
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
View file
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
View file
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
View file
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
View file
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

0 comments on commit 3b9e335

Please sign in to comment.