Re: [Mesa-dev] [PATCH 2/2] radv: ignore inline uniform blocks in radv_CmdPushDescriptorSetKHR()
On 6/11/19 12:19 PM, Samuel Iglesias Gonsálvez wrote: On 6/11/19 12:05 PM, Józef Kucia wrote: On Tue, Jun 11, 2019 at 11:57 AM Samuel Iglesias Gonsálvez wrote: According to the Vulkan spec, uniform blocks are not allowed to be updated through vkCmdPushDescriptorSetKHR(). There are these spec quotes from "13.2.1. Descriptor Set Layout" that are relevant for this case: "VK_DESCRIPTOR_SET_LAYOUT_CREATE_PUSH_DESCRIPTOR_BIT_KHR specifies that descriptor sets must not be allocated using this layout, and descriptors are instead pushed by vkCmdPushDescriptorSetKHR." "If flags contains VK_DESCRIPTOR_SET_LAYOUT_CREATE_PUSH_DESCRIPTOR_BIT_KHR, then all elements of pBindings must not have a descriptorType of VK_DESCRIPTOR_TYPE_INLINE_UNIFORM_BLOCK_EXT". There is no explicit mention in vkCmdPushDescriptorSetKHR() to forbid this case but it is implied in the creation of the descriptor set layout as aforementioned. Signed-off-by: Samuel Iglesias Gonsálvez --- My only doubt is I did well in the case of radv_meta_push_descriptor_set(), let me know if you prefer to change it to false. Perhaps I'm missing something, but what is the point to add the additional checks for invalid usage? A correct program must not use inline uniform blocks with push descriptors. Right, it should be detected by the Validation Layers. However it is arguable what to do in the driver's side. We can just keep it as it is now, ignore inline uniform block updates (this patch) or even add an assert if it affects stability of the HW (it is not the case here, we tested it). I think ignoring the updates it's the best option, but I am OK with what RADV developers prefer to do. Adding an assertion looks better to me as well. Sam ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] radv: ignore inline uniform blocks in radv_CmdPushDescriptorSetKHR()
On Tue, Jun 11, 2019 at 12:19 PM Samuel Iglesias Gonsálvez wrote: > Right, it should be detected by the Validation Layers. However it is > arguable what to do in the driver's side. We can just keep it as it is > now, ignore inline uniform block updates (this patch) or even add an > assert if it affects stability of the HW (it is not the case here, we > tested it). I think ignoring the updates it's the best option, but I am > OK with what RADV developers prefer to do. IMHO the code should be left as it is. An assert could be a good addition, but an additional bool parameter only for handling invalid usage doesn't seem right. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] radv: ignore inline uniform blocks in radv_CmdPushDescriptorSetKHR()
On 6/11/19 12:05 PM, Józef Kucia wrote: > On Tue, Jun 11, 2019 at 11:57 AM Samuel Iglesias Gonsálvez > wrote: >> According to the Vulkan spec, uniform blocks are not allowed to be >> updated through vkCmdPushDescriptorSetKHR(). >> >> There are these spec quotes from "13.2.1. Descriptor Set Layout" >> that are relevant for this case: >> >> "VK_DESCRIPTOR_SET_LAYOUT_CREATE_PUSH_DESCRIPTOR_BIT_KHR specifies >> that descriptor sets must not be allocated using this layout, and >> descriptors are instead pushed by vkCmdPushDescriptorSetKHR." >> >> "If flags contains >> VK_DESCRIPTOR_SET_LAYOUT_CREATE_PUSH_DESCRIPTOR_BIT_KHR, then all >> elements of pBindings must not have a descriptorType of >> VK_DESCRIPTOR_TYPE_INLINE_UNIFORM_BLOCK_EXT". >> >> There is no explicit mention in vkCmdPushDescriptorSetKHR() to forbid >> this case but it is implied in the creation of the descriptor set >> layout as aforementioned. >> >> Signed-off-by: Samuel Iglesias Gonsálvez >> --- >> >> My only doubt is I did well in the case of >> radv_meta_push_descriptor_set(), let me know if you prefer to change >> it to false. >> > Perhaps I'm missing something, but what is the point to add the > additional checks for invalid usage? A correct program must not use > inline uniform blocks with push descriptors. > Right, it should be detected by the Validation Layers. However it is arguable what to do in the driver's side. We can just keep it as it is now, ignore inline uniform block updates (this patch) or even add an assert if it affects stability of the HW (it is not the case here, we tested it). I think ignoring the updates it's the best option, but I am OK with what RADV developers prefer to do. Sam signature.asc Description: OpenPGP digital signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] radv: ignore inline uniform blocks in radv_CmdPushDescriptorSetKHR()
On Tue, Jun 11, 2019 at 11:57 AM Samuel Iglesias Gonsálvez wrote: > > According to the Vulkan spec, uniform blocks are not allowed to be > updated through vkCmdPushDescriptorSetKHR(). > > There are these spec quotes from "13.2.1. Descriptor Set Layout" > that are relevant for this case: > > "VK_DESCRIPTOR_SET_LAYOUT_CREATE_PUSH_DESCRIPTOR_BIT_KHR specifies > that descriptor sets must not be allocated using this layout, and > descriptors are instead pushed by vkCmdPushDescriptorSetKHR." > > "If flags contains > VK_DESCRIPTOR_SET_LAYOUT_CREATE_PUSH_DESCRIPTOR_BIT_KHR, then all > elements of pBindings must not have a descriptorType of > VK_DESCRIPTOR_TYPE_INLINE_UNIFORM_BLOCK_EXT". > > There is no explicit mention in vkCmdPushDescriptorSetKHR() to forbid > this case but it is implied in the creation of the descriptor set > layout as aforementioned. > > Signed-off-by: Samuel Iglesias Gonsálvez > --- > > My only doubt is I did well in the case of > radv_meta_push_descriptor_set(), let me know if you prefer to change > it to false. > Perhaps I'm missing something, but what is the point to add the additional checks for invalid usage? A correct program must not use inline uniform blocks with push descriptors. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/2] radv: ignore inline uniform blocks in radv_CmdPushDescriptorSetKHR()
According to the Vulkan spec, uniform blocks are not allowed to be
updated through vkCmdPushDescriptorSetKHR().
There are these spec quotes from "13.2.1. Descriptor Set Layout"
that are relevant for this case:
"VK_DESCRIPTOR_SET_LAYOUT_CREATE_PUSH_DESCRIPTOR_BIT_KHR specifies
that descriptor sets must not be allocated using this layout, and
descriptors are instead pushed by vkCmdPushDescriptorSetKHR."
"If flags contains
VK_DESCRIPTOR_SET_LAYOUT_CREATE_PUSH_DESCRIPTOR_BIT_KHR, then all
elements of pBindings must not have a descriptorType of
VK_DESCRIPTOR_TYPE_INLINE_UNIFORM_BLOCK_EXT".
There is no explicit mention in vkCmdPushDescriptorSetKHR() to forbid
this case but it is implied in the creation of the descriptor set
layout as aforementioned.
Signed-off-by: Samuel Iglesias Gonsálvez
---
My only doubt is I did well in the case of
radv_meta_push_descriptor_set(), let me know if you prefer to change
it to false.
src/amd/vulkan/radv_cmd_buffer.c | 4 ++--
src/amd/vulkan/radv_descriptor_set.c | 11 ---
src/amd/vulkan/radv_private.h| 3 ++-
3 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/amd/vulkan/radv_cmd_buffer.c b/src/amd/vulkan/radv_cmd_buffer.c
index 3faaf94eb99..38a2d0d0efe 100644
--- a/src/amd/vulkan/radv_cmd_buffer.c
+++ b/src/amd/vulkan/radv_cmd_buffer.c
@@ -3219,7 +3219,7 @@ void radv_meta_push_descriptor_set(
radv_update_descriptor_sets(cmd_buffer->device, cmd_buffer,
radv_descriptor_set_to_handle(push_set),
- descriptorWriteCount, pDescriptorWrites, 0,
NULL);
+ descriptorWriteCount, pDescriptorWrites, 0,
NULL, true);
radv_set_descriptor_set(cmd_buffer, pipelineBindPoint, push_set, set);
}
@@ -3247,7 +3247,7 @@ void radv_CmdPushDescriptorSetKHR(
radv_update_descriptor_sets(cmd_buffer->device, cmd_buffer,
radv_descriptor_set_to_handle(push_set),
- descriptorWriteCount, pDescriptorWrites, 0,
NULL);
+ descriptorWriteCount, pDescriptorWrites, 0,
NULL, true);
radv_set_descriptor_set(cmd_buffer, pipelineBindPoint, push_set, set);
descriptors_state->push_dirty = true;
diff --git a/src/amd/vulkan/radv_descriptor_set.c
b/src/amd/vulkan/radv_descriptor_set.c
index bd00f68a3cb..04bbafb1ed5 100644
--- a/src/amd/vulkan/radv_descriptor_set.c
+++ b/src/amd/vulkan/radv_descriptor_set.c
@@ -939,7 +939,8 @@ void radv_update_descriptor_sets(
uint32_tdescriptorWriteCount,
const VkWriteDescriptorSet* pDescriptorWrites,
uint32_tdescriptorCopyCount,
- const VkCopyDescriptorSet* pDescriptorCopies)
+ const VkCopyDescriptorSet* pDescriptorCopies,
+ const bool isPushDescriptorSet)
{
uint32_t i, j;
for (i = 0; i < descriptorWriteCount; i++) {
@@ -961,7 +962,11 @@ void radv_update_descriptor_sets(
ptr += binding_layout->offset / 4;
if (writeset->descriptorType ==
VK_DESCRIPTOR_TYPE_INLINE_UNIFORM_BLOCK_EXT) {
- write_block_descriptor(device, cmd_buffer,
(uint8_t*)ptr + writeset->dstArrayElement, writeset);
+ /* Ignore inline uniform block updates when called from
vkCmdPushDescriptorSetKHR()
+* because it is invalid, according to Vulkan spec.
+*/
+ if (!isPushDescriptorSet)
+ write_block_descriptor(device, cmd_buffer,
(uint8_t*)ptr + writeset->dstArrayElement, writeset);
continue;
}
@@ -1094,7 +1099,7 @@ void radv_UpdateDescriptorSets(
RADV_FROM_HANDLE(radv_device, device, _device);
radv_update_descriptor_sets(device, NULL, VK_NULL_HANDLE,
descriptorWriteCount, pDescriptorWrites,
- descriptorCopyCount, pDescriptorCopies);
+ descriptorCopyCount, pDescriptorCopies,
false);
}
VkResult radv_CreateDescriptorUpdateTemplate(VkDevice _device,
diff --git a/src/amd/vulkan/radv_private.h b/src/amd/vulkan/radv_private.h
index 8f2e80b3017..bead0867119 100644
--- a/src/amd/vulkan/radv_private.h
+++ b/src/amd/vulkan/radv_private.h
@@ -1976,7 +1976,8 @@ radv_update_descriptor_sets(struct radv_device *device,
uint32_t descriptorWriteCount,
const VkWriteDescriptorSet *pDescriptorWrites,
uint32_t descriptorCopyCount,
-const VkCopyDescriptorSet *pDescriptorCopies);
+const VkCopyDescriptorSet *pDescriptorCopies,
+const bool isPushDescriptorSet);
