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

2018-09-18 Thread Samuel Pitoiset 



On 9/18/18 5:20 PM, Gustaw Smolarczyk wrote:

pt., 14 wrz 2018 o 15:00 Bas Nieuwenhuizen  
napisał(a):


Reviewed-by: Bas Nieuwenhuizen 
On Fri, Sep 14, 2018 at 2:55 PM Samuel Pitoiset
 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 
Signed-off-by: Samuel Pitoiset 
---
  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@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


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


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-09-18 Thread Bas Nieuwenhuizen 
On Tue, 18 Sep 2018, 17:21 Gustaw Smolarczyk,  wrote:

> pt., 14 wrz 2018 o 15:00 Bas Nieuwenhuizen 
> napisał(a):
> >
> > Reviewed-by: Bas Nieuwenhuizen 
> > On Fri, Sep 14, 2018 at 2:55 PM Samuel Pitoiset
> >  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 
> > > Signed-off-by: Samuel Pitoiset 
> > > ---
> > >  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@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
>
> 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.
>
>
> 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.
>

Totally agree. Was thinking about the descriptors set allocation logic
instead of the pool logic when reviewing this ... Apologies.

>
> 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
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-09-18 Thread Gustaw Smolarczyk 
pt., 14 wrz 2018 o 15:00 Bas Nieuwenhuizen  
napisał(a):
>
> Reviewed-by: Bas Nieuwenhuizen 
> On Fri, Sep 14, 2018 at 2:55 PM Samuel Pitoiset
>  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 
> > Signed-off-by: Samuel Pitoiset 
> > ---
> >  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@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

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.


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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-09-14 Thread Bas Nieuwenhuizen 
Reviewed-by: Bas Nieuwenhuizen 
On Fri, Sep 14, 2018 at 2:55 PM Samuel Pitoiset
 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 
> Signed-off-by: Samuel Pitoiset 
> ---
>  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@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

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

2018-09-14 Thread Samuel Pitoiset 
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 
Signed-off-by: Samuel Pitoiset 
---
 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev