[Mesa-dev] [PATCH] anv/device: fix maximum number of images supported
Iago Toral
itoral at igalia.com
Fri Jan 11 11:44:52 UTC 2019
On Fri, 2019-01-11 at 11:43 +0000, Lionel Landwerlin wrote:
> 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?
Yes, Eric also noticed that. I'll fix that and I'll change the assert
to look like Eric suggested as well.
Thanks!
>
> -
>
> 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
>
>
>
More information about the mesa-dev
mailing list