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

[Iago Toral]

On Fri, 2019-01-11 at 11:14 -0600, Jason Ekstrand wrote:
> On Fri, Jan 11, 2019 at 9:13 AM Iago Toral Quiroga <itoral at igalia.com
> > 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)
> > 
> > 
> > 
> > v3:
> > 
> >  - Revert the way the assertion is written to the for it had in v1,
> > 
> >    the version in v2 was not equivalent and was incorrect. (Lionel)
> > 
> > ---
> > 
> >  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 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..ae5c19578c 100644
> > 
> > --- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> > 
> > +++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
> > 
> > @@ -529,7 +529,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));
> > 
> >        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];
> 
> Why are we dropping the comment and why isn't this MAX_GEN8_IMAGES? 
> The push params shouldn't be needed by gen9+ hence our ability to
> increase it to 64 without blowing out our push constant space.

The reason for this is that anv_nir_apply_pipeline_layout is writing
one slot per image in the shader, on all gens. So if we have 16 images,
we are writing 16 slots, even on gen9+. I take that what you mean that
we can skip this for gen9+? I'll try that and send a new patch.
>  
> >  };
> > 
> > 
> > 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190114/661fce83/attachment.html>