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

[Lionel Landwerlin]

On 11/01/2019 12:40, Iago Toral wrote:
> On Fri, 2019-01-11 at 12:31 +0000, Lionel Landwerlin wrote:
>> 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 <.
> I think <= is fine, map->image_count is not an index but the total
> count so its value should be fine up to the maximum allowed, right?


Oh dear, sorry, you're right.


>
>> Also I don't think this version works, your previous version was okay
>> :
>>
>>
>> image_count < MAX_GEN8_IMAGE || (gen > 8 && image_count < MAX_IMAGES)
> Right, I didn't read Eric's proposal properly, and it would not work
> for gen8. I'll revert the form of the assertion to the previous
> version.
>
>>>          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];
>>>    };
>>>    
>>
>>
>