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

[Lionel Landwerlin]

On 11/01/2019 11:37, Eric Engestrom wrote:
> On Friday, 2019-01-11 10:58:38 +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+. 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.
>> ---
>>   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 cd179e6801..122a58cfd6 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 a0fd226b0a..e16ad4f032 100644
>> --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
>> +++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
>> @@ -537,7 +537,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));
> s/ < / <= /, but I would also write it differently:
>
>    assert((pdevice->compiler->devinfo->gen <= 8 && map->image_count <= MAX_GEN8_IMAGES) ||
>           map->image_count <= MAX_IMAGES);
>
> I don't know about the numbers, but code-wise it looks all reasonable.


Isn't there a consistency issue here?


image_count <= MAX_IMAGES on gen9+

image_count < MAX_GEN8_IMAGES gen8 and below


Surely we want the same comparison operator regardless of the gen?


-

Lionel


>
>>         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 903931472d..3af7982bae 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
>> @@ -1864,7 +1865,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];
>>   };
>>   
>> -- 
>> 2.17.1
>>
>> _______________________________________________
>> 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