[Mesa-stable] [Mesa-dev] [PATCH] radv: fix descriptor pool allocation size

[Samuel Pitoiset]

On 9/18/18 5:20 PM, Gustaw Smolarczyk wrote:
> pt., 14 wrz 2018 o 15:00 Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl> napisał(a):
>>
>> Reviewed-by: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
>> On Fri, Sep 14, 2018 at 2:55 PM Samuel Pitoiset
>> <samuel.pitoiset at gmail.com> wrote:
>>>
>>> The size has to be multiplied by the number of sets.
>>>
>>> This gets rid of the OUT_OF_POOL_KHR error and fixes
>>> the Tangrams demo.
>>>
>>> CC: 18.1 18.2 <mesa-stable at lists.freedesktop.org>
>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>>> ---
>>>   src/amd/vulkan/radv_descriptor_set.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/amd/vulkan/radv_descriptor_set.c b/src/amd/vulkan/radv_descriptor_set.c
>>> index c4341f6ac5..49d0811bb0 100644
>>> --- a/src/amd/vulkan/radv_descriptor_set.c
>>> +++ b/src/amd/vulkan/radv_descriptor_set.c
>>> @@ -569,9 +569,10 @@ VkResult radv_CreateDescriptorPool(
>>>          }
>>>
>>>          if (!(pCreateInfo->flags & VK_DESCRIPTOR_POOL_CREATE_FREE_DESCRIPTOR_SET_BIT)) {
>>> -               uint64_t host_size = pCreateInfo->maxSets * sizeof(struct radv_descriptor_set);
>>> +               uint64_t host_size = sizeof(struct radv_descriptor_set);
>>>                  host_size += sizeof(struct radeon_winsys_bo*) * bo_count;
>>>                  host_size += sizeof(struct radv_descriptor_range) * range_count;
>>> +               host_size *= pCreateInfo->maxSets;
>>>                  size += host_size;
>>>          } else {
>>>                  size += sizeof(struct radv_descriptor_pool_entry) * pCreateInfo->maxSets;
>>> --
>>> 2.19.0
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> I don't think this change is correct.
> 
>  From the vkAllocateDescriptorSets documentation [1]:
> 
> 
> If a call to vkAllocateDescriptorSets would cause the total number of
> descriptor sets allocated from the pool to exceed the value of
> VkDescriptorPoolCreateInfo::maxSets used to create
> pAllocateInfo→descriptorPool, then the allocation may fail due to lack
> of space in the descriptor pool. Similarly, the allocation may fail
> due to lack of space if the call to vkAllocateDescriptorSets would
> cause the number of any given descriptor type to exceed the sum of all
> the descriptorCount members of each element of
> VkDescriptorPoolCreateInfo::pPoolSizes with a member equal to that
> type.

Yes. As I said to Bas over IRC that app is actually buggy and I have 
been confused. This patch should be reverted and the bug reported.

FWIW, it works with AMDVLK because they seem to always allocate more 
space than needed.

> 
> 
> What it implies (I think), is that VkDescriptorPoolCreateInfo::maxSets
> and descriptorCount of each VkDescriptorPoolCreateInfo::pPoolSizes are
> treated separately. I don't think you should multiply one by another.
> Each pool should be able to allocate at least maxSets sets and
> descriptorCount descriptors.
> 
> Please, correct me if I'm wrong.
> 
> Regards,
> 
> Gustaw Smolarczyk
> 
> [1] https://www.khronos.org/registry/vulkan/specs/1.1-extensions/man/html/vkAllocateDescriptorSets.html
>