[v3] anv/device: fix maximum number of images supported

Submitted by Iago Toral Quiroga on Jan. 11, 2019, 3:12 p.m.

Details

Message ID 20190111151258.4706-1-itoral@igalia.com
State New
Headers show
Series "anv/device: fix maximum number of images supported" ( rev: 3 ) in Mesa

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga Jan. 11, 2019, 3:12 p.m.
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+. It also sets MAX_IMAGES to 64 so we can
actually use up to 64 images in shaders, and then adds an assert
specific to gen8 to check that we never exceed 8.

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 for it had in v1,
   the version in v2 was not equivalent and was incorrect. (Lionel)
---
 src/intel/vulkan/anv_device.c                    | 7 +++++--
 src/intel/vulkan/anv_nir_apply_pipeline_layout.c | 4 +++-
 src/intel/vulkan/anv_private.h                   | 4 ++--
 3 files changed, 10 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 523f1483e2..f85458b672 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 b3daf702bc..ae5c19578c 100644
--- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
+++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
@@ -529,7 +529,9 @@  anv_nir_apply_pipeline_layout(const struct anv_physical_device *pdevice,
    }
 
    if (map->image_count > 0) {
-      assert(map->image_count <= MAX_IMAGES);
+      assert(map->image_count <= MAX_IMAGES &&
+             (pdevice->compiler->devinfo->gen > 8 ||
+              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 770254e93e..9ddd41b1fa 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_SCISSORS    16
 #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
@@ -1882,7 +1883,6 @@  struct anv_push_constants {
    /* Used for vkCmdDispatchBase */
    uint32_t base_work_group_id[3];
 
-   /* Image data for image_load_store on pre-SKL */
    struct brw_image_param images[MAX_IMAGES];
 };
 

Comments

On 11/01/2019 15:12, 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+. It also sets MAX_IMAGES to 64 so we can
> actually use up to 64 images in shaders, and then adds an assert
> specific to gen8 to check that we never exceed 8.
>
> 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 for it had in v1,
>     the version in v2 was not equivalent and was incorrect. (Lionel)


Thanks! Do you think this should go to stable?

Anyway :


Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>



> ---
>   src/intel/vulkan/anv_device.c                    | 7 +++++--
>   src/intel/vulkan/anv_nir_apply_pipeline_layout.c | 4 +++-
>   src/intel/vulkan/anv_private.h                   | 4 ++--
>   3 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> index 523f1483e2..f85458b672 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 b3daf702bc..ae5c19578c 100644
> --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> +++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> @@ -529,7 +529,9 @@ anv_nir_apply_pipeline_layout(const struct anv_physical_device *pdevice,
>      }
>   
>      if (map->image_count > 0) {
> -      assert(map->image_count <= MAX_IMAGES);
> +      assert(map->image_count <= MAX_IMAGES &&
> +             (pdevice->compiler->devinfo->gen > 8 ||
> +              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 770254e93e..9ddd41b1fa 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_SCISSORS    16
>   #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
> @@ -1882,7 +1883,6 @@ struct anv_push_constants {
>      /* Used for vkCmdDispatchBase */
>      uint32_t base_work_group_id[3];
>   
> -   /* Image data for image_load_store on pre-SKL */
>      struct brw_image_param images[MAX_IMAGES];
>   };
>
On Fri, 2019-01-11 at 15:24 +0000, Lionel Landwerlin wrote:
> On 11/01/2019 15:12, 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+. It also sets MAX_IMAGES to 64 so we can
> > actually use up to 64 images in shaders, and then adds an assert
> > specific to gen8 to check that we never exceed 8.
> > 
> > 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 for it had in
> > v1,
> >     the version in v2 was not equivalent and was incorrect.
> > (Lionel)
> 
> 
> Thanks! Do you think this should go to stable?

Yes, I think it should, I'll add a CC to stable when I push the patch,
but I'll wait for Jason's ack before doing that since I discussed the
underlying problem with him before.

Iago

> Anyway :
> 
> 
> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> 
> 
> 
> > ---
> >   src/intel/vulkan/anv_device.c                    | 7 +++++--
> >   src/intel/vulkan/anv_nir_apply_pipeline_layout.c | 4 +++-
> >   src/intel/vulkan/anv_private.h                   | 4 ++--
> >   3 files changed, 10 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/intel/vulkan/anv_device.c
> > b/src/intel/vulkan/anv_device.c
> > index 523f1483e2..f85458b672 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 b3daf702bc..ae5c19578c 100644
> > --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> > +++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> > @@ -529,7 +529,9 @@ anv_nir_apply_pipeline_layout(const struct
> > anv_physical_device *pdevice,
> >      }
> >   
> >      if (map->image_count > 0) {
> > -      assert(map->image_count <= MAX_IMAGES);
> > +      assert(map->image_count <= MAX_IMAGES &&
> > +             (pdevice->compiler->devinfo->gen > 8 ||
> > +              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 770254e93e..9ddd41b1fa 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_SCISSORS    16
> >   #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
> > @@ -1882,7 +1883,6 @@ struct anv_push_constants {
> >      /* Used for vkCmdDispatchBase */
> >      uint32_t base_work_group_id[3];
> >   
> > -   /* Image data for image_load_store on pre-SKL */
> >      struct brw_image_param images[MAX_IMAGES];
> >   };
> >   
> 
> 
>
On Fri, Jan 11, 2019 at 9:13 AM Iago Toral Quiroga <itoral@igalia.com>
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+. It also sets MAX_IMAGES to 64 so we can
> actually use up to 64 images in shaders, and then adds an assert
> specific to gen8 to check that we never exceed 8.
>
> 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 for it had in v1,
>    the version in v2 was not equivalent and was incorrect. (Lionel)
> ---
>  src/intel/vulkan/anv_device.c                    | 7 +++++--
>  src/intel/vulkan/anv_nir_apply_pipeline_layout.c | 4 +++-
>  src/intel/vulkan/anv_private.h                   | 4 ++--
>  3 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> index 523f1483e2..f85458b672 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 b3daf702bc..ae5c19578c 100644
> --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> +++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> @@ -529,7 +529,9 @@ anv_nir_apply_pipeline_layout(const struct
> anv_physical_device *pdevice,
>     }
>
>     if (map->image_count > 0) {
> -      assert(map->image_count <= MAX_IMAGES);
> +      assert(map->image_count <= MAX_IMAGES &&
> +             (pdevice->compiler->devinfo->gen > 8 ||
> +              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 770254e93e..9ddd41b1fa 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_SCISSORS    16
>  #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
> @@ -1882,7 +1883,6 @@ struct anv_push_constants {
>     /* Used for vkCmdDispatchBase */
>     uint32_t base_work_group_id[3];
>
> -   /* Image data for image_load_store on pre-SKL */
>     struct brw_image_param images[MAX_IMAGES];
>

Why are we dropping the comment and why isn't this MAX_GEN8_IMAGES?  The
push params shouldn't be needed by gen9+ hence our ability to increase it
to 64 without blowing out our push constant space.


>  };
>
> --
> 2.17.1
>
>
On Fri, 2019-01-11 at 11:14 -0600, Jason Ekstrand wrote:
> On Fri, Jan 11, 2019 at 9:13 AM Iago Toral Quiroga <itoral@igalia.com
> > 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+. It also sets MAX_IMAGES to 64 so we can
> > 
> > actually use up to 64 images in shaders, and then adds an assert
> > 
> > specific to gen8 to check that we never exceed 8.
> > 
> > 
> > 
> > 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 for it had in v1,
> > 
> >    the version in v2 was not equivalent and was incorrect. (Lionel)
> > 
> > ---
> > 
> >  src/intel/vulkan/anv_device.c                    | 7 +++++--
> > 
> >  src/intel/vulkan/anv_nir_apply_pipeline_layout.c | 4 +++-
> > 
> >  src/intel/vulkan/anv_private.h                   | 4 ++--
> > 
> >  3 files changed, 10 insertions(+), 5 deletions(-)
> > 
> > 
> > 
> > diff --git a/src/intel/vulkan/anv_device.c
> > b/src/intel/vulkan/anv_device.c
> > 
> > index 523f1483e2..f85458b672 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 b3daf702bc..ae5c19578c 100644
> > 
> > --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> > 
> > +++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> > 
> > @@ -529,7 +529,9 @@ anv_nir_apply_pipeline_layout(const struct
> > anv_physical_device *pdevice,
> > 
> >     }
> > 
> > 
> > 
> >     if (map->image_count > 0) {
> > 
> > -      assert(map->image_count <= MAX_IMAGES);
> > 
> > +      assert(map->image_count <= MAX_IMAGES &&
> > 
> > +             (pdevice->compiler->devinfo->gen > 8 ||
> > 
> > +              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 770254e93e..9ddd41b1fa 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_SCISSORS    16
> > 
> >  #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
> > 
> > @@ -1882,7 +1883,6 @@ struct anv_push_constants {
> > 
> >     /* Used for vkCmdDispatchBase */
> > 
> >     uint32_t base_work_group_id[3];
> > 
> > 
> > 
> > -   /* Image data for image_load_store on pre-SKL */
> > 
> >     struct brw_image_param images[MAX_IMAGES];
> 
> Why are we dropping the comment and why isn't this MAX_GEN8_IMAGES? 
> The push params shouldn't be needed by gen9+ hence our ability to
> increase it to 64 without blowing out our push constant space.

The reason for this is that anv_nir_apply_pipeline_layout is writing
one slot per image in the shader, on all gens. So if we have 16 images,
we are writing 16 slots, even on gen9+. I take that what you mean that
we can skip this for gen9+? I'll try that and send a new patch.
>  
> >  };
> > 
> > 
> >