Re: [Mesa-dev] [PATCH v4] anv/device: fix maximum number of images supported

2019-01-18 Thread Iago Toral 
Argh, I am really sorry about that :-(

It seems I didn't push the right version patch (the one I sent for
review) but a previous version of that. The patch that Lionel sent to
fix this is exactly what I had changed in the version I sent for
review.

I am dorry for the mess, I'll be more careful next time.

Iago

On Thu, 2019-01-17 at 10:13 -0800, Mark Janes wrote:
> Hi Iago,
> 
> It looks like you tested this patch in CI and got the same failures
> that
> we are seeing on master:
> 
> 
http://mesa-ci-results.jf.intel.com/itoral/builds/263/group/63a9f0ea7bb98050796b649e85481845
> 
> Why was this patch pushed?
> 
> -Mark
> 
> Mark Janes  writes:
> 
> > This patch regresses thousands of tests on BDW and HSW:
> > 
> > 
http://mesa-ci-results.jf.intel.com/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails
> > 
> > I'll revert it as soon as my testing completes.
> > 
> > Iago Toral Quiroga  writes:
> > 
> > > We had defined MAX_IMAGES as 8, which we used to size the array
> > > for
> > > image push constant data. The comment there stated that this was
> > > for
> > > gen8, but anv_nir_apply_pipeline_layout runs for all gens and
> > > writes
> > > that array, asserting that we don't exceed that number of images,
> > > which imposes a limit of MAX_IMAGES on all gens.
> > > 
> > > Furthermore, despite this, we are exposing up to 64 images per
> > > shader
> > > stage on all gens, gen8 included.
> > > 
> > > This patch lowers the number of images we expose in gen8 to 8 and
> > > keeps 64 images for gen9+ while making sure that only pre-SKL
> > > gens
> > > use push constant space to handle images.
> > > 
> > > v2:
> > >  - <= instead of < in the assert (Eric, Lionel)
> > >  - Change the way the assertion is written (Eric)
> > > 
> > > v3:
> > >  - Revert the way the assertion is written to the form it had in
> > > v1,
> > >the version in v2 was not equivalent and was incorrect.
> > > (Lionel)
> > > 
> > > v4:
> > >  - gen9+ doesn't need push constants for images at all (Jason)
> > > ---
> > >  src/intel/vulkan/anv_device.c |  7 --
> > >  .../vulkan/anv_nir_apply_pipeline_layout.c|  4 +--
> > >  src/intel/vulkan/anv_private.h|  5 ++--
> > >  src/intel/vulkan/genX_cmd_buffer.c| 25
> > > +--
> > >  4 files changed, 28 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/src/intel/vulkan/anv_device.c
> > > b/src/intel/vulkan/anv_device.c
> > > index 523f1483e29..f85458b672e 100644
> > > --- a/src/intel/vulkan/anv_device.c
> > > +++ b/src/intel/vulkan/anv_device.c
> > > @@ -987,9 +987,12 @@ void anv_GetPhysicalDeviceProperties(
> > > const uint32_t max_samplers = (devinfo->gen >= 8 || devinfo-
> > > >is_haswell) ?
> > >   128 : 16;
> > >  
> > > +   const uint32_t max_images = devinfo->gen < 9 ?
> > > MAX_GEN8_IMAGES : MAX_IMAGES;
> > > +
> > > VkSampleCountFlags sample_counts =
> > >isl_device_get_sample_counts(&pdevice->isl_dev);
> > >  
> > > +
> > > VkPhysicalDeviceLimits limits = {
> > >.maxImageDimension1D  = (1 << 14),
> > >.maxImageDimension2D  = (1 << 14),
> > > @@ -1009,7 +1012,7 @@ void anv_GetPhysicalDeviceProperties(
> > >.maxPerStageDescriptorUniformBuffers  = 64,
> > >.maxPerStageDescriptorStorageBuffers  = 64,
> > >.maxPerStageDescriptorSampledImages   = max_samplers,
> > > -  .maxPerStageDescriptorStorageImages   = 64,
> > > +  .maxPerStageDescriptorStorageImages   = max_images,
> > >.maxPerStageDescriptorInputAttachments= 64,
> > >.maxPerStageResources = 250,
> > >.maxDescriptorSetSamplers = 6 *
> > > max_samplers, /* number of stages * maxPerStageDescriptorSamplers
> > > */
> > > @@ -1018,7 +1021,7 @@ void anv_GetPhysicalDeviceProperties(
> > >.maxDescriptorSetStorageBuffers   = 6 *
> > > 64,   /* number of stages *
> > > maxPerStageDescriptorStorageBuffers */
> > >.maxDescriptorSetStorageBuffersDynamic=
> > > MAX_DYNAMIC_BUFFERS / 2,
> > >.maxDescriptorSetSampledImages= 6 *
> > > max_samplers, /* number of stages *
> > > maxPerStageDescriptorSampledImages */
> > > -  .maxDescriptorSetStorageImages= 6 *
> > > 64,   /* number of stages *
> > > maxPerStageDescriptorStorageImages */
> > > +  .maxDescriptorSetStorageImages= 6 *
> > > max_images,   /* number of stages *
> > > maxPerStageDescriptorStorageImages */
> > >.maxDescriptorSetInputAttachments = 256,
> > >.maxVertexInputAttributes = MAX_VBS,
> > >.maxVertexInputBindings   = MAX_VBS,
> > > diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> > > b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> > > index b3daf702bc0..623984b0f8c 100644
> > > --- a/src/intel/vulkan/anv

Re: [Mesa-dev] [PATCH v4] anv/device: fix maximum number of images supported

2019-01-17 Thread Iago Toral 
On Thu, 2019-01-17 at 12:57 -0600, Jason Ekstrand wrote:
> Yup, that'll do it.  Gotta watch out for ++...  Assuming it fixes the
> problem, that patch is
> 
> Reviewed-by: Jason Ekstrand 
> 
> 
> On Thu, Jan 17, 2019 at 12:35 PM Lionel Landwerlin <
> lionel.g.landwer...@intel.com> wrote:
> >   
> > 
> >   
> >   
> > Looking at the change the binding table
> >   emission, I think the image++ has been moved such that it
> > doesn't
> >   produce the same tables anymore.
> > Trying this change on CI :
> > 
https://github.com/djdeath/mesa/commit/a6b8eaf1325389d94d1d8a5b3bb952a362125eb2
> > 
> > 
> > 

Yes, this is what the patch that I sent for review did, I just pushed a
previous version, sorry for the mess :-(
Iago


> > On 17/01/2019 18:19, Clayton Craft
> >   wrote:
> > 
> > 
> > 
> > >   Quoting Mark Janes (2019-01-17 10:13:37)
> > > 
> > >   
> > > > Hi Iago,
> > > > It looks like you tested this patch in CI and got the same
> > > > failures thatwe are seeing on master:
> > > > 
http://mesa-ci-results.jf.intel.com/itoral/builds/263/group/63a9f0ea7bb98050796b649e85481845
> > > > 
> > > >   
> > > 
> > >   The correct link is:
> > > https://mesa-ci.01.org/itoral/builds/263/group/63a9f0ea7bb98050796b649e85481845
> > > 
> > > 
> > >   
> > > > Why was this patch pushed?
> > > > -Mark
> > > > Mark Janes  writes:
> > > > 
> > > > 
> > > > 
> > > > >   This patch regresses thousands of tests on BDW and
> > > > > HSW:
> > > > > 
http://mesa-ci-results.jf.intel.com/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails
> > > > > 
> > > > > 
> > > > 
> > > >   
> > > 
> > >   
> > > https://mesa-ci.01.org/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails
> > > 
> > > 
> > >   
> > > > 
> > > > >   I'll revert it as soon as my testing completes.
> > > > > Iago Toral Quiroga  writes:
> > > > > 
> > > > > 
> > > > >   
> > > > > > We had defined MAX_IMAGES as 8, which we used
> > > > > > to size the array forimage push constant data. The comment
> > > > > > there stated that this was forgen8, but
> > > > > > anv_nir_apply_pipeline_layout runs for all gens and
> > > > > > writesthat array, asserting that we don't exceed that
> > > > > > number of images,which imposes a limit of MAX_IMAGES on all
> > > > > > gens.
> > > > > > Furthermore, despite this, we are exposing up to 64 images
> > > > > > per shaderstage on all gens, gen8 included.
> > > > > > This patch lowers the number of images we expose in gen8 to
> > > > > > 8 andkeeps 64 images for gen9+ while making sure that only
> > > > > > pre-SKL gensuse push constant space to handle images.
> > > > > > v2: - <= instead of < in the assert (Eric, Lionel) - Change
> > > > > > the way the assertion is written (Eric)
> > > > > > v3: - Revert the way the assertion is written to the form
> > > > > > it had in v1,   the version in v2 was not equivalent and
> > > > > > was incorrect. (Lionel)
> > > > > > v4: - gen9+ doesn't need push constants for images at all
> > > > > > (Jason)---
> > > > > > src/intel/vulkan/anv_device.c |  7 --
> > > > > > .../vulkan/anv_nir_apply_pipeline_layout.c|  4 +--
> > > > > > src/intel/vulkan/anv_private.h|  5 ++--
> > > > > > src/intel/vulkan/genX_cmd_buffer.c| 25
> > > > > > +-- 4 files changed, 28 insertions(+), 13
> > > > > > deletions(-)
> > > > > > diff --git a/src/intel/vulkan/anv_device.c
> > > > > > b/src/intel/vulkan/anv_device.cindex
> > > > > > 523f1483e29..f85458b672e 100644---
> > > > > > a/src/intel/vulkan/anv_device.c+++
> > > > > > b/src/intel/vulkan/anv_device.c@@ -987,9 +987,12 @@ void
> > > > > > anv_GetPhysicalDeviceProperties(const uint32_t
> > > > > > max_samplers = (devinfo->gen >= 8 || devinfo->is_haswell)
> > > > > > ?  128 : 16; +   const
> > > > > > uint32_t max_images = devinfo->gen < 9 ? MAX_GEN8_IMAGES :
> > > > > > MAX_IMAGES;+VkSampleCountFlags sample_counts
> > > > > > =   isl_device_get_sample_counts(&pdevice->isl_dev);
> > > > > > +VkPhysicalDeviceLimits limits =
> > > > > > {   .maxImageDimension1D  = (1 <<
> > > > > > 14),   .maxImageDimension2D  = (1
> > > > > > << 14),@@ -1009,7 +1012,7 @@ void
> > > > > > anv_GetPhysicalDeviceProperties(   .maxPerStageDescript
> > > > > > orUniformBuffers  =
> > > > > > 64,   .maxPerStageDescriptorStorageBuffers  =
> > > > > > 64,   .maxPerStageDescriptorSampledImages   =
> > > > > > max_samplers,-  .maxPerStageDescriptorStorageImages
> > > > > >= 64,+  .maxPerStageDescriptorStorageImages   =
> > > > > > max_images,   .maxPerStageDescriptorInputAttachments   
> > > > > >  = 64,   .maxPerStageResources =
> > > > > > 250,   .maxDescriptorSetSamplers

Re: [Mesa-dev] [PATCH v4] anv/device: fix maximum number of images supported

2019-01-17 Thread Jason Ekstrand 
Yup, that'll do it.  Gotta watch out for ++...  Assuming it fixes the
problem, that patch is

Reviewed-by: Jason Ekstrand 

On Thu, Jan 17, 2019 at 12:35 PM Lionel Landwerlin <
lionel.g.landwer...@intel.com> wrote:

> Looking at the change the binding table emission, I think the image++ has
> been moved such that it doesn't produce the same tables anymore.
> Trying this change on CI :
> https://github.com/djdeath/mesa/commit/a6b8eaf1325389d94d1d8a5b3bb952a362125eb2
>
> On 17/01/2019 18:19, Clayton Craft wrote:
>
> Quoting Mark Janes (2019-01-17 10:13:37)
>
> Hi Iago,
>
> It looks like you tested this patch in CI and got the same failures that
> we are seeing on master:
> http://mesa-ci-results.jf.intel.com/itoral/builds/263/group/63a9f0ea7bb98050796b649e85481845
>
> The correct link 
> is:https://mesa-ci.01.org/itoral/builds/263/group/63a9f0ea7bb98050796b649e85481845
>
> Why was this patch pushed?
>
> -Mark
>
> Mark Janes   writes:
>
>
> This patch regresses thousands of tests on BDW and HSW:
> http://mesa-ci-results.jf.intel.com/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails
>
> https://mesa-ci.01.org/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails
>
> I'll revert it as soon as my testing completes.
>
> Iago Toral Quiroga   writes:
>
>
> We had defined MAX_IMAGES as 8, which we used to size the array for
> image push constant data. The comment there stated that this was for
> gen8, but anv_nir_apply_pipeline_layout runs for all gens and writes
> that array, asserting that we don't exceed that number of images,
> which imposes a limit of MAX_IMAGES on all gens.
>
> Furthermore, despite this, we are exposing up to 64 images per shader
> stage on all gens, gen8 included.
>
> This patch lowers the number of images we expose in gen8 to 8 and
> keeps 64 images for gen9+ while making sure that only pre-SKL gens
> use push constant space to handle images.
>
> v2:
>  - <= instead of < in the assert (Eric, Lionel)
>  - Change the way the assertion is written (Eric)
>
> v3:
>  - Revert the way the assertion is written to the form it had in v1,
>the version in v2 was not equivalent and was incorrect. (Lionel)
>
> v4:
>  - gen9+ doesn't need push constants for images at all (Jason)
> ---
>  src/intel/vulkan/anv_device.c |  7 --
>  .../vulkan/anv_nir_apply_pipeline_layout.c|  4 +--
>  src/intel/vulkan/anv_private.h|  5 ++--
>  src/intel/vulkan/genX_cmd_buffer.c| 25 +--
>  4 files changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> index 523f1483e29..f85458b672e 100644
> --- a/src/intel/vulkan/anv_device.c
> +++ b/src/intel/vulkan/anv_device.c
> @@ -987,9 +987,12 @@ void anv_GetPhysicalDeviceProperties(
> const uint32_t max_samplers = (devinfo->gen >= 8 || devinfo->is_haswell) ?
>   128 : 16;
>
> +   const uint32_t max_images = devinfo->gen < 9 ? MAX_GEN8_IMAGES : 
> MAX_IMAGES;
> +
> VkSampleCountFlags sample_counts =
>isl_device_get_sample_counts(&pdevice->isl_dev);
>
> +
> VkPhysicalDeviceLimits limits = {
>.maxImageDimension1D  = (1 << 14),
>.maxImageDimension2D  = (1 << 14),
> @@ -1009,7 +1012,7 @@ void anv_GetPhysicalDeviceProperties(
>.maxPerStageDescriptorUniformBuffers  = 64,
>.maxPerStageDescriptorStorageBuffers  = 64,
>.maxPerStageDescriptorSampledImages   = max_samplers,
> -  .maxPerStageDescriptorStorageImages   = 64,
> +  .maxPerStageDescriptorStorageImages   = max_images,
>.maxPerStageDescriptorInputAttachments= 64,
>.maxPerStageResources = 250,
>.maxDescriptorSetSamplers = 6 * max_samplers, /* 
> number of stages * maxPerStageDescriptorSamplers */
> @@ -1018,7 +1021,7 @@ void anv_GetPhysicalDeviceProperties(
>.maxDescriptorSetStorageBuffers   = 6 * 64,   /* 
> number of stages * maxPerStageDescriptorStorageBuffers */
>.maxDescriptorSetStorageBuffersDynamic= MAX_DYNAMIC_BUFFERS / 2,
>.maxDescriptorSetSampledImages= 6 * max_samplers, /* 
> number of stages * maxPerStageDescriptorSampledImages */
> -  .maxDescriptorSetStorageImages= 6 * 64,   /* 
> number of stages * maxPerStageDescriptorStorageImages */
> +  .maxDescriptorSetStorageImages= 6 * max_images,   /* 
> number of stages * maxPerStageDescriptorStorageImages */
>.maxDescriptorSetInputAttachments = 256,
>.maxVertexInputAttributes = MAX_VBS,
>.maxVertexInputBindings   = MAX_VBS,
> diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c 
> b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> index b3daf702bc0..623984b0f8c 100644
> --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
>

Re: [Mesa-dev] [PATCH v4] anv/device: fix maximum number of images supported

2019-01-17 Thread Lionel Landwerlin 
Looking at the change the binding table emission, I think the image++ 
has been moved such that it doesn't produce the same tables anymore.
Trying this change on CI : 
https://github.com/djdeath/mesa/commit/a6b8eaf1325389d94d1d8a5b3bb952a362125eb2


On 17/01/2019 18:19, Clayton Craft wrote:

Quoting Mark Janes (2019-01-17 10:13:37)

Hi Iago,

It looks like you tested this patch in CI and got the same failures that
we are seeing on master:

http://mesa-ci-results.jf.intel.com/itoral/builds/263/group/63a9f0ea7bb98050796b649e85481845

The correct link is:
https://mesa-ci.01.org/itoral/builds/263/group/63a9f0ea7bb98050796b649e85481845


Why was this patch pushed?

-Mark

Mark Janes  writes:


This patch regresses thousands of tests on BDW and HSW:

http://mesa-ci-results.jf.intel.com/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails

https://mesa-ci.01.org/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails


I'll revert it as soon as my testing completes.

Iago Toral Quiroga  writes:


We had defined MAX_IMAGES as 8, which we used to size the array for
image push constant data. The comment there stated that this was for
gen8, but anv_nir_apply_pipeline_layout runs for all gens and writes
that array, asserting that we don't exceed that number of images,
which imposes a limit of MAX_IMAGES on all gens.

Furthermore, despite this, we are exposing up to 64 images per shader
stage on all gens, gen8 included.

This patch lowers the number of images we expose in gen8 to 8 and
keeps 64 images for gen9+ while making sure that only pre-SKL gens
use push constant space to handle images.

v2:
  - <= instead of < in the assert (Eric, Lionel)
  - Change the way the assertion is written (Eric)

v3:
  - Revert the way the assertion is written to the form it had in v1,
the version in v2 was not equivalent and was incorrect. (Lionel)

v4:
  - gen9+ doesn't need push constants for images at all (Jason)
---
  src/intel/vulkan/anv_device.c |  7 --
  .../vulkan/anv_nir_apply_pipeline_layout.c|  4 +--
  src/intel/vulkan/anv_private.h|  5 ++--
  src/intel/vulkan/genX_cmd_buffer.c| 25 +--
  4 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 523f1483e29..f85458b672e 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -987,9 +987,12 @@ void anv_GetPhysicalDeviceProperties(
 const uint32_t max_samplers = (devinfo->gen >= 8 || devinfo->is_haswell) ?
   128 : 16;
  
+   const uint32_t max_images = devinfo->gen < 9 ? MAX_GEN8_IMAGES : MAX_IMAGES;

+
 VkSampleCountFlags sample_counts =
isl_device_get_sample_counts(&pdevice->isl_dev);
  
+

 VkPhysicalDeviceLimits limits = {
.maxImageDimension1D  = (1 << 14),
.maxImageDimension2D  = (1 << 14),
@@ -1009,7 +1012,7 @@ void anv_GetPhysicalDeviceProperties(
.maxPerStageDescriptorUniformBuffers  = 64,
.maxPerStageDescriptorStorageBuffers  = 64,
.maxPerStageDescriptorSampledImages   = max_samplers,
-  .maxPerStageDescriptorStorageImages   = 64,
+  .maxPerStageDescriptorStorageImages   = max_images,
.maxPerStageDescriptorInputAttachments= 64,
.maxPerStageResources = 250,
.maxDescriptorSetSamplers = 6 * max_samplers, /* number 
of stages * maxPerStageDescriptorSamplers */
@@ -1018,7 +1021,7 @@ void anv_GetPhysicalDeviceProperties(
.maxDescriptorSetStorageBuffers   = 6 * 64,   /* number 
of stages * maxPerStageDescriptorStorageBuffers */
.maxDescriptorSetStorageBuffersDynamic= MAX_DYNAMIC_BUFFERS / 2,
.maxDescriptorSetSampledImages= 6 * max_samplers, /* number 
of stages * maxPerStageDescriptorSampledImages */
-  .maxDescriptorSetStorageImages= 6 * 64,   /* number 
of stages * maxPerStageDescriptorStorageImages */
+  .maxDescriptorSetStorageImages= 6 * max_images,   /* number 
of stages * maxPerStageDescriptorStorageImages */
.maxDescriptorSetInputAttachments = 256,
.maxVertexInputAttributes = MAX_VBS,
.maxVertexInputBindings   = MAX_VBS,
diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c 
b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
index b3daf702bc0..623984b0f8c 100644
--- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
+++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
@@ -528,8 +528,8 @@ anv_nir_apply_pipeline_layout(const struct 
anv_physical_device *pdevice,
}
 }
  
-   if (map->image_count > 0) {

-  assert(map->image_count <= MAX_IMAGES);
+   if (map->image_count > 0 && pdevice->compiler->devinfo->gen < 9) {
+  assert(map->image_count <= MAX_GEN8_IMAGES);

Re: [Mesa-dev] [PATCH v4] anv/device: fix maximum number of images supported

2019-01-17 Thread Clayton Craft 
Quoting Mark Janes (2019-01-17 10:13:37)
> Hi Iago,
> 
> It looks like you tested this patch in CI and got the same failures that
> we are seeing on master:
> 
> http://mesa-ci-results.jf.intel.com/itoral/builds/263/group/63a9f0ea7bb98050796b649e85481845

The correct link is:
https://mesa-ci.01.org/itoral/builds/263/group/63a9f0ea7bb98050796b649e85481845

> 
> Why was this patch pushed?
> 
> -Mark
> 
> Mark Janes  writes:
> 
> > This patch regresses thousands of tests on BDW and HSW:
> >
> > http://mesa-ci-results.jf.intel.com/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails

https://mesa-ci.01.org/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails

> >
> > I'll revert it as soon as my testing completes.
> >
> > Iago Toral Quiroga  writes:
> >
> >> We had defined MAX_IMAGES as 8, which we used to size the array for
> >> image push constant data. The comment there stated that this was for
> >> gen8, but anv_nir_apply_pipeline_layout runs for all gens and writes
> >> that array, asserting that we don't exceed that number of images,
> >> which imposes a limit of MAX_IMAGES on all gens.
> >>
> >> Furthermore, despite this, we are exposing up to 64 images per shader
> >> stage on all gens, gen8 included.
> >>
> >> This patch lowers the number of images we expose in gen8 to 8 and
> >> keeps 64 images for gen9+ while making sure that only pre-SKL gens
> >> use push constant space to handle images.
> >>
> >> v2:
> >>  - <= instead of < in the assert (Eric, Lionel)
> >>  - Change the way the assertion is written (Eric)
> >>
> >> v3:
> >>  - Revert the way the assertion is written to the form it had in v1,
> >>the version in v2 was not equivalent and was incorrect. (Lionel)
> >>
> >> v4:
> >>  - gen9+ doesn't need push constants for images at all (Jason)
> >> ---
> >>  src/intel/vulkan/anv_device.c |  7 --
> >>  .../vulkan/anv_nir_apply_pipeline_layout.c|  4 +--
> >>  src/intel/vulkan/anv_private.h|  5 ++--
> >>  src/intel/vulkan/genX_cmd_buffer.c| 25 +--
> >>  4 files changed, 28 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> >> index 523f1483e29..f85458b672e 100644
> >> --- a/src/intel/vulkan/anv_device.c
> >> +++ b/src/intel/vulkan/anv_device.c
> >> @@ -987,9 +987,12 @@ void anv_GetPhysicalDeviceProperties(
> >> const uint32_t max_samplers = (devinfo->gen >= 8 || 
> >> devinfo->is_haswell) ?
> >>   128 : 16;
> >>  
> >> +   const uint32_t max_images = devinfo->gen < 9 ? MAX_GEN8_IMAGES : 
> >> MAX_IMAGES;
> >> +
> >> VkSampleCountFlags sample_counts =
> >>isl_device_get_sample_counts(&pdevice->isl_dev);
> >>  
> >> +
> >> VkPhysicalDeviceLimits limits = {
> >>.maxImageDimension1D  = (1 << 14),
> >>.maxImageDimension2D  = (1 << 14),
> >> @@ -1009,7 +1012,7 @@ void anv_GetPhysicalDeviceProperties(
> >>.maxPerStageDescriptorUniformBuffers  = 64,
> >>.maxPerStageDescriptorStorageBuffers  = 64,
> >>.maxPerStageDescriptorSampledImages   = max_samplers,
> >> -  .maxPerStageDescriptorStorageImages   = 64,
> >> +  .maxPerStageDescriptorStorageImages   = max_images,
> >>.maxPerStageDescriptorInputAttachments= 64,
> >>.maxPerStageResources = 250,
> >>.maxDescriptorSetSamplers = 6 * max_samplers, /* 
> >> number of stages * maxPerStageDescriptorSamplers */
> >> @@ -1018,7 +1021,7 @@ void anv_GetPhysicalDeviceProperties(
> >>.maxDescriptorSetStorageBuffers   = 6 * 64,   /* 
> >> number of stages * maxPerStageDescriptorStorageBuffers */
> >>.maxDescriptorSetStorageBuffersDynamic= MAX_DYNAMIC_BUFFERS / 2,
> >>.maxDescriptorSetSampledImages= 6 * max_samplers, /* 
> >> number of stages * maxPerStageDescriptorSampledImages */
> >> -  .maxDescriptorSetStorageImages= 6 * 64,   /* 
> >> number of stages * maxPerStageDescriptorStorageImages */
> >> +  .maxDescriptorSetStorageImages= 6 * max_images,   /* 
> >> number of stages * maxPerStageDescriptorStorageImages */
> >>.maxDescriptorSetInputAttachments = 256,
> >>.maxVertexInputAttributes = MAX_VBS,
> >>.maxVertexInputBindings   = MAX_VBS,
> >> diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c 
> >> b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> >> index b3daf702bc0..623984b0f8c 100644
> >> --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> >> +++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> >> @@ -528,8 +528,8 @@ anv_nir_apply_pipeline_layout(const struct 
> >> anv_physical_device *pdevice,
> >>}
> >> }
> >>  
> >> -   if (map->image_count > 0) {
> >> -  assert(map->image_count <= MAX

Re: [Mesa-dev] [PATCH v4] anv/device: fix maximum number of images supported

2019-01-17 Thread Mark Janes 
Hi Iago,

It looks like you tested this patch in CI and got the same failures that
we are seeing on master:

http://mesa-ci-results.jf.intel.com/itoral/builds/263/group/63a9f0ea7bb98050796b649e85481845

Why was this patch pushed?

-Mark

Mark Janes  writes:

> This patch regresses thousands of tests on BDW and HSW:
>
> http://mesa-ci-results.jf.intel.com/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails
>
> I'll revert it as soon as my testing completes.
>
> Iago Toral Quiroga  writes:
>
>> We had defined MAX_IMAGES as 8, which we used to size the array for
>> image push constant data. The comment there stated that this was for
>> gen8, but anv_nir_apply_pipeline_layout runs for all gens and writes
>> that array, asserting that we don't exceed that number of images,
>> which imposes a limit of MAX_IMAGES on all gens.
>>
>> Furthermore, despite this, we are exposing up to 64 images per shader
>> stage on all gens, gen8 included.
>>
>> This patch lowers the number of images we expose in gen8 to 8 and
>> keeps 64 images for gen9+ while making sure that only pre-SKL gens
>> use push constant space to handle images.
>>
>> v2:
>>  - <= instead of < in the assert (Eric, Lionel)
>>  - Change the way the assertion is written (Eric)
>>
>> v3:
>>  - Revert the way the assertion is written to the form it had in v1,
>>the version in v2 was not equivalent and was incorrect. (Lionel)
>>
>> v4:
>>  - gen9+ doesn't need push constants for images at all (Jason)
>> ---
>>  src/intel/vulkan/anv_device.c |  7 --
>>  .../vulkan/anv_nir_apply_pipeline_layout.c|  4 +--
>>  src/intel/vulkan/anv_private.h|  5 ++--
>>  src/intel/vulkan/genX_cmd_buffer.c| 25 +--
>>  4 files changed, 28 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
>> index 523f1483e29..f85458b672e 100644
>> --- a/src/intel/vulkan/anv_device.c
>> +++ b/src/intel/vulkan/anv_device.c
>> @@ -987,9 +987,12 @@ void anv_GetPhysicalDeviceProperties(
>> const uint32_t max_samplers = (devinfo->gen >= 8 || devinfo->is_haswell) 
>> ?
>>   128 : 16;
>>  
>> +   const uint32_t max_images = devinfo->gen < 9 ? MAX_GEN8_IMAGES : 
>> MAX_IMAGES;
>> +
>> VkSampleCountFlags sample_counts =
>>isl_device_get_sample_counts(&pdevice->isl_dev);
>>  
>> +
>> VkPhysicalDeviceLimits limits = {
>>.maxImageDimension1D  = (1 << 14),
>>.maxImageDimension2D  = (1 << 14),
>> @@ -1009,7 +1012,7 @@ void anv_GetPhysicalDeviceProperties(
>>.maxPerStageDescriptorUniformBuffers  = 64,
>>.maxPerStageDescriptorStorageBuffers  = 64,
>>.maxPerStageDescriptorSampledImages   = max_samplers,
>> -  .maxPerStageDescriptorStorageImages   = 64,
>> +  .maxPerStageDescriptorStorageImages   = max_images,
>>.maxPerStageDescriptorInputAttachments= 64,
>>.maxPerStageResources = 250,
>>.maxDescriptorSetSamplers = 6 * max_samplers, /* 
>> number of stages * maxPerStageDescriptorSamplers */
>> @@ -1018,7 +1021,7 @@ void anv_GetPhysicalDeviceProperties(
>>.maxDescriptorSetStorageBuffers   = 6 * 64,   /* 
>> number of stages * maxPerStageDescriptorStorageBuffers */
>>.maxDescriptorSetStorageBuffersDynamic= MAX_DYNAMIC_BUFFERS / 2,
>>.maxDescriptorSetSampledImages= 6 * max_samplers, /* 
>> number of stages * maxPerStageDescriptorSampledImages */
>> -  .maxDescriptorSetStorageImages= 6 * 64,   /* 
>> number of stages * maxPerStageDescriptorStorageImages */
>> +  .maxDescriptorSetStorageImages= 6 * max_images,   /* 
>> number of stages * maxPerStageDescriptorStorageImages */
>>.maxDescriptorSetInputAttachments = 256,
>>.maxVertexInputAttributes = MAX_VBS,
>>.maxVertexInputBindings   = MAX_VBS,
>> diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c 
>> b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
>> index b3daf702bc0..623984b0f8c 100644
>> --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
>> +++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
>> @@ -528,8 +528,8 @@ anv_nir_apply_pipeline_layout(const struct 
>> anv_physical_device *pdevice,
>>}
>> }
>>  
>> -   if (map->image_count > 0) {
>> -  assert(map->image_count <= MAX_IMAGES);
>> +   if (map->image_count > 0 && pdevice->compiler->devinfo->gen < 9) {
>> +  assert(map->image_count <= MAX_GEN8_IMAGES);
>>assert(shader->num_uniforms == prog_data->nr_params * 4);
>>state.first_image_uniform = shader->num_uniforms;
>>uint32_t *param = brw_stage_prog_data_add_params(prog_data,
>> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
>> index 770254e93ea..47878adb

Re: [Mesa-dev] [PATCH v4] anv/device: fix maximum number of images supported

2019-01-17 Thread Mark Janes 
This patch regresses thousands of tests on BDW and HSW:

http://mesa-ci-results.jf.intel.com/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails

I'll revert it as soon as my testing completes.

Iago Toral Quiroga  writes:

> We had defined MAX_IMAGES as 8, which we used to size the array for
> image push constant data. The comment there stated that this was for
> gen8, but anv_nir_apply_pipeline_layout runs for all gens and writes
> that array, asserting that we don't exceed that number of images,
> which imposes a limit of MAX_IMAGES on all gens.
>
> Furthermore, despite this, we are exposing up to 64 images per shader
> stage on all gens, gen8 included.
>
> This patch lowers the number of images we expose in gen8 to 8 and
> keeps 64 images for gen9+ while making sure that only pre-SKL gens
> use push constant space to handle images.
>
> v2:
>  - <= instead of < in the assert (Eric, Lionel)
>  - Change the way the assertion is written (Eric)
>
> v3:
>  - Revert the way the assertion is written to the form it had in v1,
>the version in v2 was not equivalent and was incorrect. (Lionel)
>
> v4:
>  - gen9+ doesn't need push constants for images at all (Jason)
> ---
>  src/intel/vulkan/anv_device.c |  7 --
>  .../vulkan/anv_nir_apply_pipeline_layout.c|  4 +--
>  src/intel/vulkan/anv_private.h|  5 ++--
>  src/intel/vulkan/genX_cmd_buffer.c| 25 +--
>  4 files changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> index 523f1483e29..f85458b672e 100644
> --- a/src/intel/vulkan/anv_device.c
> +++ b/src/intel/vulkan/anv_device.c
> @@ -987,9 +987,12 @@ void anv_GetPhysicalDeviceProperties(
> const uint32_t max_samplers = (devinfo->gen >= 8 || devinfo->is_haswell) ?
>   128 : 16;
>  
> +   const uint32_t max_images = devinfo->gen < 9 ? MAX_GEN8_IMAGES : 
> MAX_IMAGES;
> +
> VkSampleCountFlags sample_counts =
>isl_device_get_sample_counts(&pdevice->isl_dev);
>  
> +
> VkPhysicalDeviceLimits limits = {
>.maxImageDimension1D  = (1 << 14),
>.maxImageDimension2D  = (1 << 14),
> @@ -1009,7 +1012,7 @@ void anv_GetPhysicalDeviceProperties(
>.maxPerStageDescriptorUniformBuffers  = 64,
>.maxPerStageDescriptorStorageBuffers  = 64,
>.maxPerStageDescriptorSampledImages   = max_samplers,
> -  .maxPerStageDescriptorStorageImages   = 64,
> +  .maxPerStageDescriptorStorageImages   = max_images,
>.maxPerStageDescriptorInputAttachments= 64,
>.maxPerStageResources = 250,
>.maxDescriptorSetSamplers = 6 * max_samplers, /* 
> number of stages * maxPerStageDescriptorSamplers */
> @@ -1018,7 +1021,7 @@ void anv_GetPhysicalDeviceProperties(
>.maxDescriptorSetStorageBuffers   = 6 * 64,   /* 
> number of stages * maxPerStageDescriptorStorageBuffers */
>.maxDescriptorSetStorageBuffersDynamic= MAX_DYNAMIC_BUFFERS / 2,
>.maxDescriptorSetSampledImages= 6 * max_samplers, /* 
> number of stages * maxPerStageDescriptorSampledImages */
> -  .maxDescriptorSetStorageImages= 6 * 64,   /* 
> number of stages * maxPerStageDescriptorStorageImages */
> +  .maxDescriptorSetStorageImages= 6 * max_images,   /* 
> number of stages * maxPerStageDescriptorStorageImages */
>.maxDescriptorSetInputAttachments = 256,
>.maxVertexInputAttributes = MAX_VBS,
>.maxVertexInputBindings   = MAX_VBS,
> diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c 
> b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> index b3daf702bc0..623984b0f8c 100644
> --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> +++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> @@ -528,8 +528,8 @@ anv_nir_apply_pipeline_layout(const struct 
> anv_physical_device *pdevice,
>}
> }
>  
> -   if (map->image_count > 0) {
> -  assert(map->image_count <= MAX_IMAGES);
> +   if (map->image_count > 0 && pdevice->compiler->devinfo->gen < 9) {
> +  assert(map->image_count <= MAX_GEN8_IMAGES);
>assert(shader->num_uniforms == prog_data->nr_params * 4);
>state.first_image_uniform = shader->num_uniforms;
>uint32_t *param = brw_stage_prog_data_add_params(prog_data,
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
> index 770254e93ea..47878adb066 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -157,7 +157,8 @@ struct gen_l3_config;
>  #define MAX_SCISSORS16
>  #define MAX_PUSH_CONSTANTS_SIZE 128
>  #define MAX_DYNAMIC_BUFFERS 16
> -#define MAX_IMAGES 8
> +#define MAX_IMAGES 64
> +#define MAX_GEN8_IMAGES 8
>  #define MAX_PUSH_DESCRIPTORS 32 /* Minimum requi

Re: [Mesa-dev] [PATCH v4] anv/device: fix maximum number of images supported

2019-01-16 Thread Jason Ekstrand 
Yup.

Reviewed-by: Jason Ekstrand 

On Wed, Jan 16, 2019 at 6:08 AM Iago Toral  wrote:

> Jason, does this version look good to you?
>
> On Mon, 2019-01-14 at 12:42 +0100, Iago Toral Quiroga wrote:
> > We had defined MAX_IMAGES as 8, which we used to size the array for
> > image push constant data. The comment there stated that this was for
> > gen8, but anv_nir_apply_pipeline_layout runs for all gens and writes
> > that array, asserting that we don't exceed that number of images,
> > which imposes a limit of MAX_IMAGES on all gens.
> >
> > Furthermore, despite this, we are exposing up to 64 images per shader
> > stage on all gens, gen8 included.
> >
> > This patch lowers the number of images we expose in gen8 to 8 and
> > keeps 64 images for gen9+ while making sure that only pre-SKL gens
> > use push constant space to handle images.
> >
> > v2:
> >  - <= instead of < in the assert (Eric, Lionel)
> >  - Change the way the assertion is written (Eric)
> >
> > v3:
> >  - Revert the way the assertion is written to the form it had in v1,
> >the version in v2 was not equivalent and was incorrect. (Lionel)
> >
> > v4:
> >  - gen9+ doesn't need push constants for images at all (Jason)
> > ---
> >  src/intel/vulkan/anv_device.c |  7 --
> >  .../vulkan/anv_nir_apply_pipeline_layout.c|  4 +--
> >  src/intel/vulkan/anv_private.h|  5 ++--
> >  src/intel/vulkan/genX_cmd_buffer.c| 25 +--
> > 
> >  4 files changed, 28 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/intel/vulkan/anv_device.c
> > b/src/intel/vulkan/anv_device.c
> > index 523f1483e29..f85458b672e 100644
> > --- a/src/intel/vulkan/anv_device.c
> > +++ b/src/intel/vulkan/anv_device.c
> > @@ -987,9 +987,12 @@ void anv_GetPhysicalDeviceProperties(
> > const uint32_t max_samplers = (devinfo->gen >= 8 || devinfo-
> > >is_haswell) ?
> >   128 : 16;
> >
> > +   const uint32_t max_images = devinfo->gen < 9 ? MAX_GEN8_IMAGES :
> > MAX_IMAGES;
> > +
> > VkSampleCountFlags sample_counts =
> >isl_device_get_sample_counts(&pdevice->isl_dev);
> >
> > +
> > VkPhysicalDeviceLimits limits = {
> >.maxImageDimension1D  = (1 << 14),
> >.maxImageDimension2D  = (1 << 14),
> > @@ -1009,7 +1012,7 @@ void anv_GetPhysicalDeviceProperties(
> >.maxPerStageDescriptorUniformBuffers  = 64,
> >.maxPerStageDescriptorStorageBuffers  = 64,
> >.maxPerStageDescriptorSampledImages   = max_samplers,
> > -  .maxPerStageDescriptorStorageImages   = 64,
> > +  .maxPerStageDescriptorStorageImages   = max_images,
> >.maxPerStageDescriptorInputAttachments= 64,
> >.maxPerStageResources = 250,
> >.maxDescriptorSetSamplers = 6 * max_samplers,
> > /* number of stages * maxPerStageDescriptorSamplers */
> > @@ -1018,7 +1021,7 @@ void anv_GetPhysicalDeviceProperties(
> >.maxDescriptorSetStorageBuffers   = 6 *
> > 64,   /* number of stages *
> > maxPerStageDescriptorStorageBuffers */
> >.maxDescriptorSetStorageBuffersDynamic=
> > MAX_DYNAMIC_BUFFERS / 2,
> >.maxDescriptorSetSampledImages= 6 * max_samplers,
> > /* number of stages * maxPerStageDescriptorSampledImages */
> > -  .maxDescriptorSetStorageImages= 6 *
> > 64,   /* number of stages *
> > maxPerStageDescriptorStorageImages */
> > +  .maxDescriptorSetStorageImages= 6 *
> > max_images,   /* number of stages *
> > maxPerStageDescriptorStorageImages */
> >.maxDescriptorSetInputAttachments = 256,
> >.maxVertexInputAttributes = MAX_VBS,
> >.maxVertexInputBindings   = MAX_VBS,
> > diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> > b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> > index b3daf702bc0..623984b0f8c 100644
> > --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> > +++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> > @@ -528,8 +528,8 @@ anv_nir_apply_pipeline_layout(const struct
> > anv_physical_device *pdevice,
> >}
> > }
> >
> > -   if (map->image_count > 0) {
> > -  assert(map->image_count <= MAX_IMAGES);
> > +   if (map->image_count > 0 && pdevice->compiler->devinfo->gen < 9)
> > {
> > +  assert(map->image_count <= MAX_GEN8_IMAGES);
> >assert(shader->num_uniforms == prog_data->nr_params * 4);
> >state.first_image_uniform = shader->num_uniforms;
> >uint32_t *param = brw_stage_prog_data_add_params(prog_data,
> > diff --git a/src/intel/vulkan/anv_private.h
> > b/src/intel/vulkan/anv_private.h
> > index 770254e93ea..47878adb066 100644
> > --- a/src/intel/vulkan/anv_private.h
> > +++ b/src/intel/vulkan/anv_private.h
> > @@ -157,7 +157,8 @@ struct gen_l3_config;
> >  #define MAX_SCISSORS16
> >  #define MAX_PUSH_CONSTANTS_

Re: [Mesa-dev] [PATCH v4] anv/device: fix maximum number of images supported

2019-01-16 Thread Iago Toral 
Jason, does this version look good to you?

On Mon, 2019-01-14 at 12:42 +0100, Iago Toral Quiroga wrote:
> We had defined MAX_IMAGES as 8, which we used to size the array for
> image push constant data. The comment there stated that this was for
> gen8, but anv_nir_apply_pipeline_layout runs for all gens and writes
> that array, asserting that we don't exceed that number of images,
> which imposes a limit of MAX_IMAGES on all gens.
> 
> Furthermore, despite this, we are exposing up to 64 images per shader
> stage on all gens, gen8 included.
> 
> This patch lowers the number of images we expose in gen8 to 8 and
> keeps 64 images for gen9+ while making sure that only pre-SKL gens
> use push constant space to handle images.
> 
> v2:
>  - <= instead of < in the assert (Eric, Lionel)
>  - Change the way the assertion is written (Eric)
> 
> v3:
>  - Revert the way the assertion is written to the form it had in v1,
>the version in v2 was not equivalent and was incorrect. (Lionel)
> 
> v4:
>  - gen9+ doesn't need push constants for images at all (Jason)
> ---
>  src/intel/vulkan/anv_device.c |  7 --
>  .../vulkan/anv_nir_apply_pipeline_layout.c|  4 +--
>  src/intel/vulkan/anv_private.h|  5 ++--
>  src/intel/vulkan/genX_cmd_buffer.c| 25 +--
> 
>  4 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/src/intel/vulkan/anv_device.c
> b/src/intel/vulkan/anv_device.c
> index 523f1483e29..f85458b672e 100644
> --- a/src/intel/vulkan/anv_device.c
> +++ b/src/intel/vulkan/anv_device.c
> @@ -987,9 +987,12 @@ void anv_GetPhysicalDeviceProperties(
> const uint32_t max_samplers = (devinfo->gen >= 8 || devinfo-
> >is_haswell) ?
>   128 : 16;
>  
> +   const uint32_t max_images = devinfo->gen < 9 ? MAX_GEN8_IMAGES :
> MAX_IMAGES;
> +
> VkSampleCountFlags sample_counts =
>isl_device_get_sample_counts(&pdevice->isl_dev);
>  
> +
> VkPhysicalDeviceLimits limits = {
>.maxImageDimension1D  = (1 << 14),
>.maxImageDimension2D  = (1 << 14),
> @@ -1009,7 +1012,7 @@ void anv_GetPhysicalDeviceProperties(
>.maxPerStageDescriptorUniformBuffers  = 64,
>.maxPerStageDescriptorStorageBuffers  = 64,
>.maxPerStageDescriptorSampledImages   = max_samplers,
> -  .maxPerStageDescriptorStorageImages   = 64,
> +  .maxPerStageDescriptorStorageImages   = max_images,
>.maxPerStageDescriptorInputAttachments= 64,
>.maxPerStageResources = 250,
>.maxDescriptorSetSamplers = 6 * max_samplers,
> /* number of stages * maxPerStageDescriptorSamplers */
> @@ -1018,7 +1021,7 @@ void anv_GetPhysicalDeviceProperties(
>.maxDescriptorSetStorageBuffers   = 6 *
> 64,   /* number of stages *
> maxPerStageDescriptorStorageBuffers */
>.maxDescriptorSetStorageBuffersDynamic=
> MAX_DYNAMIC_BUFFERS / 2,
>.maxDescriptorSetSampledImages= 6 * max_samplers,
> /* number of stages * maxPerStageDescriptorSampledImages */
> -  .maxDescriptorSetStorageImages= 6 *
> 64,   /* number of stages *
> maxPerStageDescriptorStorageImages */
> +  .maxDescriptorSetStorageImages= 6 *
> max_images,   /* number of stages *
> maxPerStageDescriptorStorageImages */
>.maxDescriptorSetInputAttachments = 256,
>.maxVertexInputAttributes = MAX_VBS,
>.maxVertexInputBindings   = MAX_VBS,
> diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> index b3daf702bc0..623984b0f8c 100644
> --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> +++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> @@ -528,8 +528,8 @@ anv_nir_apply_pipeline_layout(const struct
> anv_physical_device *pdevice,
>}
> }
>  
> -   if (map->image_count > 0) {
> -  assert(map->image_count <= MAX_IMAGES);
> +   if (map->image_count > 0 && pdevice->compiler->devinfo->gen < 9)
> {
> +  assert(map->image_count <= MAX_GEN8_IMAGES);
>assert(shader->num_uniforms == prog_data->nr_params * 4);
>state.first_image_uniform = shader->num_uniforms;
>uint32_t *param = brw_stage_prog_data_add_params(prog_data,
> diff --git a/src/intel/vulkan/anv_private.h
> b/src/intel/vulkan/anv_private.h
> index 770254e93ea..47878adb066 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -157,7 +157,8 @@ struct gen_l3_config;
>  #define MAX_SCISSORS16
>  #define MAX_PUSH_CONSTANTS_SIZE 128
>  #define MAX_DYNAMIC_BUFFERS 16
> -#define MAX_IMAGES 8
> +#define MAX_IMAGES 64
> +#define MAX_GEN8_IMAGES 8
>  #define MAX_PUSH_DESCRIPTORS 32 /* Minimum requirement */
>  
>  /* The kernel relocation API has a limitation of a 32-bit delta
> value
> @@ -1883,7 +1884,7 @@ stru

[Mesa-dev] [PATCH v4] anv/device: fix maximum number of images supported

2019-01-14 Thread Iago Toral Quiroga 
We had defined MAX_IMAGES as 8, which we used to size the array for
image push constant data. The comment there stated that this was for
gen8, but anv_nir_apply_pipeline_layout runs for all gens and writes
that array, asserting that we don't exceed that number of images,
which imposes a limit of MAX_IMAGES on all gens.

Furthermore, despite this, we are exposing up to 64 images per shader
stage on all gens, gen8 included.

This patch lowers the number of images we expose in gen8 to 8 and
keeps 64 images for gen9+ while making sure that only pre-SKL gens
use push constant space to handle images.

v2:
 - <= instead of < in the assert (Eric, Lionel)
 - Change the way the assertion is written (Eric)

v3:
 - Revert the way the assertion is written to the form it had in v1,
   the version in v2 was not equivalent and was incorrect. (Lionel)

v4:
 - gen9+ doesn't need push constants for images at all (Jason)
---
 src/intel/vulkan/anv_device.c |  7 --
 .../vulkan/anv_nir_apply_pipeline_layout.c|  4 +--
 src/intel/vulkan/anv_private.h|  5 ++--
 src/intel/vulkan/genX_cmd_buffer.c| 25 +--
 4 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 523f1483e29..f85458b672e 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -987,9 +987,12 @@ void anv_GetPhysicalDeviceProperties(
const uint32_t max_samplers = (devinfo->gen >= 8 || devinfo->is_haswell) ?
  128 : 16;
 
+   const uint32_t max_images = devinfo->gen < 9 ? MAX_GEN8_IMAGES : MAX_IMAGES;
+
VkSampleCountFlags sample_counts =
   isl_device_get_sample_counts(&pdevice->isl_dev);
 
+
VkPhysicalDeviceLimits limits = {
   .maxImageDimension1D  = (1 << 14),
   .maxImageDimension2D  = (1 << 14),
@@ -1009,7 +1012,7 @@ void anv_GetPhysicalDeviceProperties(
   .maxPerStageDescriptorUniformBuffers  = 64,
   .maxPerStageDescriptorStorageBuffers  = 64,
   .maxPerStageDescriptorSampledImages   = max_samplers,
-  .maxPerStageDescriptorStorageImages   = 64,
+  .maxPerStageDescriptorStorageImages   = max_images,
   .maxPerStageDescriptorInputAttachments= 64,
   .maxPerStageResources = 250,
   .maxDescriptorSetSamplers = 6 * max_samplers, /* number 
of stages * maxPerStageDescriptorSamplers */
@@ -1018,7 +1021,7 @@ void anv_GetPhysicalDeviceProperties(
   .maxDescriptorSetStorageBuffers   = 6 * 64,   /* number 
of stages * maxPerStageDescriptorStorageBuffers */
   .maxDescriptorSetStorageBuffersDynamic= MAX_DYNAMIC_BUFFERS / 2,
   .maxDescriptorSetSampledImages= 6 * max_samplers, /* number 
of stages * maxPerStageDescriptorSampledImages */
-  .maxDescriptorSetStorageImages= 6 * 64,   /* number 
of stages * maxPerStageDescriptorStorageImages */
+  .maxDescriptorSetStorageImages= 6 * max_images,   /* number 
of stages * maxPerStageDescriptorStorageImages */
   .maxDescriptorSetInputAttachments = 256,
   .maxVertexInputAttributes = MAX_VBS,
   .maxVertexInputBindings   = MAX_VBS,
diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c 
b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
index b3daf702bc0..623984b0f8c 100644
--- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
+++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
@@ -528,8 +528,8 @@ anv_nir_apply_pipeline_layout(const struct 
anv_physical_device *pdevice,
   }
}
 
-   if (map->image_count > 0) {
-  assert(map->image_count <= MAX_IMAGES);
+   if (map->image_count > 0 && pdevice->compiler->devinfo->gen < 9) {
+  assert(map->image_count <= MAX_GEN8_IMAGES);
   assert(shader->num_uniforms == prog_data->nr_params * 4);
   state.first_image_uniform = shader->num_uniforms;
   uint32_t *param = brw_stage_prog_data_add_params(prog_data,
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index 770254e93ea..47878adb066 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -157,7 +157,8 @@ struct gen_l3_config;
 #define MAX_SCISSORS16
 #define MAX_PUSH_CONSTANTS_SIZE 128
 #define MAX_DYNAMIC_BUFFERS 16
-#define MAX_IMAGES 8
+#define MAX_IMAGES 64
+#define MAX_GEN8_IMAGES 8
 #define MAX_PUSH_DESCRIPTORS 32 /* Minimum requirement */
 
 /* The kernel relocation API has a limitation of a 32-bit delta value
@@ -1883,7 +1884,7 @@ struct anv_push_constants {
uint32_t base_work_group_id[3];
 
/* Image data for image_load_store on pre-SKL */
-   struct brw_image_param images[MAX_IMAGES];
+   struct brw_image_param images[MAX_GEN8_IMAGES];
 };
 
 struct anv_dynamic_state {
diff --git a/src/intel/vulkan/genX_cmd_buffer.c 
b/src/intel/vulkan/genX_cmd_b