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

[Lionel Landwerlin]

On 11/01/2019 12: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)
> ---
>   src/intel/vulkan/anv_device.c                    | 7 +++++--
>   src/intel/vulkan/anv_nir_apply_pipeline_layout.c | 3 ++-
>   src/intel/vulkan/anv_private.h                   | 4 ++--
>   3 files changed, 9 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..53de347b3c 100644
> --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> +++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> @@ -529,7 +529,8 @@ anv_nir_apply_pipeline_layout(const struct anv_physical_device *pdevice,
>      }
>   
>      if (map->image_count > 0) {
> -      assert(map->image_count <= MAX_IMAGES);
> +      assert((pdevice->compiler->devinfo->gen <= 8 && map->image_count <= MAX_GEN8_IMAGES) ||
> +              map->image_count <= MAX_IMAGES);


I'm not sure why it wasn't < instead of <= previously, sounds like it 
should be <.


Also I don't think this version works, your previous version was okay :


image_count < MAX_GEN8_IMAGE || (gen > 8 && image_count < MAX_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];
>   };
>