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

Iago Toral itoral at igalia.com
Wed Jan 16 12:08:58 UTC 2019


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_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
> @@ -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_buffer.c
> index 35a70f7fe37..e23f8cfda45 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -2006,6 +2006,7 @@ emit_binding_table(struct anv_cmd_buffer
> *cmd_buffer,
>                     gl_shader_stage stage,
>                     struct anv_state *bt_state)
>  {
> +   const struct gen_device_info *devinfo = &cmd_buffer->device-
> >info;
>     struct anv_subpass *subpass = cmd_buffer->state.subpass;
>     struct anv_cmd_pipeline_state *pipe_state;
>     struct anv_pipeline *pipeline;
> @@ -2063,7 +2064,8 @@ emit_binding_table(struct anv_cmd_buffer
> *cmd_buffer,
>     if (map->surface_count == 0)
>        goto out;
>  
> -   if (map->image_count > 0) {
> +   /* We only use push constant space for images before gen9 */
> +   if (map->image_count > 0 && devinfo->gen < 9) {
>        VkResult result =
>           anv_cmd_buffer_ensure_push_constant_field(cmd_buffer,
> stage, images);
>        if (result != VK_SUCCESS)
> @@ -2177,10 +2179,15 @@ emit_binding_table(struct anv_cmd_buffer
> *cmd_buffer,
>           assert(surface_state.alloc_size);
>           add_surface_state_relocs(cmd_buffer, sstate);
>  
> -         struct brw_image_param *image_param =
> -            &cmd_buffer->state.push_constants[stage]-
> >images[image++];
> +         if (devinfo->gen < 9) {
> +            assert(image < MAX_GEN8_IMAGES);
> +            struct brw_image_param *image_param =
> +               &cmd_buffer->state.push_constants[stage]-
> >images[image];
>  
> -         *image_param = desc->image_view->planes[binding-
> >plane].storage_image_param;
> +            *image_param =
> +               desc->image_view->planes[binding-
> >plane].storage_image_param;
> +         }
> +         image++;
>           break;
>        }
>  
> @@ -2226,10 +2233,14 @@ emit_binding_table(struct anv_cmd_buffer
> *cmd_buffer,
>           add_surface_reloc(cmd_buffer, surface_state,
>                             desc->buffer_view->address);
>  
> -         struct brw_image_param *image_param =
> -            &cmd_buffer->state.push_constants[stage]-
> >images[image++];
> +         if (devinfo->gen < 9) {
> +            assert(image < MAX_GEN8_IMAGES);
> +            struct brw_image_param *image_param =
> +               &cmd_buffer->state.push_constants[stage]-
> >images[image];
>  
> -         *image_param = desc->buffer_view->storage_image_param;
> +            *image_param = desc->buffer_view->storage_image_param;
> +         }
> +         image++;
>           break;
>  
>        default:



More information about the mesa-dev mailing list