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.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 https://mesa-ci.01.org/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
signature.asc
Description: signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
