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

Clayton Craft clayton.a.craft at intel.com
Thu Jan 17 18:19:41 UTC 2019


Quoting Mark Janes (2019-01-17 10:13:37)
> Hi Iago,
> 
> It looks like you tested this patch in CI and got the same failures that
> we are seeing on master:
> 
> http://mesa-ci-results.jf.intel.com/itoral/builds/263/group/63a9f0ea7bb98050796b649e85481845

The correct link is:
https://mesa-ci.01.org/itoral/builds/263/group/63a9f0ea7bb98050796b649e85481845

> 
> Why was this patch pushed?
> 
> -Mark
> 
> Mark Janes <mark.a.janes at intel.com> writes:
> 
> > This patch regresses thousands of tests on BDW and HSW:
> >
> > http://mesa-ci-results.jf.intel.com/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails

https://mesa-ci.01.org/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails

> >
> > I'll revert it as soon as my testing completes.
> >
> > Iago Toral Quiroga <itoral at igalia.com> writes:
> >
> >> 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:
> >> -- 
> >> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190117/01c992a0/attachment.sig>


More information about the mesa-dev mailing list