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

[Vulkan] updateDescriptorSet is different from the DirectX12 #152

Closed
y18a opened this issue Nov 11, 2019 · 6 comments
Closed

[Vulkan] updateDescriptorSet is different from the DirectX12 #152

y18a opened this issue Nov 11, 2019 · 6 comments

Comments

@y18a
Copy link

y18a commented Nov 11, 2019

The following code works correctly in DirectX12.
However, to malfunction in Vulkan.

DescriptorData params_foo[1] = {};
updateDescriptorSet(pRenderer, 0, pDescriptorSet, 1, params_foo);
DescriptorData params_bar[1] = {};
updateDescriptorSet(pRenderer, 0, pDescriptorSet, 1, params_bar);

I give up to use the vkUpdateDescriptorSetWithTemplateKHR has changed in the code that uses the vkUpdateDescriptorSets, such as in the following code.

VkWriteDescriptorSet* descSetWrites = (VkWriteDescriptorSet*)alloca(pRootSignature->mVkCumulativeDescriptorCounts[updateFreq] * sizeof(VkWriteDescriptorSet));
uint32_t descSetWriteCount = 0;

~~~snip~~~

                VkWriteDescriptorSet* pWrite = descSetWrites + descSetWriteCount;
                pWrite->sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET;
                pWrite->pNext = nullptr;
                pWrite->dstSet = pDescriptorSet->pHandles[index];
                pWrite->dstBinding = pDesc->mDesc.reg;
                pWrite->dstArrayElement = arr;
                pWrite->descriptorCount = 1;
                pWrite->descriptorType = pDesc->mVkType;
                pWrite->pImageInfo = &pUpdateData[pDesc->mHandleIndex + arr].mImageInfo;
                pWrite->pBufferInfo = nullptr;
                pWrite->pTexelBufferView = nullptr;
                ++descSetWriteCount;

~~~snip~~~

if (descSetWriteCount)
  vkUpdateDescriptorSets(pRenderer->pVkDevice, descSetWriteCount, descSetWrites, 0, nullptr);
@manas-kulkarni
Copy link

Could you post the code where you fill params_foo[0], params_bar[0]

@wolfgangfengel
Copy link
Contributor

Hey y18a, do you want to contribute to our GitLab repository? We can add you. In case you want to, please send me your e-mail and skype id.

@y18a
Copy link
Author

y18a commented Nov 13, 2019

Anyway, it is a patch. It is ad hoc fixes.

diff --git a/Common_3/Renderer/Vulkan/Vulkan.cpp b/Common_3/Renderer/Vulkan/Vulkan.cpp
index 7977cc66..b32331a3 100644
--- a/Common_3/Renderer/Vulkan/Vulkan.cpp
+++ b/Common_3/Renderer/Vulkan/Vulkan.cpp
@@ -3938,8 +3938,9 @@ void updateDescriptorSet(Renderer* pRenderer, uint32_t index, DescriptorSet* pDe
        const RootSignature* pRootSignature = pDescriptorSet->pRootSignature;
        DescriptorUpdateFrequency updateFreq = (DescriptorUpdateFrequency)pDescriptorSet->mUpdateFrequency;
        DescriptorUpdateData* pUpdateData = (DescriptorUpdateData*)alloca(pRootSignature->mVkCumulativeDescriptorCounts[updateFreq] * sizeof(DescriptorUpdateData));
-       memcpy(pUpdateData, pRootSignature->pUpdateTemplateData[updateFreq][pDescriptorSet->mNodeIndex], pRootSignature->mVkCumulativeDescriptorCounts[updateFreq] * sizeof(DescriptorUpdateData));
-       bool update = false;
+
+       VkWriteDescriptorSet* descSetWrites = (VkWriteDescriptorSet*)alloca(pRootSignature->mVkCumulativeDescriptorCounts[updateFreq] * sizeof(VkWriteDescriptorSet));
+       uint32_t descSetWriteCount = 0;
 
 #ifdef ENABLE_RAYTRACING
        VkWriteDescriptorSet* raytracingWrites = NULL;
@@ -3992,7 +3993,19 @@ void updateDescriptorSet(Renderer* pRenderer, uint32_t index, DescriptorSet* pDe
                                VALIDATE_DESCRIPTOR(pParam->ppSamplers[arr], "NULL Sampler (%s [%u] )", pDesc->mDesc.name, arr);
 
                                pUpdateData[pDesc->mHandleIndex + arr].mImageInfo = { pParam->ppSamplers[arr]->pVkSampler, VK_NULL_HANDLE };
-                               update = true;
+
+                               VkWriteDescriptorSet* pWrite = descSetWrites + descSetWriteCount;
+                               pWrite->sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET;
+                               pWrite->pNext = nullptr;
+                               pWrite->dstSet = pDescriptorSet->pHandles[index];
+                               pWrite->dstBinding = pDesc->mDesc.reg;
+                               pWrite->dstArrayElement = arr;
+                               pWrite->descriptorCount = 1;
+                               pWrite->descriptorType = pDesc->mVkType;
+                               pWrite->pImageInfo = &pUpdateData[pDesc->mHandleIndex + arr].mImageInfo;
+                               pWrite->pBufferInfo = nullptr;
+                               pWrite->pTexelBufferView = nullptr;
+                               ++descSetWriteCount;
                        }
                        break;
                }
@@ -4013,7 +4026,18 @@ void updateDescriptorSet(Renderer* pRenderer, uint32_t index, DescriptorSet* pDe
                                                VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL           // Image Layout
                                        };
                     
-                                       update = true;
+                                       VkWriteDescriptorSet* pWrite = descSetWrites + descSetWriteCount;
+                                       pWrite->sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET;
+                                       pWrite->pNext = nullptr;
+                                       pWrite->dstSet = pDescriptorSet->pHandles[index];
+                                       pWrite->dstBinding = pDesc->mDesc.reg;
+                                       pWrite->dstArrayElement = arr;
+                                       pWrite->descriptorCount = 1;
+                                       pWrite->descriptorType = pDesc->mVkType;
+                                       pWrite->pImageInfo = &pUpdateData[pDesc->mHandleIndex + arr].mImageInfo;
+                                       pWrite->pBufferInfo = nullptr;
+                                       pWrite->pTexelBufferView = nullptr;
+                                       ++descSetWriteCount;
                                }
                        }
                        else
@@ -4029,7 +4053,18 @@ void updateDescriptorSet(Renderer* pRenderer, uint32_t index, DescriptorSet* pDe
                                                VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL           // Image Layout
                                        };
 
-                                       update = true;
+                                       VkWriteDescriptorSet* pWrite = descSetWrites + descSetWriteCount;
+                                       pWrite->sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET;
+                                       pWrite->pNext = nullptr;
+                                       pWrite->dstSet = pDescriptorSet->pHandles[index];
+                                       pWrite->dstBinding = pDesc->mDesc.reg;
+                                       pWrite->dstArrayElement = arr;
+                                       pWrite->descriptorCount = 1;
+                                       pWrite->descriptorType = pDesc->mVkType;
+                                       pWrite->pImageInfo = &pUpdateData[pDesc->mHandleIndex + arr].mImageInfo;
+                                       pWrite->pBufferInfo = nullptr;
+                                       pWrite->pTexelBufferView = nullptr;
+                                       ++descSetWriteCount;
                                }
                        }
                        break;
@@ -4052,7 +4087,18 @@ void updateDescriptorSet(Renderer* pRenderer, uint32_t index, DescriptorSet* pDe
                                        VK_IMAGE_LAYOUT_GENERAL                                // Image Layout
                                };
 
-                               update = true;
+                               VkWriteDescriptorSet* pWrite = descSetWrites + descSetWriteCount;
+                               pWrite->sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET;
+                               pWrite->pNext = nullptr;
+                               pWrite->dstSet = pDescriptorSet->pHandles[index];
+                               pWrite->dstBinding = pDesc->mDesc.reg;
+                               pWrite->dstArrayElement = arr;
+                               pWrite->descriptorCount = 1;
+                               pWrite->descriptorType = pDesc->mVkType;
+                               pWrite->pImageInfo = &pUpdateData[pDesc->mHandleIndex + arr].mImageInfo;
+                               pWrite->pBufferInfo = nullptr;
+                               pWrite->pTexelBufferView = nullptr;
+                               ++descSetWriteCount;
                        }
                        break;
                }
@@ -4071,18 +4117,31 @@ void updateDescriptorSet(Renderer* pRenderer, uint32_t index, DescriptorSet* pDe
                                        pRenderer->pVkActiveGPUProperties->properties.limits.maxUniformBufferRange);
 
                                pDescriptorSet->pDynamicOffsets[index][pDesc->mDynamicUniformIndex] = pParam->pOffsets ? (uint32_t)pParam->pOffsets[0] : 0;
-                               pUpdateData[pDesc->mHandleIndex + 0].mBufferInfo = pParam->ppBuffers[0]->mVkBufferInfo;
-                               pUpdateData[pDesc->mHandleIndex + 0].mBufferInfo.range = pParam->pSizes[0];
-
                                // If this is a different size we have to update the VkDescriptorBufferInfo::range so a call to vkUpdateDescriptorSet is necessary
                                if (pParam->pSizes[0] != (uint32_t)pDescriptorSet->pDynamicSizes[index][pDesc->mDynamicUniformIndex])
                                {
                                        pDescriptorSet->pDynamicSizes[index][pDesc->mDynamicUniformIndex] = (uint32_t)pParam->pSizes[0];
-                                       update = true;
+
+                                       pUpdateData[pDesc->mHandleIndex + 0].mBufferInfo = pParam->ppBuffers[0]->mVkBufferInfo;
+                                       pUpdateData[pDesc->mHandleIndex + 0].mBufferInfo.range = pParam->pSizes[0];
+
+                                       VkWriteDescriptorSet* pWrite = descSetWrites + descSetWriteCount;
+                                       pWrite->sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET;
+                                       pWrite->pNext = nullptr;
+                                       pWrite->dstSet = pDescriptorSet->pHandles[index];
+                                       pWrite->dstBinding = pDesc->mDesc.reg;
+                                       pWrite->dstArrayElement = 0;
+                                       pWrite->descriptorCount = 1;
+                                       pWrite->descriptorType = pDesc->mVkType;
+                                       pWrite->pImageInfo = nullptr;
+                                       pWrite->pBufferInfo = &pUpdateData[pDesc->mHandleIndex + 0].mBufferInfo;
+                                       pWrite->pTexelBufferView = nullptr;
+                                       ++descSetWriteCount;
                                }
 
                                break;
                        }
+        }
                case DESCRIPTOR_TYPE_BUFFER:
                case DESCRIPTOR_TYPE_BUFFER_RAW:
                case DESCRIPTOR_TYPE_RW_BUFFER:
@@ -4095,6 +4154,7 @@ void updateDescriptorSet(Renderer* pRenderer, uint32_t index, DescriptorSet* pDe
                                VALIDATE_DESCRIPTOR(pParam->ppBuffers[arr], "NULL Buffer (%s [%u] )", pDesc->mDesc.name, arr);
 
                                pUpdateData[pDesc->mHandleIndex + arr].mBufferInfo = pParam->ppBuffers[arr]->mVkBufferInfo;
+
                                if (pParam->pOffsets)
                                {
                                        VALIDATE_DESCRIPTOR(pParam->pSizes, "Descriptor (%s) - pSizes must be provided with pOffsets", pDesc->mDesc.name);
@@ -4108,10 +4168,20 @@ void updateDescriptorSet(Renderer* pRenderer, uint32_t index, DescriptorSet* pDe
                                        pUpdateData[pDesc->mHandleIndex + arr].mBufferInfo.range = pParam->pSizes[arr];
                                }
 
-                               update = true;
+                               VkWriteDescriptorSet* pWrite = descSetWrites + descSetWriteCount;
+                               pWrite->sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET;
+                               pWrite->pNext = nullptr;
+                               pWrite->dstSet = pDescriptorSet->pHandles[index];
+                               pWrite->dstBinding = pDesc->mDesc.reg;
+                               pWrite->dstArrayElement = arr;
+                               pWrite->descriptorCount = 1;
+                               pWrite->descriptorType = pDesc->mVkType;
+                               pWrite->pImageInfo = nullptr;
+                               pWrite->pBufferInfo = &pUpdateData[pDesc->mHandleIndex + arr].mBufferInfo;
+                               pWrite->pTexelBufferView = nullptr;
+                               ++descSetWriteCount;
                        }
 
-               }
                        break;
                }
                case DESCRIPTOR_TYPE_TEXEL_BUFFER:
@@ -4122,7 +4192,19 @@ void updateDescriptorSet(Renderer* pRenderer, uint32_t index, DescriptorSet* pDe
                        {
                                VALIDATE_DESCRIPTOR(pParam->ppBuffers[arr], "NULL Texel Buffer (%s [%u] )", pDesc->mDesc.name, arr);
                                pUpdateData[pDesc->mHandleIndex + arr].mBuferView = pParam->ppBuffers[arr]->pVkUniformTexelView;
-                               update = true;
+
+                               VkWriteDescriptorSet* pWrite = descSetWrites + descSetWriteCount;
+                               pWrite->sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET;
+                               pWrite->pNext = nullptr;
+                               pWrite->dstSet = pDescriptorSet->pHandles[index];
+                               pWrite->dstBinding = pDesc->mDesc.reg;
+                               pWrite->dstArrayElement = arr;
+                               pWrite->descriptorCount = 1;
+                               pWrite->descriptorType = pDesc->mVkType;
+                               pWrite->pImageInfo = nullptr;
+                               pWrite->pBufferInfo = nullptr;
+                               pWrite->pTexelBufferView = &pUpdateData[pDesc->mHandleIndex + arr].mBuferView;
+                               ++descSetWriteCount;
                        }
 
                        break;
@@ -4135,7 +4217,19 @@ void updateDescriptorSet(Renderer* pRenderer, uint32_t index, DescriptorSet* pDe
                        {
                                VALIDATE_DESCRIPTOR(pParam->ppBuffers[arr], "NULL RW Texel Buffer (%s [%u] )", pDesc->mDesc.name, arr);
                                pUpdateData[pDesc->mHandleIndex + arr].mBuferView = pParam->ppBuffers[arr]->pVkStorageTexelView;
-                               update = true;
+
+                               VkWriteDescriptorSet* pWrite = descSetWrites + descSetWriteCount;
+                               pWrite->sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET;
+                               pWrite->pNext = nullptr;
+                               pWrite->dstSet = pDescriptorSet->pHandles[index];
+                               pWrite->dstBinding = pDesc->mDesc.reg;
+                               pWrite->dstArrayElement = arr;
+                               pWrite->descriptorCount = 1;
+                               pWrite->descriptorType = pDesc->mVkType;
+                               pWrite->pImageInfo = nullptr;
+                               pWrite->pBufferInfo = nullptr;
+                               pWrite->pTexelBufferView = &pUpdateData[pDesc->mHandleIndex + arr].mBuferView;
+                               ++descSetWriteCount;
                        }
 
                        break;
@@ -4172,10 +4266,8 @@ void updateDescriptorSet(Renderer* pRenderer, uint32_t index, DescriptorSet* pDe
                }
        }
 
-       // If this was called to just update a dynamic offset skip the update
-       if (update)
-               vkUpdateDescriptorSetWithTemplateKHR(pRenderer->pVkDevice, pDescriptorSet->pHandles[index],
-                       pRootSignature->mUpdateTemplates[updateFreq], pUpdateData);
+       if (descSetWriteCount)
+               vkUpdateDescriptorSets(pRenderer->pVkDevice, descSetWriteCount, descSetWrites, 0, nullptr);
 
 #ifdef ENABLE_RAYTRACING
        // Raytracing Update Descriptor Set since it does not support update template
@@ -4727,6 +4819,7 @@ void addRootSignature(Renderer* pRenderer, const RootSignatureDesc* pRootSignatu
        {
                if (pRootSignature->mVkDescriptorCounts[setIndex])
                {
+#if 0
                        const UpdateFrequencyLayoutInfo& layout = layouts[setIndex];
                        VkDescriptorUpdateTemplateEntry* pEntries = (VkDescriptorUpdateTemplateEntry*)alloca(pRootSignature->mVkDescriptorCounts[setIndex] * sizeof(VkDescriptorUpdateTemplateEntry));
                        uint32_t entryCount = 0;
@@ -4832,6 +4925,7 @@ void addRootSignature(Renderer* pRenderer, const RootSignatureDesc* pRootSignatu
                        createInfo.templateType = VK_DESCRIPTOR_UPDATE_TEMPLATE_TYPE_DESCRIPTOR_SET_KHR;
                        VkResult vkRes = vkCreateDescriptorUpdateTemplateKHR(pRenderer->pVkDevice, &createInfo, NULL, &pRootSignature->mUpdateTemplates[setIndex]);
                        ASSERT(VK_SUCCESS == vkRes);
+#endif
                }
                else if (VK_NULL_HANDLE != pRootSignature->mVkDescriptorSetLayouts[setIndex])
                {

@manas-kulkarni
Copy link

It should be fixed in the next release. But I would recommend using updateDescriptorSet in a more Vulkan / D3D12 like style (send all DescriptorData params together in one updateDescriptorSet call) instead of calling updateDescriptorSet multiple times for the same set which is what it looks like from the code you posted.

@y18a
Copy link
Author

y18a commented Nov 13, 2019

That's right. We would like to do so.

In particular,
At the time of the code such as the following,
For lightData update is not required of in OnRebuidShadowTex,
I was wondering what to do.

void xxxx::setupDescriptorSet(...)
{
  DescriptorData params[4] = {};
  params[0].pName = "shadowTex";
  params[0].ppTextures = ;
  params[1].pName = "clustersTex";
  params[1].ppTextures = ;
  params[2].pName = "lightsData";
  params[2].ppBuffers = ;
  params[3].pName = "lightsHeader";
  params[3].ppBuffers = ;
  
  for(unsigned i = 0; i < forge::GetImageCount(); ++i)
      updateDescriptorSet( renderer , i , s_descriptorSet, 4, params );
}

// Called by switching from the GUI. 
void xxxx::onRebuildShadowTex(...)
{
  DescriptorData params[1] = {};
  params[0].pName = "shadowTex";
  params[0].ppTextures = ;

  for(unsigned i = 0; i < forge::GetImageCount(); ++i)
      updateDescriptorSet( renderer , i , s_descriptorSet, 1, params );
}

@wolfgangfengel
Copy link
Contributor

Should be fixed in the release today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants