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.ja...@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 <ito...@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@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
