[Mesa-dev] [PATCH v4] anv/device: fix maximum number of images supported
Iago Toral
itoral at igalia.com
Fri Jan 18 09:51:32 UTC 2019
Argh, I am really sorry about that :-(
It seems I didn't push the right version patch (the one I sent for
review) but a previous version of that. The patch that Lionel sent to
fix this is exactly what I had changed in the version I sent for
review.
I am dorry for the mess, I'll be more careful next time.
Iago
On Thu, 2019-01-17 at 10:13 -0800, Mark Janes wrote:
> 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
>
> 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
> >
> > 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
>
>
More information about the mesa-dev
mailing list